Closed Bug 570823 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(1 file)

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.
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.
Attached patch patchSplinter Review
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.
Attachment #452970 - Flags: review?(21)
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.
Attachment #452970 - Flags: review?(21) → review+
(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
pushed with changes:
http://hg.mozilla.org/mobile-browser/rev/c5379915ff5c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.