570823 - [e10s] Add scrollTo / scrollBy message based API to browser binding

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We need to replace browser.contentWindow.scrollTo (and .scrollBy) with a message based API. Also, we need support for accessing the current .scrollX and .scrollY too.

Comment 1

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

For nested iframes scrolling i think we could add a "windowId" property on the json passed from chrome to content.

browser.messageManager.sendAsyncMessage("Content;ScrollTo", { windowId: browser.contentWindowId, x: scrollbox.scrollX, y: scrollbox.scrollY });

For getting the scrollX/scrollY coordinates, I am not fully sure of what should be done here. 
I wondering if we should add a message listener on the chrome side that listen for messages sent by the content when a "onscroll" event is fired.
This sounds the right solution to me but at http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#2864 we're calling scrollContentToBrowser to sync the view with the real scroll positions. I think this bug will go away once layers gonna be used.

Comment 2

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

Attachment: patch

This patch is a temporary fix. It removes the need to use .contentWindow when scrolling the canvas and the content. We should be able to remove it when Layers lands.

The patch does not handle any iframe scrolling.

Comment 3

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

Comment on attachment 452970 [details] [diff] [review]
patch

>diff --git a/app/mobile.js b/app/mobile.js
>--- a/app/mobile.js
>+++ b/app/mobile.js
>@@ -36,17 +36,17 @@
> 
> #filter substitution
> 
> pref("toolkit.defaultChromeURI", "chrome://browser/content/browser.xul");
> pref("general.useragent.extra.mobile", "@APP_UA_NAME_EXTRA@/@APP_VERSION_EXTRA@ Fennec/@APP_VERSION@");
> pref("browser.chromeURL", "chrome://browser/content/");
> 
> pref("browser.tabs.warnOnClose", true);
>-pref("browser.tabs.remote", false);
>+pref("browser.tabs.remote", true);
> 

forgot to remove it from the patch?

>diff --git a/chrome/content/BrowserView.js b/chrome/content/BrowserView.js
>   updatePageScroll: function updatePageScroll(aMessage) {
>     if (aMessage.target != this._browser || this._ignorePageScroll)
>       return;
> 
>     // XXX shouldn't really make calls to Browser
>-    Browser.scrollContentToBrowser();
>+    Browser.scrollContentToBrowser(aMessage.json.scrollX, aMessage.json.scrollY);
>   },

let json = aMessage.json;
Browser.scrollContentToBrowser(json.scrollX, json.scrollY);

>diff --git a/chrome/content/bindings/browser.js b/chrome/content/bindings/browser.js
>--- a/chrome/content/bindings/browser.js
>+++ b/chrome/content/bindings/browser.js
>@@ -297,8 +297,28 @@ let DOMEvents =  {
>           }
>         }
>         break;
>     }
>   }
> };
> 
> DOMEvents.init();
>+
>+let ContentScroll =  {
>+  init: function() {
>+    addMessageListener("Content:ScrollTo", this);
>+    addMessageListener("Content:ScrollBy", this);
>+  },
>+
>+  receiveMessage: function(message) {
>+    switch (message.name) {
>+      case "Content:ScrollTo":
>+        content.scrollTo(message.json.x, message.json.y);
>+        break;
>+      case "Content:ScrollBy":
>+        content.scrollBy(message.json.dx, message.json.dy);
>+        break;
>+    }
>+  }
>+};

Add a let json =  aMessage.json at the beginning of the receiveMessage callback and use it in the switch cases, also please replace message by aMessage.

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml
>--- a/chrome/content/bindings/browser.xml
>+++ b/chrome/content/bindings/browser.xml
>@@ -97,17 +97,17 @@
>           switch (aMessage.name) {
>             case "DOMPopupBlocked":
>               this.onPopupBlocked(aMessage);
>               break;
> 
>             case "pageshow":
>               this.onPageShow(aMessage);
> 
>-              if (this.mIconURL == "")
>+              if (this.mIconURL == "" && this._documentURI)
>                 this.mIconURL = this.documentURI.prePath + "/favicon.ico";

Is there cases where documentURI is not set when we hit pageshow? It should be define during the onLocationChange phase, so before the pageshow event.

>diff --git a/chrome/content/content.js b/chrome/content/content.js
>       case "scroll":
>-        sendSyncMessage("Browser:PageScroll", {});
>+        let cwu = Util.getWindowUtils(content);
>+        let scrollX = {};
>+        let scrollY = {};
>+        cwu.getScrollXY(false, scrollX, scrollY);
>+        sendSyncMessage("Browser:PageScroll", { scrollX: scrollX.value, scrollY: scrollY.value });
>         break;
>     }
>   },

The code to get the offsets is done into a helper in Util.js#getScrollOffset. We should use this one or replace the places where it is used into the code and remove it from Util.js.

The more I think and used Util.js, the more I would like to see Point and Rect classes live into a file called Geometry.js or anything else and import only this file with loadFrameScript.
I don't like the idea of having a file where it can live any type of code and load it anywhere... (I'll file a bug for that)


r+ with nits addressed.

Comment 4

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

(In reply to comment #3)

> >-              if (this.mIconURL == "")
> >+              if (this.mIconURL == "" && this._documentURI)
> >                 this.mIconURL = this.documentURI.prePath + "/favicon.ico";
> 
> Is there cases where documentURI is not set when we hit pageshow? It should be
> define during the onLocationChange phase, so before the pageshow event.

Looks like the initial about:blank never causes the onLocationChange to fire, so we can get into a _documentURI = null situation. It doesn't seem to happen for real pages though, so this change is just a defensive change.

> r+ with nits addressed.

All other changes made

Comment 5

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

pushed with changes:
http://hg.mozilla.org/mobile-browser/rev/c5379915ff5c