568092 - e10s: Add a messsage API for DOMTitleChanged/DOMLinkAdded/...

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip v0.1

This patch depends on bug 566586 and bug 566640 to be applied.
I would like to have feedback to be sure i'm going into the right direction.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 447387 [details] [diff] [review]
wip v0.1

>diff -r 98c9bea94e41 chrome/content/bindings/browser.js

>+    addEventListener("MozApplicationManifest", this, false);

Looks like this is really in frame-script.js and not used in browser.js
Probably the right thing to do for now.

>       case "DOMLinkAdded":
>-        let link = aEvent.originalTarget;
>-        let ownerDoc = link.ownerDocument;
>-        if (!link.href || !ownerDoc)
>+        let target = aEvent.originalTarget;
>+        let isRootDocument = (target.ownerDocument == document);
>+        if (!isRootDocument || !target.href || target.disabled)
>           return;

I am thinking we should always send this and then use the "location" JSON to filter on the chrome side.

>+      case "DOMWindowClose":
>+        if (!aEvent.isTrusted)
>+          return;
>+
>+        sendAsyncMessage(aEvent.type, { });
>+        break;

We need a way to allow DOMWindowClose to be canceled, I think

>diff -r 98c9bea94e41 chrome/content/bindings/browser.xml
>+            case "DOMContentLoaded":
>               let data = aMessage.json;
>               if (this.mIconURL == "") {
>                 let documentURI = gIOService.newURI(data.location, null, null);

Not part of this patch, but maybe we should make a <browser> property for "documentURI" similar to "currentURI"

>+            case "DOMLinkAdded":
>               let link = aMessage.json;
>-              if (!link.isRootDocument)
>-                return;

Add a check here for the top document by comparing | link.location == documentURI.spec |

>diff -r 98c9bea94e41 chrome/content/browser-ui.js
>+  _domWindowClose: function(aBrowser) {
>     // Find the relevant tab, and close it.
>     let browsers = Browser.browsers;
>     for (let i = 0; i < browsers.length; i++) {
>-      if (browsers[i] == aEvent.target) {
>+      if (browsers[i] == aBrowser) {
>         Browser.closeTab(Browser.getTabAtIndex(i));
>         aEvent.preventDefault();

We need to handle the "aEvent.preventDefault();"

>     // Push the panel initialization out of the startup path
>     // (Using an event because we have no good way to delay-init [Bug 535366])
>+    // XXX e10s - should we use DOMContentLoaded
>+    let browsers = document.getElementById("browsers");
>     browsers.addEventListener("load", function() {

DOMContentLoaded should be OK for now, but we should consider also sending messages for "load", "unload" and maybe "pageshow" and "pagehide"

>   receiveMessage: function(aMessage) {
>     switch(aMessage.name) {

Add | let browser = aMessage.target; | and use it locally ?

>+      case "DOMTitleChanged":
>+        this._titleChanged(aMessage.target);
>+        break;
>+      case "DOMWillOpenModalDialog":
>+        this._domWillOpenModalDialog(aMessage.target);
>+        break;
>+      case "DOMWindowClose":
>+        this._domWindowClose(aMessage.target);
>+        break;
>       case "DOMLinkAdded":
>         this._updateIcon(Browser.selectedBrowser.mIconURL);

Not part of your patch, I know, but I think this line is wrong. "DOMLinkAdded" message can be sent from background tabs too. I think the line should be:

          if (Browser.selectedBrowser == aMessage.target)
            this._updateIcon(Browser.selectedBrowser.mIconURL);


Overall, very good patch (feedback+), but fix the parts that are problems.

One thing that worries me is the way we pass documentURIObject.spec back over the JSON as "location". I think we could confuse developers. What does "location" mean? Is it always the documentURIObject or is it currentURI sometimes too? We should make sure that we are consistent. If we need to, we should use "currentURI" and "documentURI" in the JSON names. Thoughts?

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #1)
> (From update of attachment 447387 [details] [diff] [review])
> >diff -r 98c9bea94e41 chrome/content/bindings/browser.js
> >+        let target = aEvent.originalTarget;
> >+        let isRootDocument = (target.ownerDocument == document);
> >+        if (!isRootDocument || !target.href || target.disabled)
> >           return;
> 
> I am thinking we should always send this and then use the "location" JSON to
> filter on the chrome side.

> >+            case "DOMLinkAdded":
> >               let link = aMessage.json;
> >-              if (!link.isRootDocument)
> >-                return;
> 
> Add a check here for the top document by comparing | link.location ==
> documentURI.spec |

> >diff -r 98c9bea94e41 chrome/content/bindings/browser.xml
> >+            case "DOMContentLoaded":
> >               let data = aMessage.json;
> >               if (this.mIconURL == "") {
> >                 let documentURI = gIOService.newURI(data.location, null, null);
> 
> Not part of this patch, but maybe we should make a <browser> property for
> "documentURI" similar to "currentURI"


> One thing that worries me is the way we pass documentURIObject.spec back over
> the JSON as "location". I think we could confuse developers. What does
> "location" mean? Is it always the documentURIObject or is it currentURI
> sometimes too? We should make sure that we are consistent. If we need to, we
> should use "currentURI" and "documentURI" in the JSON names. Thoughts?

I still haven't made up my mind on what's the right decision for the bubbling of the nested iframes events, I don't see any utility for them, but because this is probably more flexible for extension's developers I won't fight for _not_ doing it.

I afraid of using "URI" in the name because I think extensions developers will expect a nsIURI in this case.  But I'm a bit unuseful here because I can't think of a better name :s

> 
> >+      case "DOMWindowClose":
> >+        if (!aEvent.isTrusted)
> >+          return;
> >+
> >+        sendAsyncMessage(aEvent.type, { });
> >+        break;
> 
> We need a way to allow DOMWindowClose to be canceled, I think
> 
 
> >diff -r 98c9bea94e41 chrome/content/browser-ui.js
> >+  _domWindowClose: function(aBrowser) {
> >     // Find the relevant tab, and close it.
> >     let browsers = Browser.browsers;
> >     for (let i = 0; i < browsers.length; i++) {
> >-      if (browsers[i] == aEvent.target) {
> >+      if (browsers[i] == aBrowser) {
> >         Browser.closeTab(Browser.getTabAtIndex(i));
> >         aEvent.preventDefault();
> 
> We need to handle the "aEvent.preventDefault();"

What do you think of sending back a message to the content-process in this case?
But if we really want to preventDefault() the event we should probably use _sync_ message both from the content->chrome that from the chrome->content context.

Comment 3

[Benjamin Stover (:stechz)]

Comment on attachment 447387 [details] [diff] [review]
wip v0.1

Discussed with Vivien and Mark in IRC.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.1

I've move the filter for sub-documents on the chrome side for now (location == documentURI) but i can file a followup to use DOMWindowCreated event and the outerWindowID as suggested on IRC, once bug 567357 has landed)

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 448348 [details] [diff] [review]
Patch v0.1

I removing review because I'm working on adding the DOMPopupBlocked/pageshow/pagehide messages.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Updated with DOMPopupBlocked, pageshow, pagehide

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 448515 [details] [diff] [review]
Patch v0.2

>diff -r 98c9bea94e41 chrome/content/bindings/browser.js

>     let json = {
>       isRootWindow: aWebProgress.DOMWindow == aWebProgress.DOMWindow.parent,

Replace with windowID when ready, right?

>   onProgressChange: function onProgressChange(aWebProgress, aRequest, aCurSelf, aMaxSelf, 

>       isRootWindow: aWebProgress.DOMWindow == aWebProgress.DOMWindow.parent,

Replace with windowID when ready, right?

>   onLocationChange: function onLocationChange(aWebProgress, aRequest, aLocationURI) {

>       isRootWindow: aWebProgress.DOMWindow == aWebProgress.DOMWindow.parent,
>+      documentURI: aWebProgress.DOMWindow.document.documentURIObject.spec,

These two should be replaced with windowID when ready, right?

>+      case "pagehide": {

>+        let util = aEvent.target.defaultView.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                                            .getInterface(Components.interfaces.nsIDOMWindowUtils);

You can use Cc and Ci, right?

>+      case "DOMPopupBlocked": {

>+        let util = aEvent.requestingWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                                          .getInterface(Components.interfaces.nsIDOMWindowUtils);

Same

>diff -r 98c9bea94e41 chrome/content/bindings/browser.xml

>+      <field name="_documentURI">null</field>
>+      <property name="documentURI"
>+                onget="return gIOService.newURI(this._documentURI, null, null)"
>+                readonly="true"/>

>+            case "DOMContentLoaded":
>               let data = aMessage.json;
>               if (this.mIconURL == "") {
>-                let documentURI = gIOService.newURI(data.location, null, null);
>+                let documentURI = gIOService.newURI(this._documentURI, null, null);
>                 this.mIconURL = documentURI.prePath + "/favicon.ico";

Use this.documentURI? Now that it's a property.

>+            case "DOMLinkAdded":
>               let link = aMessage.json;
>-              if (!link.isRootDocument)
>+              if (link.location != this._documentURI)

Use windowID, when DOMWindowCreated lands! File a bug please

>           receiveMessage: function(aMessage) {
>             let args;
>+            let message = aMessage.json;

All of the aMessage.json -> message changes make me sad. If you want to shorten the coee please use aMessage.json -> json

>       <method name="onPageShow">

>-                if (this.pageReport[i].requestingWindow &&
>-                    (this.pageReport[i].requestingWindow.document ==
>-                     this.pageReport[i].requestingDocument))
>+                if (this.pageReport[i].requestingWindowId == json.windowId)
>                   i++;
>                 else
>                   this.pageReport.splice(i, 1);

Do these changes require any fixes in the application? (places where page reports are examined)

I don't think so, but I am wrong a lot.

>       <method name="onPageHide">

>+            // XXX e10s contentDocument is not accessible, we need to use
>+            // outerWindowID
>+            if (this.feeds && aMessage.target == this.contentDocument)
>               this.feeds = null;

We should file a new bug for this

>       <method name="onPopupBlocked">

>             if (!this.pageReport) {
>-              this.pageReport = new Array();
>+              try {
>+                dump(this + "\n");
>+                this.pageReport = [];
>+              }
>+              catch(e) {
>+                dump(e + "\n");
>+              }

Remove the dumps?

>+            let json = aMessage.json;
>+            var obj = {
>+              requestingWindowId: json.windowId,
>+              popupWindowURI: gIOService.newURI(json.popupWindowURI.spec, json.popupWindowURI.charset, null),
>+              popupWindowFeatures: json.popupWindowFeatures,
>+              popupWindowName: json.popupWindowName
>+            };

Hmm, what changes do we need in the application do we need to make?

>diff -r 98c9bea94e41 chrome/content/browser-ui.js

>+  _domWillOpenModalDialog: function(aBrowser) {
>     // We're about to open a modal dialog, make sure the opening
>     // tab is brought to the front.
> 
>-    let browser = e.target;
>     for (let i = 0; i < Browser.tabs.length; i++) {
>-      if (Browser._tabs[i].browser == browser) {
>+      if (Browser._tabs[i].browser == aBrowser) {
>         Browser.selectedTab = Browser.tabs[i];
>         break;
>       }
>     }
>+    return { };

Is this required? Is there anything we can do to make it optional for the default case?

>+  _domWindowClose: function(aBrowser) {
>     // Find the relevant tab, and close it.
>     let browsers = Browser.browsers;
>     for (let i = 0; i < browsers.length; i++) {
>-      if (browsers[i] == aEvent.target) {
>+      if (browsers[i] == aBrowser) {
>         Browser.closeTab(Browser.getTabAtIndex(i));
>-        aEvent.preventDefault();
>+        return { preventDefault: true };
>         break;
>       }
>     }
>+
>+    return { };

Same

>     // Push the panel initialization out of the startup path
>     // (Using an event because we have no good way to delay-init [Bug 535366])
>+    let browsers = document.getElementById("browsers");
>     browsers.addEventListener("load", function() {

We need to change this don't we? To am "load" or "DOMContentLoaded" message?

Sorry for the long review. Most are questions about windowID, which we should file a new bug.

Should be an r+ next time, after the questions get answered.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #7)> 
> Replace with windowID when ready, right?

Yep. I need to investigate a bit more the behavior of outerWindowID though, it looks strange to me that: to popuptest.com (window id=0) -> go anywhere else -> go back in history (window id=0 again)

I will look at that in a followup for the various cases where we need to use WindowID.
 
> 
> >       <method name="onPageShow">
> 
> >-                if (this.pageReport[i].requestingWindow &&
> >-                    (this.pageReport[i].requestingWindow.document ==
> >-                     this.pageReport[i].requestingDocument))
> >+                if (this.pageReport[i].requestingWindowId == json.windowId)
> >                   i++;
> >                 else
> >                   this.pageReport.splice(i, 1);
> 
> Do these changes require any fixes in the application? (places where page
> reports are examined)
> 
> I don't think so, but I am wrong a lot.

The popupblocker works with these changes but I'll look deeper just in case

> 
> >       <method name="onPageHide">
> 
> >+            // XXX e10s contentDocument is not accessible, we need to use
> >+            // outerWindowID
> >+            if (this.feeds && aMessage.target == this.contentDocument)
> >               this.feeds = null;
> 
> We should file a new bug for this

Will do.


> >diff -r 98c9bea94e41 chrome/content/browser-ui.js
> 
> >+  _domWillOpenModalDialog: function(aBrowser) {
> >     // We're about to open a modal dialog, make sure the opening
> >     // tab is brought to the front.
> > 
> >-    let browser = e.target;
> >     for (let i = 0; i < Browser.tabs.length; i++) {
> >-      if (Browser._tabs[i].browser == browser) {
> >+      if (Browser._tabs[i].browser == aBrowser) {
> >         Browser.selectedTab = Browser.tabs[i];
> >         break;
> >       }
> >     }
> >+    return { };
> 
> Is this required? Is there anything we can do to make it optional for the
> default case?

Agreed that's the reason of bug 568818, which is already r+ed! :)
But smaug told me he wants to do some tests and I don't want to wait for it to land.

> 
> >     // Push the panel initialization out of the startup path
> >     // (Using an event because we have no good way to delay-init [Bug 535366])
> >+    let browsers = document.getElementById("browsers");
> >     browsers.addEventListener("load", function() {
> 
> We need to change this don't we? To am "load" or "DOMContentLoaded" message?

I've removed my comment but i've forgot to change the code (sigh). "DOMContentLoaded" sounds fine to me.

Also, I want to handle "contextmenu" in this bug.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

> 
> >       <method name="onPageShow">
> 
> >-                if (this.pageReport[i].requestingWindow &&
> >-                    (this.pageReport[i].requestingWindow.document ==
> >-                     this.pageReport[i].requestingDocument))
> >+                if (this.pageReport[i].requestingWindowId == json.windowId)
> >                   i++;
> >                 else
> >                   this.pageReport.splice(i, 1);
> 
> Do these changes require any fixes in the application? (places where page
> reports are examined)
> 
> I don't think so, but I am wrong a lot.

I've add a comment in the source code for that. It affects 2 lines of firefox's browser.js.


Also I'll open a followup for the various windowID case once this bug will land.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 448975 [details] [diff] [review]
Patch v0.2

Looks good

Comment 11

[Egor Starkov]

Attachment: DOMLinkAdded message

I'm testing Yummy extension and want to make it work on Electrolysis Fennec. I tried attached patches and didn't have any success with getting DOMLinkAdded message in extension. So I tried my own way and finally got message. Nicolas, could you please take a look at this patch?

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 449616 [details] [diff] [review]
DOMLinkAdded message

We do not want to use this method. We do not want to create "fake" DOM events. The event is not the same as one sent for real.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Egor - It sounds like you have code that expects DOMLinkAdded to be sent as a re DOM event and not a new e10s message. You will need to change your code to expect a message, not an event.

Comment 14

[Egor Starkov]

(In reply to comment #13)
> Egor - It sounds like you have code that expects DOMLinkAdded to be sent as a
> re DOM event and not a new e10s message. You will need to change your code to
> expect a message, not an event.

Yes, Yummy extension expects for DOMLinkAdded event and this mechanism works fine with non-electrolysis Fennec. I don't think that modifying extension code to catch messages instead of events and make it electrolysis dependent. Or did you mean something else?

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #14)

> Yes, Yummy extension expects for DOMLinkAdded event and this mechanism works
> fine with non-electrolysis Fennec. I don't think that modifying extension code
> to catch messages instead of events and make it electrolysis dependent. Or did
> you mean something else?

No, that is exactly what I meant. Add-ons have to adapt to e10s, not the other way around. There are a few ways you could do it, but for now I suggest that you add code to Yummy that does both: listen for DOMLinkAdded as an event and listen for DOMLinkAdded as a message.

You have more choices too.

You can refactor the code into 2 new files: 1 that hooks up and listens for the DOMLinkAdded event and a second file the hooks up and listens for DOMLinkAdded as a message. Then you use chrome.manifest application version flags to pick the right JS file. This is my preferred method.

A quick and dirty method would be to keep the old and new code together in the existing file but use nsIXULAppInfo.version [1] to choose the right code. Remember, nsIXULAppInfo.version is a string and can have "a" and "b" in it. You need to use the version comparator [2] to make comparisons. I consider this method a hack and it will make your code harder to read and extend.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIXULAppInfo.idl#74
[2] http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIVersionComparator.idl#80

Comment 16

[Matt Brubeck (:mbrubeck)]

Comment on attachment 448975 [details] [diff] [review]
Patch v0.2

Checking "aEvent.isTrusted" separately for each message seems repetitive and maybe fragile (easy to forget when adding a new one).  Also, it makes me wonder why we don't do it for some events like "DOMLinkAdded".

If that's intentional, then a comment would be good.  Or if it's not, should we check aEvent.isTrusted for all events, before the "switch"?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

You don't need to check event.isTrusted for event listeners added to chrome objects, unless they were added with the aWantsUntrusted=true (a magical fourth parameter to addEventListener in chrome).

Comment 18

[Matt Brubeck (:mbrubeck)]

(In reply to comment #17)
> You don't need to check event.isTrusted for event listeners added to chrome
> objects, unless they were added with the aWantsUntrusted=true (a magical fourth
> parameter to addEventListener in chrome).

So we don't need to check isTrusted for any of the events in this file, right?  (Or does it mean we have to check on all of them?  I don't know if the global object in the content process script is "chrome" or not.)

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Yeah, it looks to me like the isTrusted checks are unnecessary in that file (it's "chrome" in the "runs in a privileged chrome:// docshell" sense, despite being in the "content" process - e10s kinda throws a wrench in the standard content/chrome nomenclature).

Comment 20

[Matt Brubeck (:mbrubeck)]

Comment on attachment 448975 [details] [diff] [review]
Patch v0.2

Thanks, Gavin.

r+ with the removal of the isTrusted checks.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #20)
> (From update of attachment 448975 [details] [diff] [review])
> Thanks, Gavin.
> 
> r+ with the removal of the isTrusted checks.

Good catch and thanks for your comments Matt and Gavin, because removing the isTrusted I will just do a quick test to see if a fake custom event can fired those because I don't think we want that.

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch updated on trunk and with isTrusted removals

It seems the isTrusted are effectively unnecessary. I will land this patch as soon as the e10s actual trunk is clean (there is a zoom bug since yesterday night)

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

My zoom problem is because my electrolysis tree is too old and I missed DOMMetaAdded! I rebuild and I will land

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #23)
> My zoom problem is because my electrolysis tree is too old and I missed
> DOMMetaAdded! I rebuild and I will land

I still have some redraws problems i don't think I have seen before, which I guess are related to this errors on the console:
*&*&*&*&*&*&*&**&*&&*& FATAL ERROR: 'GetInterface' UNIMPLEMENTED: /home/steakdepapillon/Devel/electrolysis/netwerk/protocol/http/HttpChannelParent.cpp +283*&*&*& HttpChannelParent::GetInterface: uuid={314d8a54-1caf-4721-94d7-f6c82d9b82ed} not impl'd yet! File a bug!

In the doubt I wait for landing it to have more informations.

Comment 25

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #24)
> (In reply to comment #23)
> > My zoom problem is because my electrolysis tree is too old and I missed
> > DOMMetaAdded! I rebuild and I will land
> 
> I still have some redraws problems i don't think I have seen before, which I
> guess are related to this errors on the console:
> *&*&*&*&*&*&*&**&*&&*& FATAL ERROR: 'GetInterface' UNIMPLEMENTED:
> /home/steakdepapillon/Devel/electrolysis/netwerk/protocol/http/HttpChannelParent.cpp
> +283*&*&*& HttpChannelParent::GetInterface:
> uuid={314d8a54-1caf-4721-94d7-f6c82d9b82ed} not impl'd yet! File a bug!
> 

This is bug 570675 not sure this is related.

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch for m-b (WIP)

This patch is for mobile-browser. It has parts of other patches too. I am working on getting the patch to pass browser-chrome tests. Currently fails 40 tests.

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch for m-b

Some cleanup of tests and browser.xml to get tests to pass.

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 449864 [details] [diff] [review]
patch for m-b

>   handleEvent: function(aEvent) {
>+    dump("--- event: " + aEvent.type + "\n");
>+    let document = content.document;
 
>+  receiveMessage: function(aMessage) {
>+    dump("*** message: " + aMessage.name + "\n");
>+    let browser = aMessage.target;

Looks good but do you really want to keep the dumps?

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #28)

> Looks good but do you really want to keep the dumps?

Nope. They were just for debugging.

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-b:
http://hg.mozilla.org/mobile-browser/file/0bfff6592da5

Marking FIXED since we wanted this on mobile-browser

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

Oops, real changeset is:
http://hg.mozilla.org/mobile-browser/rev/0bfff6592da5