Bill McCloskey [inactive unless it's an emergency] (:billm) Reporter
	Description • 11 years ago

Sometimes it opens a link in a new tab, and sometimes it tries to open the URI in the clipboard in the current tab. Sometimes it does both. We need to make sure both of these things happen when they're supposed to.

	Josh Matthews [:jdm]
	Comment 1 • 11 years ago

The contentAreaClick function is likely relevant here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5077

	Tom S [:evilpie] Assignee
	Comment 2 • 11 years ago

I reimplemented most of contentAreaClick based on the Content:Click message from content when we click on something. There are only two things missing, sidebars and saving links. I feel like these are two not so important features, so that we should be able to land this on central. Especially to prevent this from breaking by a merge again.

http://hg.mozilla.org/projects/larch/rev/8b7f42717898

	Tom S [:evilpie] Assignee
	Comment 3 • 11 years ago

This handles almost anything. There is some code for sidebars, but these might not even be remote.

	Dão Gottwald [:dao]
	Comment 4 • 11 years ago

Comment on attachment 783917 [details] [diff] [review]
v1

>+let Content = {

Please rename this to ClickEventHandler or something like that.

>+  contentAreaClick: function(event) {

and this to handleEvent

>+    function isHTMLLink(aNode)
>+    {

nit: put these on one line

	Tom S [:evilpie] Assignee
	Comment 5 • 11 years ago

This class does a lot more on larch, this is just the cherry picked part for this.

	Dão Gottwald [:dao]
	Comment 6 • 11 years ago

(In reply to Tom Schuster [:evilpie] from comment #5)
> This class does a lot more on larch, this is just the cherry picked part for
> this.

That code hasn't been reviewed, so this isn't really convincing. Anything in browser-child.js is about content, so a catch-all "Content" object doesn't seem useful and is likely to become a mess.

	Tom S [:evilpie] Assignee
	Comment 7 • 11 years ago

Fixed dao's feedback. Thanks.

	:Felipe Gomes (needinfo for replies!)
	Comment 8 • 11 years ago

Comment on attachment 784626 [details] [diff] [review]
v1.1

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

the parent code is going in browser/ while the child is going in toolkit/, which is not good. This is an entire front-end feature so you can move the code being added from browser-child.js to browser/base/content/content.js

There's a lot of code duplication going on here. The right approach for this kind of change is to move all of the content-touching code to a content script which can run both in e10s/non-e10s mode. Alternatively I think it would be possible to further modify the existing contentAreaUtils (and some of the functions it calls) to handle the real event and the "fake" json event case.
But I've reviewed the code as is anyway: maybe we can get away with it temporarily if this is a highly-desired feature (as IIRC Bill said)

::: browser/base/content/browser-parent.js
@@ +4,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +let BrowserParent = {
> +  init: function() {
> +

nit: empty line

@@ +45,5 @@
> +                                       }, window);
> +      return;
> +    }
> +
> +    // Todo: maybe code for sidebars?

Replace the TODOs with a single comment saying that this function is not yet complete and with a bug # to a follow-up bug to complete/reimplement it

::: browser/base/content/browser.js
@@ +761,5 @@
>      messageManager.loadFrameScript("chrome://browser/content/content.js", true);
>      messageManager.loadFrameScript("chrome://browser/content/content-sessionStore.js", true);
>  
> +    if (gMultiProcessBrowser)
> +      BrowserParent.init();

Make BrowserParent a lazily-loaded .jsm that is not even loaded for non-e10s. See an example of how Bill did it (with the message listeners) in bug 899222

::: dom/ipc/TabChild.h
@@ +124,5 @@
> +  PreHandleEvent(nsEventChainPreVisitor& aVisitor)
> +  {
> +    aVisitor.mForceContentDispatch = true;
> +    return NS_OK;
> +  }

is this necessary for the same reason from the other use cases of this flag? bug 329119? Even with the systemEventListener? If so add the FIXME message similarly.

I can't review event-handling changes so remember to flag smaug for review from the get-go.

::: toolkit/content/browser-child.js
@@ +191,5 @@
> +let ClickEventHandler = {
> +  init: function init() {
> +    Cc["@mozilla.org/eventlistenerservice;1"]
> +      .getService(Ci.nsIEventListenerService)
> +      .addSystemEventListener(global, "click", this, true);

could you use `content` instead of `global` here?

@@ +236,5 @@
> + * @return [href, linkNode].
> + *
> + * @note linkNode will be null if the click wasn't on an anchor
> + *       element (or XLink).
> + */

wrong indentation in this comment

	Tom S [:evilpie] Assignee
	Comment 9 • 11 years ago

(In reply to :Felipe Gomes from comment #8)
> Comment on attachment 784626 [details] [diff] [review]
> v1.1
> 
> Review of attachment 784626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> the parent code is going in browser/ while the child is going in toolkit/,
> which is not good. This is an entire front-end feature so you can move the
> code being added from browser-child.js to browser/base/content/content.js
> 
> There's a lot of code duplication going on here. The right approach for this
> kind of change is to move all of the content-touching code to a content
> script which can run both in e10s/non-e10s mode. Alternatively I think it
> would be possible to further modify the existing contentAreaUtils (and some
> of the functions it calls) to handle the real event and the "fake" json
> event case.
> But I've reviewed the code as is anyway: maybe we can get away with it
> temporarily if this is a highly-desired feature (as IIRC Bill said)
> 
This is actually not so easy to do cleanly. Because of linkNode and the panel code.
> ::: browser/base/content/browser-parent.js
> @@ +4,5 @@
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +let BrowserParent = {
> > +  init: function() {
> > +
> 
> nit: empty line
> 
> @@ +45,5 @@
> > +                                       }, window);
> > +      return;
> > +    }
> > +
> > +    // Todo: maybe code for sidebars?
> 
> Replace the TODOs with a single comment saying that this function is not yet
> complete and with a bug # to a follow-up bug to complete/reimplement it
> 
The sidebar code actually only needs to be in chrome, so I added a comment about that.
> ::: browser/base/content/browser.js
> @@ +761,5 @@
> >      messageManager.loadFrameScript("chrome://browser/content/content.js", true);
> >      messageManager.loadFrameScript("chrome://browser/content/content-sessionStore.js", true);
> >  
> > +    if (gMultiProcessBrowser)
> > +      BrowserParent.init();
> 
> Make BrowserParent a lazily-loaded .jsm that is not even loaded for
> non-e10s. See an example of how Bill did it (with the message listeners) in
> bug 899222
> 
Can you link me to the line? It looks like AboutHome is still always initialized.
> ::: dom/ipc/TabChild.h
> @@ +124,5 @@
> > +  PreHandleEvent(nsEventChainPreVisitor& aVisitor)
> > +  {
> > +    aVisitor.mForceContentDispatch = true;
> > +    return NS_OK;
> > +  }
> 
> is this necessary for the same reason from the other use cases of this flag?
> bug 329119? Even with the systemEventListener? If so add the FIXME message
> similarly.
> 
> I can't review event-handling changes so remember to flag smaug for review
> from the get-go.
> 
He gave me r+ via irc.
> ::: toolkit/content/browser-child.js
> @@ +191,5 @@
> > +let ClickEventHandler = {
> > +  init: function init() {
> > +    Cc["@mozilla.org/eventlistenerservice;1"]
> > +      .getService(Ci.nsIEventListenerService)
> > +      .addSystemEventListener(global, "click", this, true);
> 
> could you use `content` instead of `global` here?
> 
No. We also use addEventListener in other places and this is basically the same thing here.
> @@ +236,5 @@
> > + * @return [href, linkNode].
> > + *
> > + * @note linkNode will be null if the click wasn't on an anchor
> > + *       element (or XLink).
> > + */
> 
> wrong indentation in this comment
I am not seeing it. One more space?

	:Felipe Gomes (needinfo for replies!)
	Comment 10 • 11 years ago

(In reply to Tom Schuster [:evilpie] from comment #9)
> > 
> > Make BrowserParent a lazily-loaded .jsm that is not even loaded for
> > non-e10s. See an example of how Bill did it (with the message listeners) in
> > bug 899222
> > 
> Can you link me to the line? It looks like AboutHome is still always
> initialized.

AboutHome is always initialized because the content script is handling both e10s/non-e10s cases. But I meant it as an example of how the message listeners are registered only once and the message is used to find out which browser/window it came from.

> > could you use `content` instead of `global` here?
> > 
> No. We also use addEventListener in other places and this is basically the
> same thing here.

ok

> > 
> > wrong indentation in this comment
> I am not seeing it. One more space?

two more, as the function declaration starts at 1 indent (two spaces) and the comment should be the same

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 11 • 11 years ago

Can you put the duplication/hackiness behind an MAKE_E10S_WORK ifdef?

	Tom S [:evilpie] Assignee
	Comment 12 • 11 years ago

It's already in it's own jsm now.

	:Felipe Gomes (needinfo for replies!)
	Comment 14 • 11 years ago

Comment on attachment 787709 [details] [diff] [review]
v2

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

::: browser/base/content/browser.js
@@ +732,5 @@
>  
>      var mustLoadSidebar = false;
>  
> +    if (!gMultiProcessBrowser) {
> +      // There is an Content:Click message manually sent from content.

s/an/a/

@@ +757,5 @@
>      messageManager.loadFrameScript("chrome://browser/content/content.js", true);
>      messageManager.loadFrameScript("chrome://browser/content/content-sessionStore.js", true);
>  
> +    if (gMultiProcessBrowser)
> +      BrowserParent.init();

left-over from previous patch, needs to be removed

::: browser/modules/moz.build
@@ +26,5 @@
>      ]
>  
>  EXTRA_PP_JS_MODULES += [
>      'AboutHome.jsm',
> +    'ContentClick.jsm',

you don't need this in the PP section as there's no preprocessor code in the module. Add it to EXTRA_JS_MODULES instead

	Tom S [:evilpie] Assignee
	Comment 15 • 11 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/57b34c38a191

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/57b34c38a191