Closed Bug 562339 Opened 14 years ago Closed 14 years ago

Clean up nsContextMenu.js, lay the base for future work touching it

Categories

(SeaMonkey :: UI Design, defect)

Product:
SeaMonkey
Component:
UI Design
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.1a2

People

(Reporter: kairo, Assigned: kairo)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 3 obsolete files)

diff -w for easier review
66.12 KB, patch
Details | Diff | Splinter Review
v1.2: make ellipsis a lazy getter
110.59 KB, patch
neil
: review+
Details | Diff | Splinter Review 
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/browser/base/content/nsContextMenu.js and http://hg.mozilla.org/mozilla-central/log/tip/browser/base/content/nsContextMenu.js have a long history, and our current version is even missing the step where that CVS log starts - which I noticed now as we don't have a var for the browser set, but it would come in handy with my places work in bug 498596.

My intent is to look at porting bug 356590 and those things that followed that are reasonable for us, as I found that file could really need some love.
Version: unspecified → Trunk
OK, lied about catching up much more with Firefox. :P

In the middle of the work, I figured that alone porting the relevant parts of bug 356590 and cleaning up indentation/whitespace as well as small bits of other code style makes this end up in such a huge patch that I don't want to push anything else on top of it.
Here's the actual patch, doing a ton of whitespace cleanup and some other code style cleanup, as well as some minor code changes such as the addition of this.browser as in the noted Firefox bug.

Diffstat says |3 files changed, 1210 insertions(+), 1188 deletions(-)| so this actual diff is not for the faint of the heart. I'll attach a diff -w for easier reviewing in a minute.
Attachment #442133 - Flags: review?(neil)
Here's the |diff -w| that reduces the changes to |3 files changed, 291 insertions(+), 269 deletions(-)| which should be better reviewable.
Oh, note that the default engine stuff (as well as the gDefaultEngine var) have been unused since the search item doesn't tell the name of the engine we're using for the search - it just has never been removed...
Comment on attachment 442134 [details] [diff] [review]
diff -w for easier review

>+  this.browser           = null;
Unused? (Plus all the dependent stuff...)

