Closed
Bug 562339
Opened 15 years ago
Closed 15 years ago
Clean up nsContextMenu.js, lay the base for future work touching it
Categories
(SeaMonkey :: UI Design, defect)
SeaMonkey
UI Design
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.1a2
People
(Reporter: kairo, Assigned: kairo)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files, 3 obsolete files)
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
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.
|
||
Updated•15 years ago
|
Version: unspecified → Trunk
|
Assignee | |
Comment 1•15 years ago
|
||
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.
|
|
Assignee | |
Comment 2•15 years ago
|
||
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)
|
|
Assignee | |
Comment 3•15 years ago
|
||
Here's the |diff -w| that reduces the changes to |3 files changed, 291 insertions(+), 269 deletions(-)| which should be better reviewable.
|
|
Assignee | |
Comment 4•15 years ago
|
||
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•15 years ago
|
||
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?]
|
|
Assignee | |
Comment 6•15 years ago
|
||
(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•15 years ago
|
||
(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?
|
|
Assignee | |
Comment 8•15 years ago
|
||
(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•15 years ago
|
||
(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•15 years ago
|
||
(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.
|
|
Assignee | |
Comment 11•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
(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;
});
|
|
Assignee | |
Comment 14•15 years ago
|
||
(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).
|
|
Assignee | |
Comment 15•15 years ago
|
||
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)
|
|
Assignee | |
Comment 16•15 years ago
|
||
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.
|
|
Assignee | |
Comment 17•15 years ago
|
||
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 18•15 years ago
|
||
Comment on attachment 445216 [details] [diff] [review]
v1.2: make ellipsis a lazy getter
This isn't bug 525869...
Attachment #445216 -
Flags: review?(neil)
|
|
Assignee | |
Comment 19•15 years ago
|
||
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
|
|
Assignee | |
Comment 20•15 years ago
|
||
This is the actually right patch, from what I see. Sorry.
Attachment #445604 -
Flags: review?(neil)
|
|
||
Updated•15 years ago
|
Attachment #445604 -
Flags: review?(neil) → review+
|
|
Assignee | |
Comment 21•15 years ago
|
||
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: 15 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
|
|
||
Updated•15 years ago
|
Summary: Clean up nsContextMenu.js, lay the base forfuture work touching it → Clean up nsContextMenu.js, lay the base for future work touching it
You need to log in
before you can comment on or make changes to this bug.
Description
•