903016 - Reimplement contentAreaClick so it can be used in content and chrome

Status: NEW

(Firefox :: General, enhancement, P5)

Description

[Tom S [:evilpie]]

In Bug 897062 I duplicated a lot of code to make it work in content. We should rather have the same code for it and contentAreaClick in browser.js.

Comment 1

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Marco, can you add this bug to this iteration?

Comment 3

[Marco Mucci [:MarcoM]]

Added to IT 38.3

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I get a warning when I middle click in a remote tab:

middleMousePaste@chrome://browser/content/browser.js:15942:6
ContentClick.contentAreaClick@resource:///modules/ContentClick.jsm:41:9
ContentClick.receiveMessage@resource:///modules/ContentClick.jsm:27:9
JavaScript error: chrome://browser/content/browser.js, line 15943: TypeError: event.stopPropagation is not a function

It looks like ContentClick.contentAreaClick passes some JSON as the event to middleMousePaste. Then when we try to call stopPropagation() on the JSON, we get an error.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hey Felipe, what's the status here?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Bill, I have been prioritizing other bugs over this one because this is something that currently works, and just needs a reimplementation. Although I intend to get to it soon because this is the non-hacky way to fix bug 1111960

Comment 7

[:Felipe Gomes (needinfo for replies!)]

So, I have divided this in several patches to make it simpler to review. Not all of patches work independently, so I'll have to merge them all in the end.

Previously, there was a function in the child that looked similar to browser.js's contentAreaClick and tried to gather some data and send a message to the child with an object faking an `event`.

Now I've moved all of the actual behavior of that  function to run in the child, and it send messages with the actions that the parent should take.


The new structure works like this:

ContentClick.jsm implements now two objects, one which will be used by the parent (receiving messages and performing actions), and one will be used by the child (to handle the actual click).

ContentClickParent is the obj that runs in the child, and ContentClickChild is where the new contentAreaClick is implemented.


The patches go gradually moving the code from one place to another, and some leftover code may just be pushed down in one patch, but all is cleaned up at the end.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 0 - Move hrefAndLinkNodeForClickEvent function to ContentClick

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 1 - Basic structure, and child side of PlacesAddBookmark

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 2 - Parent side of PlacesAddBookmark

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 3 - Move whereToOpenLink to BrowserUtils

I'm not entirely sure if this is the best place to move this function. Despite its name, BrowserUtils is part of toolkit. So that leaves me wondering.. Although it looks like other similar functions are already there, so..

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 4 - middleMousePaste

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 5 - HandleLinkClick part 1: saveURL

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 6 - HandleLinkClick part 2: openLinkIn

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 7 - markPageAsFollowedLink

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 8 - remove contentAreaClick from browser.js, but make it panelAreaClick

Unfortunately, we can't totally remove the contentAreaClick from the parent at this point because it's also used for the non-remote sidebars. It's possible that the new code works reasonably well with them, but I haven't tested it, and I don't want to bring that to the scope of this bug.  So what I did was just to rename the function and make it only used for the panels, and left it untouched otherwise.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 9 - Remove left-over from content.js

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8614184 [details] [diff] [review]
Part 0 - Move hrefAndLinkNodeForClickEvent function to ContentClick

Review of attachment 8614184 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/ContentClick.jsm
@@ +20,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "BrowserUtils",
> +  "resource://gre/modules/BrowserUtils.jsm");
> +
> +
> +

Too many newlines.

@@ +93,5 @@
>  };
> +
> +
> +
> +

Same here.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8614190 [details] [diff] [review]
Part 3 - Move whereToOpenLink to BrowserUtils

Review of attachment 8614190 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/BrowserUtils.jsm
@@ +458,5 @@
> +    var middleUsesTabs = getBoolPref("browser.tabs.opentabfor.middleclick", true);
> +
> +    // Don't do anything special with right-mouse clicks.  They're probably clicks on context menu items.
> +
> +  #ifdef XP_MACOSX

You can't use the preprocessor here. Use AppConstants.platform instead.

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8614202 [details] [diff] [review]
Part 8 - remove contentAreaClick from browser.js, but make it panelAreaClick

Review of attachment 8614202 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +5408,5 @@
>   * @param isPanelClick
>   *        Whether the event comes from a web panel.
>   * @note default event is prevented if the click is handled.
>   */
> +function panelAreaClick(event, isPanelClick)

Seems like the arg is no longer needed. Since you renamed it, panels must be the only callers.

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8614188 [details] [diff] [review]
Part 1 - Basic structure, and child side of PlacesAddBookmark

Review of attachment 8614188 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +5431,5 @@
>        event.preventDefault();
>      }
>      return;
>    }
> +/////

This needs to be removed before checkin.

@@ +5480,5 @@
>        return;
>      }
>    }
>  
> +////

Same.