>-        this.showItem( "context-sep-properties", !( this.inDirList || this.isContentSelected || this.onTextInput ||
>+    this.showItem("context-sep-properties",
>+                  !(this.inDirList || this.isContentSelected || this.onTextInput ||
>                                                     this.onCanvas || this.onVideo || this.onAudio ) );
[Are you removing spaces before )s as well as after (s or is -w hiding that?]

>+    this.showItem("context-fitimage", this.onStandaloneImage &&
>+                                      content.document.imageResizingEnabled);
You wrapped the other showItem calls with each parameter on its own line - was this one subtly different because it happens to fit in 80 characters this way?

>+      else if ( this.target instanceof HTMLHtmlElement ) {
>                // pages with multiple <body>s are lame. we'll teach them a lesson.
>-               var bodyElt = this.target.ownerDocument.getElementsByTagName("body")[0];
>+        var bodyElt = this.target.ownerDocument.body;
This isn't quite the same, as you're assuming that an HTMLHtmlElement lives inside an HTMLDocument ;-)

>+    return this.linkProtocol && !(
>+           this.linkProtocol == "mailto"     ||
>+           this.linkProtocol == "javascript" ||
>+           this.linkProtocol == "news"       ||
>+           this.linkProtocol == "snews"      );
[Odd bracketing!]

>-        var timeToWait =
>-                Components.classes["@mozilla.org/preferences-service;1"]
>+    var timeToWait = Components.classes["@mozilla.org/preferences-service;1"]
>                           .getService(Components.interfaces.nsIPrefBranch)
[Still waiting for Services I see!]

>     ///////////////
>     // Utilities //
>     ///////////////
[I wonder how many of these we still use?]

>+      searchSelectText = searchSelectText.substr(0, 15) + this.ellipsis;
[Can we avoid fetching the ellipsis unless we use it?]
(In reply to comment #5)
> (From update of attachment 442134 [details] [diff] [review])
> >+  this.browser           = null;
> Unused? (Plus all the dependent stuff...)

Unused as of now, yes (it's only being set for the moment), I will use it in the places bookmarks patches, but I didn't want to add even more stuff to the work there.

> >-        this.showItem( "context-sep-properties", !( this.inDirList || this.isContentSelected || this.onTextInput ||
> >+    this.showItem("context-sep-properties",
> >+                  !(this.inDirList || this.isContentSelected || this.onTextInput ||
> >                                                     this.onCanvas || this.onVideo || this.onAudio ) );
> [Are you removing spaces before )s as well as after (s or is -w hiding that?]

Yes, removing both, -w is hiding that.

> >+    this.showItem("context-fitimage", this.onStandaloneImage &&
> >+                                      content.document.imageResizingEnabled);
> You wrapped the other showItem calls with each parameter on its own line - was
> this one subtly different because it happens to fit in 80 characters this way?

Yes, but I can change that one if you want.

> >+      else if ( this.target instanceof HTMLHtmlElement ) {
> >                // pages with multiple <body>s are lame. we'll teach them a lesson.
> >-               var bodyElt = this.target.ownerDocument.getElementsByTagName("body")[0];
> >+        var bodyElt = this.target.ownerDocument.body;
> This isn't quite the same, as you're assuming that an HTMLHtmlElement lives
> inside an HTMLDocument ;-)

I do, and Firefox does as well, that's why I copied this as a simplification.

> >+    return this.linkProtocol && !(
> >+           this.linkProtocol == "mailto"     ||
> >+           this.linkProtocol == "javascript" ||
> >+           this.linkProtocol == "news"       ||
> >+           this.linkProtocol == "snews"      );
> [Odd bracketing!]

Copied that from Firefox, can change it to more normal one if you want.

> >-        var timeToWait =
> >-                Components.classes["@mozilla.org/preferences-service;1"]
> >+    var timeToWait = Components.classes["@mozilla.org/preferences-service;1"]
> >                           .getService(Components.interfaces.nsIPrefBranch)
> [Still waiting for Services I see!]

Yes, I didn't want to put even more stuff into this patch when I saw how large it's been growing.

> >     ///////////////
> >     // Utilities //
> >     ///////////////
> [I wonder how many of these we still use?]

I'll file a followup to look into that even more.

> >+      searchSelectText = searchSelectText.substr(0, 15) + this.ellipsis;
> [Can we avoid fetching the ellipsis unless we use it?]

Good question. Firefox uses it in two places, we only do in one - should I just move all the fetching into this function?
(In reply to comment #6)
>(In reply to comment #5)
>>(From update of attachment 442134 [details] [diff] [review])
>>>+  this.browser           = null;
>>Unused? (Plus all the dependent stuff...)
>Unused as of now, yes (it's only being set for the moment), I will use it in
>the places bookmarks patches, but I didn't want to add even more stuff to the
>work there.
"will"? so as yet I don't know what it's for?

>>>+      searchSelectText = searchSelectText.substr(0, 15) + this.ellipsis;
>>[Can we avoid fetching the ellipsis unless we use it?]
>Good question. Firefox uses it in two places, we only do in one - should I just
>move all the fetching into this function?
Out of interest, what's the second place?
(In reply to comment #7)
> (In reply to comment #6)
> >(In reply to comment #5)
> >>(From update of attachment 442134 [details] [diff] [review])
> >>>+  this.browser           = null;
> >>Unused? (Plus all the dependent stuff...)
> >Unused as of now, yes (it's only being set for the moment), I will use it in
> >the places bookmarks patches, but I didn't want to add even more stuff to the
> >work there.
> "will"? so as yet I don't know what it's for?

http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/nsContextMenu.js#1415 is the part I'd like to copy and that uses it (and which led me to do this whole patch). Firefox uses this.browser in a few other places in that file, it might be interesting to investigate those cases as there might be some where we can use it as well to make code easier to understand.

> >>>+      searchSelectText = searchSelectText.substr(0, 15) + this.ellipsis;
> >>[Can we avoid fetching the ellipsis unless we use it?]
> >Good question. Firefox uses it in two places, we only do in one - should I just
> >move all the fetching into this function?
> Out of interest, what's the second place?

http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/nsContextMenu.js#351 - apparently their image blocking entry.
(In reply to comment #6)
>(In reply to comment #5)
>>(From update of attachment 442134 [details] [diff] [review])
>>>+  this.browser           = null;
>>Unused? (Plus all the dependent stuff...)
Oh, I see you mentioned it in comment #0, but it wasn't clear...
(In reply to comment #5)
>(From update of attachment 442134 [details] [diff] [review])
>>+      searchSelectText = searchSelectText.substr(0, 15) + this.ellipsis;
>[Can we avoid fetching the ellipsis unless we use it?]
PlacesUIUtils has a lazy ellipsis getter which we could copy.
(In reply to comment #9)
> Oh, I see you mentioned it in comment #0, but it wasn't clear...

Yup, I hope comment #8 made it clearer - this was the original cause why I started looking into this in the first place. ;-)

(In reply to comment #10)
> PlacesUIUtils has a lazy ellipsis getter which we could copy.

Which uses XPCOMUtils - I thought that would be a module-only thing? Can I use it here and can I be sure it's available here? We are not in a module here, and included in different places...
Comment on attachment 442134 [details] [diff] [review]
diff -w for easier review

>+  // Generate fully qualified URL for clicked-on link.
>+  getLinkURL: function() {
>+    if (this.link.href)
>           return this.link.href;
>+
>+    href = this.link.getAttributeNS("http://www.w3.org/1999/xlink", "href");
As mentioned on IRC, should be var href = ...
(In reply to comment #11)
> (In reply to comment #10)
> > PlacesUIUtils has a lazy ellipsis getter which we could copy.
> Which uses XPCOMUtils - I thought that would be a module-only thing? Can I use
> it here and can I be sure it's available here? We are not in a module here, and
> included in different places...
I was thinking you could just copy the 2-4 lines of code:
XPCOMUtils.defineLazyGetter(nsContextMenu.prototype, "ellipsis", function() {
  return Services.prefs.getComplexValue("intl.ellipsis",
      Components.interfaces.nsIPrefLocalizedString).data;
});
(In reply to comment #13)
> I was thinking you could just copy the 2-4 lines of code:
> XPCOMUtils.defineLazyGetter(nsContextMenu.prototype, "ellipsis", function() {
>   return Services.prefs.getComplexValue("intl.ellipsis",
>       Components.interfaces.nsIPrefLocalizedString).data;
> });

Exactly what I thought. When I do that, I get:

Error: XPCOMUtils is not defined
Source File: chrome://communicator/content/nsContextMenu.js
Line: 1348

Note that nsContextMenu isn't a JS component or module (though the latter might have some some merit as a future target).
Attached patch v1.1: minor fixes to the patch (obsolete) — — Splinter Review 
Here's an updated patch with the var Ian mentioned and two or three other small fixes.
Attachment #442133 - Attachment is obsolete: true
Attachment #445210 - Flags: review?(neil)
Attachment #442133 - Flags: review?(neil)
If you like, I can update the -w as well, but if you have a decent interdiff tool, comparing the first and 1.1 patch is easier than re-reading a -w diff.
OK, as the import should be cheap, here's the patch with the lazy getter.
Attachment #445210 - Attachment is obsolete: true
Attachment #445216 - Flags: review?(neil)
Attachment #445210 - Flags: review?(neil)
Comment on attachment 445216 [details] [diff] [review]
v1.2: make ellipsis a lazy getter

This isn't bug 525869...
Attachment #445216 - Flags: review?(neil)
Comment on attachment 445216 [details] [diff] [review]
v1.2: make ellipsis a lazy getter

Damn.... Sorry, obviously picked a wrong patch when attaching. I should clean up my patches directories.
Attachment #445216 - Attachment is obsolete: true
This is the actually right patch, from what I see. Sorry.
Attachment #445604 - Flags: review?(neil)
Attachment #445604 - Flags: review?(neil) → review+
Pushed as http://hg.mozilla.org/comm-central/rev/8c6e4d62d98e - I'll probably file followups for more work on this file.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Summary: Clean up nsContextMenu.js and bring it more in sync with Firefox → Clean up nsContextMenu.js, lay the base forfuture work touching it
Target Milestone: --- → seamonkey2.1a2
Blocks: 566213
Blocks: 566214
Summary: Clean up nsContextMenu.js, lay the base forfuture work touching it → Clean up nsContextMenu.js, lay the base for future work touching it
Depends on: 574264
Depends on: 656886
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: