570969 - [f10s] Remove isRootWindow and use currentInnerWindowId instead

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: Patch

Comment 1

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

btw I don't really like:
+      windowId: aWebProgress.DOMWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID,

so if you have a better idea it would be nice!

Comment 2

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

Comment on attachment 450120 [details] [diff] [review]
Patch

>diff -r d0c2030cc255 chrome/content/bindings/browser.js

Change all sendAsyncMessage calls to sendSyncMessage in this file.

>diff -r d0c2030cc255 chrome/content/bindings/browser.xml

>+      <field name="currentInnerWindowId">null</field>
>+

Let's just call it contentWindowId

>       <field name="_webProgress"><![CDATA[

>+          get DOMWindow() { throw "DOMWindow: Not Remoteable" },
>+          isLoadingDocument: false,

Change to:
            get isLoadingDocument() { throw "isLoadingDocument: Not Remoteable"; },

Change in remote-browser too

>               case "WebProgress:LocationChange":
>-                if (json.isRootWindow) {
>+                let currentURI = this._browser._ios.newURI(json.location, null, null);
>+
>+                args = [
>+                  { windowId: json.windowId, browser: this._browser },
>+                  {},
>+                  currentURI
>+                ];

Not sure why, but I like using "locationURI" instead of "currentURI". Change in remote-browser too

>       ]]></field>

You added back _webProgress, but also add back the "webProgress" property. I don't want people to use browser._webProgress

>diff -r d0c2030cc255 chrome/content/browser.js

>   onStateChange: function onStateChange(aWebProgress, aRequest, aStateFlags, aStatus) {
>     // ignore notification that aren't about the main document (iframes, etc)
>-    if (aWebProgress.DOMWindow != this._tab.browser.contentWindow)
>+    if (aWebProgress.windowId != this._tab.browser.currentInnerWindowId)

Change to this._tab.browser.contentWindowId

>   onLocationChange: function onLocationChange(aWebProgress, aRequest, aLocationURI) {
>     // ignore notification that aren't about the main document (iframes, etc)
>-    if (aWebProgress.DOMWindow != this._tab.browser.contentWindow)
>+    if (aWebProgress.windowId != this._tab.browser.currentInnerWindowId)

Change to this._tab.browser.contentWindowId

>     this._listener = new ProgressController(this);
>-    browser.webProgress.addProgressListener(this._listener, flags);
>+    browser._webProgress.addProgressListener(this._listener, flags);

No, leave this alone.

Comment 3

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

Attachment: Patch v0.2

This patch address comments and also correct a little bug found during debugging

Comment 4

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

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

But we still fail tests without changing sendAsyncMessage to sendSyncMessage in bindings/browser.js.

r+ with that change. we can figure out why async is failing in a followup.

Comment 5

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

http://hg.mozilla.org/mobile-browser/rev/212d5877bd97