Closed
Bug 235187
Opened 21 years ago
Closed 21 years ago
nsContextMenuInfo shouldn't use low-level API and remove presContext FindFrameBackground()
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: p_ch, Assigned: p_ch)
References
Details
Attachments
(1 file, 2 obsolete files)
|
19.71 KB,
patch
|
ccarlen
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
As Boris suggested in bug 119735, the only caller of FindFrameBackground is
nsContextMenuInfo.cpp and it shouldn't rely on low-level API but rather on the
DOM and computed styles.
|
Assignee | |
Comment 1•21 years ago
|
||
To test this patch, I copied the methods in one of our interface, since firefox
doesn't use nsContextMenuInfo.
This patch assumes that for html document with:
html { background-image: url(im1);}
body { background-image: url(im2);)
the im1 the priority...
That's not what gecko does... im2 is displayed on top of im1...
I need to know what should be done.
|
|
Assignee | |
Comment 2•21 years ago
|
||
s/the im1 the priority.../the im1 has the priority...
|
||
Comment 3•21 years ago
|
||
> the im1 the priority...
> That's not what gecko does... im2 is displayed on top of im1...
im1 has the priority when determining the _canvas_ background. im2 is painted
as the _body_ background. They are painted in different rects. So the question
is where the context menu was brought up. In other words, use the first
background you come to as you traverse parents.
|
|
Assignee | |
Comment 4•21 years ago
|
||
bz: ok, that makes a lot more sense. And the patch will be even shorter :)
will you have time to give some r/sr love on it (within sth like 2 weeks) or
should I bug someone else?
|
|
||
Comment 5•21 years ago
|
||
Within two weeks I can manage it. ;)
|
|
Assignee | |
Comment 6•21 years ago
|
||
> In other words, use the first background you come to as you traverse parents.
hum, no, unless I can get the computed style of the canvas.
if the input DOM node is the <html> element and a background-image is only
specified on the <body> element, I'll have to get the <body> computed style
that is not reachable when traversing parents.
This patch improves the implementation in nsContextMenu.js in two ways:
- the HTML node rwon't always return the <body> background-image (see line 318
in nsContextMenu.js)
- the loop over the parents of the node ends if we hit a non-transparent
background-color.
|
|
Assignee | |
Updated•21 years ago
|
Attachment #141939 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•21 years ago
|
Attachment #142366 -
Flags: superreview?(bzbarsky)
Attachment #142366 -
Flags: review?(ccarlen)
|
|
||
Comment 7•21 years ago
|
||
Is there a reason to no longer cache the request and just redo all the work of
getting it every time?
|
|
Assignee | |
Comment 8•21 years ago
|
||
(In reply to comment #7)
> Is there a reason to no longer cache the request and just redo all the work of
> getting it every time?
Well, I can add it back to your appreciation, but that seemed to me like an
overoptimization, this part of the code is not perf sensitive. In my tests, if
we were caching, we could gain 0.1 to 0.5ms in GetBackGroundImageRequest on my
slow laptop depending on whether a background image is present. That's true for
the second and following times it is called and given that the context menu is
still on the same DOM node. The time spent would be a lot for an internal
routine, but that's not much compared to human perception time scale.
btw, is it safe/good style shortening (and sparing an indentation):
- computedStyle->GetPropertyCSSValue(NS_LITERAL_STRING("background-color"),
- getter_AddRefs(cssValue));
- if (cssValue) {
- primitiveValue = do_QueryInterface(cssValue);
- if (primitiveValue) {
to:
+ computedStyle->GetPropertyCSSValue(NS_LITERAL_STRING("background-color"),
+ getter_AddRefs(cssValue));
+ primitiveValue = do_QueryInterface(cssValue);
+ if (primitiveValue) {
?
|
||
Comment 9•21 years ago
|
||
yes, do_QUeryInterface(null) is safe.
|
|
Assignee | |
Comment 10•21 years ago
|
||
(In reply to comment #9)
> yes, do_QUeryInterface(null) is safe.
Thanks!
|
|
Assignee | |
Comment 11•21 years ago
|
||
I added some NS_ENSURE_TRUE to pretty up the patch a bit and updated per above
comments.
Attachment #142366 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•21 years ago
|
Attachment #142366 -
Flags: superreview?(bzbarsky)
Attachment #142366 -
Flags: review?(ccarlen)
|
|
Assignee | |
Updated•21 years ago
|
Attachment #142458 -
Flags: superreview?(bzbarsky)
Attachment #142458 -
Flags: review?(ccarlen)
|
||
Comment 12•21 years ago
|
||
Comment on attachment 142458 [details] [diff] [review]
patch v2.1
Thanks - that's so much better. r=ccarlen
Attachment #142458 -
Flags: review?(ccarlen) → review+
|
|
||
Comment 13•21 years ago
|
||
Comment on attachment 142458 [details] [diff] [review]
patch v2.1
>Index: embedding/browser/webBrowser/nsContextMenuInfo.cpp
>+nsContextMenuInfo::GetBackgroundImageRequest(nsIDOMNode *aDOMNode, imgIRequest **aRequest)
>+ nsresult rv = GetBackgroundImageRequestInternal(domNode, aRequest);
>+ if (NS_SUCCEEDED(rv) && aRequest)
>+ return NS_OK;
That should be checking *aRequest, right?
Other than that, looks good. sr=bzbarsky.
Attachment #142458 -
Flags: superreview?(bzbarsky) → superreview+
|
|
Assignee | |
Comment 14•21 years ago
|
||
(In reply to comment #13)
> That should be checking *aRequest, right?
oops... my bad...
|
|
Assignee | |
Comment 15•21 years ago
|
||
checked in. Thanks for the quick reviews!
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•