397457 - Clean up nsContextMenu.js

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Phil Ringnalda (:philor)]

Currently, we do a ton of work every time the context menu is shown for a click in a message, checking things like whether the click was on MathML, or whether there's a background image, for options that we don't have menuitems for, so no matter what the result of (this.onMathML && !this.isTextSelected) is, showItem() will fail to find an element with the id "context-viewpartialsource-mathml" and will do nothing.

Two possible approaches: tear out the things we're sure we won't ever want to have, leaving things like web search and undo/redo/paste for text inputs that we really ought to do, or, the scorched earth approach, take out everything that isn't currently being used, and let anyone who adds something steal a fresh copy of the code from the various other nsContextMenu.jses. I'm strongly inclined toward the latter, since as bug 397443 (which made me notice this) illustrates, people making tree-wide changes to this code won't update our copy, so we're better off having as little as possible to check, and stealing it as freshly as possible.

Comment 1

[Phil Ringnalda (:philor)]

It'd help if I filed when I first start thinking about things, so I don't forget stuff along the way: I can't tear out all the .onThisOrThat, since those are an API I've used myself in extensions. Helper functions to do things that don't work, like nsContextMenu.viewBGImage() can go along with the no-op showItem() calls, but nsContextMenu.hasBGImage really needs to stay and provide the right answer.

Comment 2

[Phil Ringnalda (:philor)]

Attachment: Fix v.1

Okay, this is either ready for review, or staring at it so long has given me Stockholm Syndrome, and I just think it is.

In general:

* Since I wound up touching so much anyway, I cleaned up the "whitespace according to rules not used in any other file in the tree" style - I'll attach a -w diff, too, but really it's only readable by applying the patch and reading the file. Sorry.

* Removed a bunch of comments of the form

// Set a to 1 plus 1
a = 1 + 1;

* Got rid of anonymous functions

* Removed methods that we don't implement, haven't maintained, and aren't likely to use - an extension that wants to implement them is better off with its own two or three lines of code.

* Removed calls to showItem() for items we never show because we don't have that menuitem to show

Specifically:

* Picked up the core of bug 303181 (use a .linkURL property fixed to what you think you're dealing with, rather than a .linkURL() method that might get different things at different times, and never, ever grovel around with string manipulation of a URL), plus some style improvements and optimizations from it

* Ask checkLoadURIWithPrincipal whether or not a link is saveable - we weren't, SeaMonkey doesn't, but Firefox does, and when I asked Jesse and bz, they both said it's the right thing to do. Firefox asks at the point of saving, so that you have an enabled menuitem which does nothing, and you have to grope in the error console for the load denied message to see why. I put the check in isLinkSaveable instead, so that instead of offering non-working UI with a hidden explanation of why, we don't even offer to save links to chrome:// or about:plugins, which strikes me as less generally confusing.

In contentAreaClick.js:

We are currently throwing a "load denied" error every time you left- or right-click a link, because we have a urlSecurityCheck() call that's using the old arguments (href, sourceURL) rather than the current (href, sourcePrincipal). Luckily (I guess), it was ineffective, right or wrong, since otherwise we wouldn't be getting a context menu on links that can't be loaded from mailbox://. The inaccurate comment about "except when left-clicking on links (special case)" probably meant that if you are a browser which opens links with middle-clicks, you have to security check those, while docShell will handle the check for left-clicks. Since we don't do anything with middle-click, we don't need to, we only need to pass left-clicks through the phishing detector.

In mailContextMenus.js:

Just the API change that linkURL is a property, rather than a method (which will probably break some extensions, it broke two I wrote when Firefox made the same change, but at least we're doing it much further before release).

Comment 3

[Phil Ringnalda (:philor)]

Attachment: Fix v.1 (-w)

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 283311 [details] [diff] [review]
Fix v.1

>+      elem = elem.parentNode;    

Trailing spaces.

>+  //selected text.   Only use the first 15 chars.

Some extra spaces...
>+  // Determines whether or not the separator with the specified ID should be 

Trailing space

>+    return false;  

Trailing spaces.

>+  /** 
>+   * extract the href from the link click event. 

Trailing spaces, and capitalize.

>+      if (target.hasAttribute("href")) 

Trailing space.

>+        href = target.form.action;      

Trailing spaces.

>+      // we may be nested inside of a link node
>+      var linkNode = event.originalTarget;
>+      while (linkNode && !(linkNode instanceof HTMLAnchorElement))
>+        linkNode = linkNode.parentNode;
>+      

Trailing spaces.

>+    return aElem.ownerDocument.defaultView.getComputedStyle(aElem, "")
>+               .getPropertyValue(aProp);

Align, one space more.

>+  function contentAreaClick(event) 

Trailing space.

>+      return gPhishingDetector.warnOnSuspiciousLinkClick(href); // let the phishing detector check the link

Long line.

>+      if (target instanceof Ci.nsIImageLoadingContent) 

Trailing space.


>+    }
>+    
>+    return true;

Trailing space.

>+  function makeURLAbsolute( base, url ) 

Trailing. And might as well remove the extra spaces, as other functions don't have it here.

Generally, you could use /** */ style comments for function documentation. And in a few places add dot to the end of comments that are full sentences.

Sure, a lot of this I assume is just getting moved around, but since your touching it... :)

I found two issues:
1) There is something fishy with the Save Target As. I don't seem to get that when selecting a link in a html message.