::: browser/modules/ContentClick.jsm
@@ +43,3 @@
>  
> +  contentAreaClick: function (event) {
> +    if (!event.isTrusted || event.defaultPrevented || event.button == 2)

I know styles differ, but the more recent frontend code I've seen always uses braces. Also, this check is already done at the top of content.js's handleEvent. Do we really need it here?

@@ +46,5 @@
> +      return;
> +
> +    let mm = event.target.ownerGlobal
> +                  .QueryInterface(Ci.nsIInterfaceRequestor)
> +                  .getInterface(Ci.nsIDocShell)

I believe you can go straight to getInterface(Ci.nsIContentFrameMessageManager) here. nsDocShell takes care of finding the right tree owner.

@@ +69,5 @@
> +    // elements, as opposed to XLink).
> +    if (linkNode && event.button == 0 &&
> +        !event.ctrlKey && !event.shiftKey && !event.altKey && !event.metaKey) {
> +
> +      this.sendAction(mm, "addBookmark", {

Don't we want to check if |if (linkNode.getAttribute("rel") == "sidebar")|?

@@ +80,5 @@
> +    }
> +
> +
> +
> +    ///////

Need to remove.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8614189 [details] [diff] [review]
Part 2 - Parent side of PlacesAddBookmark

Review of attachment 8614189 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/ContentClick.jsm
@@ +33,5 @@
> +      return;
> +    }
> +
> +    let data = message.data;
> +    let window = message.target.defaultView;

message.target will be a XUL element. Don't we want to do .ownerDocument.defaultView?

@@ +40,5 @@
> +      case "addBookmark":
> +        // This is the Opera convention for a special link that, when clicked,
> +        // allows to add a sidebar panel.  The link's title attribute contains
> +        // the title that should be used for the sidebar panel.
> +        PlacesUIUtils.showBookmarkDialog({ action: "add"

I know this was the existing style, but it seems kind of awkward. I generally prefer this:

Foo.bar({
  prop1: value1,
  prop2: value2,
  array1: [
    "a",
    "b",
  ],
});

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8614193 [details] [diff] [review]
Part 4 - middleMousePaste

Review of attachment 8614193 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +5543,5 @@
>    event.preventDefault();
>    return true;
>  }
>  
> +function middleMousePaste(event, where) {

The last line of this calls event.stopPropagation(), which won't work if ContentAreaClick.jsm passes null.

I think it would be better to split this into two functions. One of them contains the main body of middleMousePaste. It *only* takes a where param and doesn't call whereToOpenLink. It also doesn't call stopPropagation. This version would be used by ContentAreaClick. Then there would be an outer function that takes an event, computes whereToOpenLink, calls the inner function, and does stopPropagation. The rest of browser.js should use that.

I'm not sure what to do about the stopPropagation call in the content script. Can we check the clipboard in the child and call stopPropagation based on that? It's a little hacky, but I don't see any other choice.

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8614194 [details] [diff] [review]
Part 5 - HandleLinkClick part 1: saveURL

Review of attachment 8614194 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/ContentClick.jsm
@@ +113,5 @@
>        event.preventDefault();
>        return;
>      }
>  
> +    handleLinkClick(event, href, linkNode, this.saveAction.bind(mm));

I think you need to pass |this| as the first argument to bind.

@@ +228,5 @@
> +
> +  if (where == "save") {
> +    sendAction("saveURL", {
> +      href: href,
> +      fileName: linkNode ? gatherTextUnder(linkNode) : "", null,

The extra null at the end isn't doing anything.

@@ +229,5 @@
> +  if (where == "save") {
> +    sendAction("saveURL", {
> +      href: href,
> +      fileName: linkNode ? gatherTextUnder(linkNode) : "", null,
> +      referrerURI: referrerURI

This is going to generate a warning about sending XPCOM objects using the message manager. Instead please send the .spec and then use BrowserUtils.makeURI on the other side.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8614196 [details] [diff] [review]
Part 6 - HandleLinkClick part 2: openLinkIn

Review of attachment 8614196 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/ContentClick.jsm
@@ +61,5 @@
>                         true, data.referrerURI, message.objects.doc);
>          break;
>  
> +      case "openLinkIn":
> +        window.openLinkIn(data.href, data.where, data.params);

data.params isn't a thing. Probably find to just pass |data|.

@@ +249,5 @@
> +
> +  if (where == "tab" && docShell.mixedContentChannel) {
> +    const sm = Services.scriptSecurityManager;
> +    try {
> +      var targetURI = makeURI(href);

Is makeURI in scope here? Also, might as well switch to let.

@@ +251,5 @@
> +    const sm = Services.scriptSecurityManager;
> +    try {
> +      var targetURI = makeURI(href);
> +      sm.checkSameOriginURI(referrerURI, targetURI, false);
> +      persistAllowMixedContentInChildTab = true;

How odd this code was. Could we instead default this to true and set it to false in the catch clause?

@@ +261,5 @@
> +  let params = { href: href,
> +                 where: where,
> +                 charset: doc.characterSet,
> +                 allowMixedContent: persistAllowMixedContentInChildTab,
> +                 referrerURI: referrerURI,

Same thing about sending .spec here.

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8614198 [details] [diff] [review]
Part 7 - markPageAsFollowedLink

Review of attachment 8614198 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/ContentClick.jsm
@@ +126,2 @@
>      try {
>        if (!PrivateBrowsingUtils.isWindowPrivate(window))

Please switch this to isContentWindowPrivate so we don't get a warning.

@@ +126,3 @@
>      try {
>        if (!PrivateBrowsingUtils.isWindowPrivate(window))
> +        sendAction(mm, "markPageAsFollowedLink", {href: href});

As an ES6 feature, you can use {href} instead of {href: href}.

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm a little worried about test coverage here. Can you try to manually exercise all these paths?

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

a lot of add-ons use contentAreaClick (one of the reason in the past we tried to avoid changing it), some use hrefAndLinkNodeForClickEvent and some use ContentClick.jsm

Comment 29

[:Felipe Gomes (needinfo for replies!)]

Not working on it at the moment