Last Comment Bug 562339 - Clean up nsContextMenu.js, lay the base for future work touching it
: Clean up nsContextMenu.js, lay the base for future work touching it
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a2
Assigned To: Robert Kaiser (not working on stability any more)
:
Mentors:
Depends on: 574264 656886
Blocks: 566213 566214 SMPlacesBMarks
  Show dependency treegraph
 
Reported: 2010-04-28 07:46 PDT by Robert Kaiser (not working on stability any more)
Modified: 2011-05-13 05:17 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
actual patch, mostly whitespace cleanup (110.53 KB, patch)
2010-04-28 11:55 PDT, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
diff -w for easier review (66.12 KB, patch)
2010-04-28 11:57 PDT, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v1.1: minor fixes to the patch (110.50 KB, patch)
2010-05-13 15:14 PDT, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v1.2: make ellipsis a lazy getter (40.88 KB, patch)
2010-05-13 15:36 PDT, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v1.2: make ellipsis a lazy getter (110.59 KB, patch)
2010-05-16 08:12 PDT, Robert Kaiser (not working on stability any more)
neil: review+
Details | Diff | Review

Description Robert Kaiser (not working on stability any more) 2010-04-28 07:46:48 PDT
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.
Comment 1 Robert Kaiser (not working on stability any more) 2010-04-28 11:39:41 PDT
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.
Comment 2 Robert Kaiser (not working on stability any more) 2010-04-28 11:55:53 PDT
Created attachment 442133 [details] [diff] [review]
actual patch, mostly whitespace cleanup

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.
Comment 3 Robert Kaiser (not working on stability any more) 2010-04-28 11:57:27 PDT
Created attachment 442134 [details] [diff] [review]
diff -w for easier review

Here's the |diff -w| that reduces the changes to |3 files changed, 291 insertions(+), 269 deletions(-)| which should be better reviewable.
Comment 4 Robert Kaiser (not working on stability any more) 2010-04-28 11:59:14 PDT
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 5 neil@parkwaycc.co.uk 2010-04-28 13:24:26 PDT
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?]
Comment 6 Robert Kaiser (not working on stability any more) 2010-04-28 14:33:32 PDT
(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?
Comment 7 neil@parkwaycc.co.uk 2010-04-28 16:32:06 PDT
(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?
Comment 8 Robert Kaiser (not working on stability any more) 2010-04-28 17:19:30 PDT
(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.
Comment 9 neil@parkwaycc.co.uk 2010-04-30 15:59:23 PDT
(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...
Comment 10 neil@parkwaycc.co.uk 2010-04-30 16:05:59 PDT
(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.
Comment 11 Robert Kaiser (not working on stability any more) 2010-05-03 06:18:17 PDT
(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 12 Ian Neal 2010-05-04 14:57:06 PDT
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 = ...
Comment 13 neil@parkwaycc.co.uk 2010-05-12 09:28:17 PDT
(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;
});
Comment 14 Robert Kaiser (not working on stability any more) 2010-05-13 15:07:01 PDT
(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).
Comment 15 Robert Kaiser (not working on stability any more) 2010-05-13 15:14:12 PDT
Created attachment 445210 [details] [diff] [review]
v1.1: minor fixes to the patch

Here's an updated patch with the var Ian mentioned and two or three other small fixes.
Comment 16 Robert Kaiser (not working on stability any more) 2010-05-13 15:16:24 PDT
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.
Comment 17 Robert Kaiser (not working on stability any more) 2010-05-13 15:36:34 PDT
Created attachment 445216 [details] [diff] [review]
v1.2: make ellipsis a lazy getter

OK, as the import should be cheap, here's the patch with the lazy getter.
Comment 18 neil@parkwaycc.co.uk 2010-05-16 07:33:31 PDT
Comment on attachment 445216 [details] [diff] [review]
v1.2: make ellipsis a lazy getter

This isn't bug 525869...
Comment 19 Robert Kaiser (not working on stability any more) 2010-05-16 08:09:47 PDT
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.
Comment 20 Robert Kaiser (not working on stability any more) 2010-05-16 08:12:25 PDT
Created attachment 445604 [details] [diff] [review]
v1.2: make ellipsis a lazy getter

This is the actually right patch, from what I see. Sorry.
Comment 21 Robert Kaiser (not working on stability any more) 2010-05-16 13:24:23 PDT
Pushed as http://hg.mozilla.org/comm-central/rev/8c6e4d62d98e - I'll probably file followups for more work on this file.

Note You need to log in before you can comment on or make changes to this bug.