Closed Bug 235187 Opened 21 years ago Closed 20 years ago

nsContextMenuInfo shouldn't use low-level API and remove presContext FindFrameBackground()

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: p_ch, Assigned: p_ch)

References

Details

Attachments

(1 file, 2 obsolete files)

patch v2.1
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.
Attached patch patch (obsolete) — — Splinter Review 
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.
s/the im1 the priority.../the im1 has the priority...
> 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.
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?
Within two weeks I can manage it.  ;)
Attached patch patch v2.0 (obsolete) — — Splinter Review 
> 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.
Attachment #141939 - Attachment is obsolete: true
Attachment #142366 - Flags: superreview?(bzbarsky)
Attachment #142366 - Flags: review?(ccarlen)
Blocks: 235189
Is there a reason to no longer cache the request and just redo all the work of
getting it every time?
(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) {

?
yes, do_QUeryInterface(null) is safe.
(In reply to comment #9)
> yes, do_QUeryInterface(null) is safe.
Thanks!
Attached patch patch v2.1 — — Splinter Review 
I added some NS_ENSURE_TRUE to pretty up the patch a bit and updated per above
comments.
Attachment #142366 - Attachment is obsolete: true
Attachment #142366 - Flags: superreview?(bzbarsky)
Attachment #142366 - Flags: review?(ccarlen)
Attachment #142458 - Flags: superreview?(bzbarsky)
Attachment #142458 - Flags: review?(ccarlen)
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 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+
(In reply to comment #13)
> That should be checking *aRequest, right?
oops... my bad...
checked in. Thanks for the quick reviews!
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: