566500 - e10s: Move viewport metadata support into the browser XBL binding

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Move Utils.getViewportMetadata into a new property in bindings/browser.xml (both remote and non-remote versions), so it can be used the same way for remote and non-remote browsers.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

Not at all finished, but here is a snapshot that mostly works, and reduces the diff between mobile-e10s and mobile-browser.  Depends on the patch from bug 568092.

Still need to tweak the message-passing bits some more, and move the actual calculations from Util.js into bindings/browser.js.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 2

Updated patch:
* Move code from Util.js to bindings/browser.js
* Uses DOMContentLoaded instead of endLoading (happens sooner, and there is already a listener in the browser binding).
* Correctly handles pages in the bfcache using the pageshow event.

I did not end up using the "viewportMetadata" property in browser.xml for anything, so the new patch just passes the metadata along with the message.  Maybe we should add a "startLoading" handler in browser.xml - or use the new DOMWindowCreated event - and then we could replace browserViewportState.metaData with a browser.xml property...

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 3

Since it doesn't look like this really fits into browser.xml, moving it to frame-content instead.  Otherwise, basically the same as the last patch.

This will call getViewportMetadata multiple times on some pages.  I'm going to try to fix that before finishing this patch.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This is ready for review.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

very minor cleanup

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

More minor cleanup: Since we are keeping this in Fennec code, we can use Util.getWindowUtils and Util.clamp.

Comment 7

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

Comment on attachment 448931 [details] [diff] [review]
patch v3

>diff -r 8fb9e9bb5496 chrome/content/browser.js

>diff -r 8fb9e9bb5496 chrome/content/frame-content.js
>   receiveMessage: Util.receiveMessage,
> 
>+

No need for the extra line

>+let ViewportHandler = {
>+  metadata: null,
>+
>+  init: function init() {

>+    this.progresscontroller = new ProgressController(this)
>+    this.progresscontroller.start();

Looks like we could change to DOMWindowCreated in the future. If not, I'd like to rework ProgressController a bit.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: mobile-browser patch

This patch updates the mobile-browser viewport code to be as identical as possible to mobile-e10s.

Comment 9

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

Found some strange test failures with the mobile-browser patch. Investigating

Running chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js...
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Tab Opened
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL Matches blank page 0
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Normal 'browser' width is 800 pixels - Got 0px, expected 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes window width is 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes zoom.dpiScale is 1.5
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL is chrome://mochikit/content/browser/mobile/chrome/browser_viewport_00.html
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport width=800 [got 0, expected 800] - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport scale=1 [got 4, expected 1] - Got false, expected true
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom enabled
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL Matches blank page 1
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Normal 'browser' width is 800 pixels - Got 0px, expected 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes window width is 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes zoom.dpiScale is 1.5
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL is chrome://mochikit/content/browser/mobile/chrome/browser_viewport_01.html
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport width=533.33 [got 0, expected 533.33] - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport scale=1.5 [got 4, expected 1.5] - Got false, expected true
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom enabled
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL Matches blank page 2
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Normal 'browser' width is 800 pixels - Got 0px, expected 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes window width is 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes zoom.dpiScale is 1.5
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL is chrome://mochikit/content/browser/mobile/chrome/browser_viewport_02.html
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport width=533.33 [got 0, expected 533.33] - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport scale=1.5 [got 4, expected 1.5] - Got false, expected true
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom enabled
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL Matches blank page 3
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Normal 'browser' width is 800 pixels - Got 0px, expected 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes window width is 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes zoom.dpiScale is 1.5
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL is chrome://mochikit/content/browser/mobile/chrome/browser_viewport_03.html
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport width=533.33 [got 0, expected 533.33] - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport scale=1.5 [got 4, expected 1.5] - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom disabled
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom in does nothing
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom out does nothing
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL Matches blank page 4
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Normal 'browser' width is 800 pixels - Got 0px, expected 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes window width is 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes zoom.dpiScale is 1.5
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL is chrome://mochikit/content/browser/mobile/chrome/browser_viewport_04.html
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport width=200 [got 0, expected 200] - Got false, expected true
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport scale=4.00 [got 4, expected 4]
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom enabled
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL Matches blank page 5
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Normal 'browser' width is 800 pixels - Got 0px, expected 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes window width is 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes zoom.dpiScale is 1.5
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL is chrome://mochikit/content/browser/mobile/chrome/browser_viewport_05.html
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport width=2000 [got 0, expected 2000] - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport scale=1.125 [got 4, expected 1.125] - Got false, expected true
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom enabled
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL Matches blank page 6
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Normal 'browser' width is 800 pixels - Got 0px, expected 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes window width is 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes zoom.dpiScale is 1.5
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL is chrome://mochikit/content/browser/mobile/chrome/browser_viewport_06.html
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport width=266.67 [got 0, expected 266.67] - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport scale=3 [got 4, expected 3] - Got false, expected true
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom enabled
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL Matches blank page 7
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Normal 'browser' width is 800 pixels - Got 0px, expected 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes window width is 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes zoom.dpiScale is 1.5
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL is chrome://mochikit/content/browser/mobile/chrome/browser_viewport_07.html
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport width=2000 [got 0, expected 2000] - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport scale=1.125 [got 4, expected 1.125] - Got false, expected true
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom enabled
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL Matches blank page 8
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Normal 'browser' width is 800 pixels - Got 0px, expected 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes window width is 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes zoom.dpiScale is 1.5
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL is chrome://mochikit/content/browser/mobile/chrome/browser_viewport_08.html
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport width=10000 [got 0, expected 10000] - Got false, expected true
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport scale=4 [got 4, expected 4]
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom enabled
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL Matches blank page 9
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Normal 'browser' width is 800 pixels - Got 0px, expected 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes window width is 800px
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Test assumes zoom.dpiScale is 1.5
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | URL is chrome://mochikit/content/browser/mobile/chrome/browser_viewport_09.html
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport width=533.33 [got 0, expected 533.33] - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Viewport scale=1.5 [got 4, expected 1.5] - Got false, expected true
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom disabled
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom in does nothing
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | Zoom out does nothing
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | before wait for focus -- loaded: complete active window: ([object ChromeWindow @ 0x9b245b8 (native @ 0x9b2ca3c)]) chrome://browser/content/browser.xul focused window: ([object ChromeWindow @ 0x9b245b8 (native @ 0x9b2ca3c)]) chrome://browser/content/browser.xul desired window: ([object ChromeWindow @ 0x9b245b8 (native @ 0x9b2ca3c)]) chrome://browser/content/browser.xul child window: ([object ChromeWindow @ 0x9b245b8 (native @ 0x9b2ca3c)]) chrome://browser/content/browser.xul docshell visible: true
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | already focused
TEST-PASS | chrome://mochikit/content/browser/mobile/chrome/browser_viewport.js | maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow @ 0x9b245b8 (native @ 0x9b2ca3c)]) chrome://browser/content/browser.xul focused window: ([object ChromeWindow @ 0x9b245b8 (native @ 0x9b2ca3c)]) chrome://browser/content/browser.xul desired window: ([object ChromeWindow @ 0x9b245b8 (native @ 0x9b2ca3c)]) chrome://browser/content/browser.xul child window: ([object ChromeWindow @ 0x9b245b8 (native @ 0x9b2ca3c)]) chrome://browser/content/browser.xul docshell visible: true

Comment 10

[Matt Brubeck (:mbrubeck)]

The broken tests (and broken behavior with new tabs in general) are fixed by bug 566024.

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: mobile-browser patch v2

With the global messageManager (bug 566024) we no longer need the executeSoon hack around loadFrameScript.

Comment 12

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

Comment on attachment 450186 [details] [diff] [review]
mobile-browser patch v2

>+  getWindowUtils: function getWindowUtils(win) {
>+    return win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
>+  },
>+
>+  getScrollOffset: function getScrollOffset(win) {
>+    var cwu = Util.getWindowUtils(win);
>+    var scrollX = {};
>+    var scrollY = {};
>+    cwu.getScrollXY(false, scrollX, scrollY);
>+    return new Point(scrollX.value, scrollY.value);
>+  },

Can you use "aSomething" for your parameters? This is not really useful here
but it can helps when you are into a big function to know that this variable is
in fact a parameter (and this is cheap)

Also use "var" instead of "let", i know the code from mobile e10s use var but
this is probably a mistake.

>+
>+  /**
>+   * Put this function in your object for event handling.  This will call a function
>+   * specific to your event, e.g. "handleMousedown" or "handleDOMLinkAdded".
>+   */
>+  handleEvent: function(e) {
>+    let name = "handle" + Util.capitalize(e.type);
>+    let fn = this[name];
>+    return fn.apply(this, arguments);
>+  },
>+

Please use "aEvent", this is done in mostly all the places in the rest of the
codebase.
Also I don't thhink we need it right?

>+  /**
>+   * Similar to handleEvent, but for cross-process messages.
>+   */
>+  receiveMessage: function(message) {
>+    let name = "receive" + Util.capitalize(message.name);
>+    let fn = this[name] || this["receiveDefault"];
>+    if (!fn)
>+      throw "Received a message " + message.name + " but did not do anything about it!";
>+
>+    let args = [message].concat(message.json);
>+    return { name: "Response", message: fn.apply(this, args) };
>+  },

We don't need it for your patch, right?

>+/**
>+ * Helper class to nsITimer that adds a little more pizazz.  Callback can be an
>+ * object with a notify method or a function.
>+ */
>+Util.Timeout = function(callback) {
>+  this._callback = callback;
>+  this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+  this._active = false;
>+}

"aCallback"

>+
>+  /** Do the callback once.  Cancels other timeouts on this object. */
>+  once: function once(delay, callback) {
>+    if (callback)
>+      this._callback = callback;
>+    this.clear();
>+    this._timer.initWithCallback(this, delay, this._timer.TYPE_ONE_SHOT);
>+    this._active = true;
>+    return this;
>+  },

aDelay, aCallback

>+
>+  /** Do the callback every delay msecs. Cancels other timeouts on this object. */
>+  interval: function interval(delay, callback) {
>+    if (callback)
>+      this._callback = callback;
>+    this.clear();
>+    this._timer.initWithCallback(this, delay, this._timer.TYPE_REPEATING_SLACK);
>+    this._active = true;
>+    return this;
>+  },

aDelay, aCallback

>+
>+  startLoading: function() {
>+    this.metadata = null;
>+    sendAsyncMessage("FennecViewportMetadata", {});
>+  },
>+
>+  stopLoading: function() {
>+  },
>+
>+  updateMetadata: function notify() {
>+    this.metadata = this.getViewportMetadata();
>+    sendAsyncMessage("FennecViewportMetadata", this.metadata);
>+  },

I think we want to get rid of all these "Fennec" prefix to use something like
"Browser:ViewportMetadata"

>+
>+  getViewportMetadata: function getViewportMetadata() {
>+    let dpiScale = gPrefService.getIntPref("zoom.dpiScale") / 100;
>+
>+    let doctype = content.document.doctype;
>+    if (doctype && /(WAP|WML|Mobile)/.test(doctype.publicId))
>+      return { defaultZoom: dpiScale, autoSize: true };
>+
>+    let windowUtils = Util.getWindowUtils(content.document.defaultView);

content == content.document.defaultView

Comment 13

[Matt Brubeck (:mbrubeck)]

Attachment: mobile-browser patch v3

Addresses all of Vivien's feedback, except for renaming the Fennec* events.  (I'd rather leave that for another patch.)

Note: If we move getWindowUtils and getScrollOffset from Util.js to content.js, then we can stop loading Util.js in the content scope.  Do we expect to need these anywhere else?

Comment 14

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

(In reply to comment #13)
> Created an attachment (id=450208) [details]
> mobile-browser patch v3
> 
> Addresses all of Vivien's feedback, except for renaming the Fennec* events. 
> (I'd rather leave that for another patch.)
> 

Thanks. :)

> Note: If we move getWindowUtils and getScrollOffset from Util.js to content.js,
> then we can stop loading Util.js in the content scope.  Do we expect to need
> these anywhere else?

I think we still want to use Util.Timeout (maybe we could get rid of it if it is use only here) and we also need the "Rect" class from Util.js

Comment 15

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

Comment on attachment 450208 [details] [diff] [review]
mobile-browser patch v3

The new gloabl messageManager patch (bug 566024) is needed for this code to pass tests. That should land tomorrow.

Comment 16

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

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/67517d995cb2