Closed Bug 566500 Opened 10 years ago Closed 10 years ago

e10s: Move viewport metadata support into the browser XBL binding

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

Attachments

(2 files, 7 obsolete files)

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.
Depends on: 567831
Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch WIP 2 (obsolete) — Splinter Review
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...
Attachment #448628 - Attachment is obsolete: true
Attached patch WIP 3 (obsolete) — Splinter Review
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.
Attachment #448779 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
This is ready for review.
Attachment #448876 - Attachment is obsolete: true
Attachment #448887 - Flags: review?(mark.finkle)
Attached patch patch v2 (obsolete) — Splinter Review
very minor cleanup
Attachment #448887 - Attachment is obsolete: true
Attachment #448889 - Flags: review?(mark.finkle)
Attachment #448887 - Flags: review?(mark.finkle)
Attached patch patch v3Splinter Review
More minor cleanup: Since we are keeping this in Fennec code, we can use Util.getWindowUtils and Util.clamp.
Attachment #448889 - Attachment is obsolete: true
Attachment #448931 - Flags: review?(mark.finkle)
Attachment #448889 - Flags: review?(mark.finkle)
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.
Attachment #448931 - Flags: review?(mark.finkle) → review+
Attached patch mobile-browser patch (obsolete) — Splinter Review
This patch updates the mobile-browser viewport code to be as identical as possible to mobile-e10s.
Attachment #449964 - Flags: review?(mark.finkle)
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
The broken tests (and broken behavior with new tabs in general) are fixed by bug 566024.
Depends on: 566024
Attached patch mobile-browser patch v2 (obsolete) — Splinter Review
With the global messageManager (bug 566024) we no longer need the executeSoon hack around loadFrameScript.
Attachment #449964 - Attachment is obsolete: true
Attachment #450186 - Flags: review?(mark.finkle)
Attachment #449964 - Flags: review?(mark.finkle)
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
Attachment #450186 - Flags: feedback-
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?
Attachment #450186 - Attachment is obsolete: true
Attachment #450208 - Flags: review?(mark.finkle)
Attachment #450186 - Flags: review?(mark.finkle)
(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 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.
Attachment #450208 - Flags: review?(mark.finkle) → review+
pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/67517d995cb2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.