2) Clicking Report Email Scam I get 
Error: gContextMenu.linkURL is not a function
Source File: chrome://messenger/content/messenger.xul
Line: 1

Besides those it seems to be working well.

Comment 5

[Phil Ringnalda (:philor)]

Comment on attachment 283311 [details] [diff] [review]
Fix v.1

*That* was what was left to do - jst-review! Thanks, Magnus.

Comment 6

[Phil Ringnalda (:philor)]

(In reply to comment #4)
> >+  function makeURLAbsolute( base, url ) 
> 
> Trailing. And might as well remove the extra spaces, as other functions don't
> have it here.

Note to my confused self: that's the copy in contentAreaClick.js, and as long as you're taking blame on the whole file to get rid of the ^M's, the function above has spaces around the argument list too, and they should all change to aArgument style, like you did in nsContextMenu.js.

Comment 7

[Scott MacGregor]

Comment on attachment 283313 [details] [diff] [review]
Fix v.1 (-w)

is the patch holding you hostage or are you holding the patch hostage?

Comment 8

[Phil Ringnalda (:philor)]

Attachment: Test mbox

(In reply to comment #7)
> is the patch holding you hostage or are you holding the patch hostage?

I think at this point, it's me. The problem with save-as was a late and stupid change to checkloaduri, made less obvious because we don't use Cc/Ci, except locally when someone has ported something from Fx, and most of my jst-review dings were also for XPCOM long lines, including the long line of the "wtf is that doing here?" unused-locally global definition of prefs at the top of contentAreaClick.js which seems to then be used all over. So, stick Cc/Ci somewhere global like mailCore.js, change everything I'm touching, get the rest later? Nope, apparently mailCore.js coming in from an XUL overlay gets it there too late. Rewrite and reorganize the inclusion of nearly all of mail/base/ and mail/components/? That went surprisingly poorly, too :)

So, an mbox with a single message that tests the things I don't want to break, and shortly, a patch that fixes just enough, and only gets a few tolerable jst-review dings.

Comment 9

[Phil Ringnalda (:philor)]

Attachment: Fix v.2f

Okay, included mailWindowOverlay.xul in the patch, not just my tree, so reportPhishingURL should work, fixed the missing |const nsIScriptSecurityManager| so that save-as works (except on prohibited links, where you get an unfortunate but tolerable "may not load or link to" message in the console for any right-click, because my patch to CAPS to have a quiet version is another big load of "lightly touching 22 files" code), I really, really hope I've finally killed all the trailing space, and I only put back a little of the "Set a to 1 plus 1" commenting, I hope (while noticing some wrongness in the search stuff, so it wasn't all bad).

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 283970 [details] [diff] [review]
Fix v.2f

Lookin good! r=mkmelin+mozilla

Comment 11

[Phil Ringnalda (:philor)]

mail/base/content/nsContextMenu.js 1.19
mail/base/content/contentAreaClick.js 1.18
mail/base/content/mailContextMenus.js 1.29
mail/base/content/mailWindowOverlay.xul 1.225
mail/base/content/mailWindow.js 1.60

Whew, now I'm done with it, until... I realize, as I did while going to sleep last night, that the whole reason I was in this file at all was to remove the dependency on strres.js, which I then forgot to do :)