Use browser elements for browsing surface

VERIFIED FIXED

Status

defect
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: mfinkle, Assigned: stechz)

Tracking

Dependency tree / graph
Bug Flags:
in-testsuite ?
in-litmus -

Firefox Tracking Flags

(fennec2.0b1+)

Details

Attachments

(1 attachment, 14 obsolete attachments)

176.51 KB, patch
mfinkle
: review+
mbrubeck
: review+
Details | Diff | Splinter Review
This bug is about removing the tile canavs system and start using the native <browser> elements for rendering
Posted patch wip 1 (obsolete) — Splinter Review
Just a proof of concept patch. The idea is to see what breaks and find ways to fix the breakage. This patch:

* Removes TileManager.js and BrowserView.js from browser.xul
* Removes any use of browser view from browser.js and browser-ui.js
** This means panning and zoom are disabled for now
* Changes browserViewportState to viewportMetaData in Tab class
* Adds showBrowsers / hideBrowsers to BrowserUI
** bug 130078 is needed to float anything over a <browser>
** Fennec floats dialogs, menus, urlbar, awesomebar and tools panels over the browsers
** for now we simply collapse (can't hide or the docshell is destroyed) browsers when we want to float something over the content

With this patch, navigation is functional and the browsers themselves work fine. The chrome UI needs more help - panning is the big broken feature currently
Assignee: nobody → mark.finkle
Posted patch Another WIP patch (obsolete) — Splinter Review
I hadn't realized Mark already started working on this.  I thought I'd post my patch because it allows panning in same-process pages.  Pretty messy, but it's the gist.

Ultimately we want the browser element to handle calls to scrollTo for both remote-process and same-process.  Of course remote process would do the trick of translating the appropriate layer, applying the scroll in the other process, and then undoing the translation.  We'll also need APIs for somehow scrolling iframes and divs from the parent process.  We could possibly let the browser figure out what should be scrolled by giving it a point on the page, moving up the stack if that particular frame is already scrolled to its maximum.  Finally, we'll need a way to zoom using a scale transaction that is replaced by a real rendering whenever the content process gets around to it.

Here are problems I wasn't expecting:
* The embedded docshell rendering bug that Mark pointed out (we should apply the patch and see how it works)
* Rendering issues when sidebar comes out (it does not seem to paint immediately)
* Mousedown and mouseup event amplification.  Since we fake those events in content.js, we receive them again in InputHandler.js.  This wasn't a problem before since the generated events were dispatched in a separate place in the document.  I guess we need some way to tell the real events from the ones we generate.
Ben - I think the only reason you have panning working is "browser.tabs.remote" = false in your patch. I found that no mouse events make it from content to chrome when tabs are remote.

I have a crappy followup patch that moves MouseModule to content.js to pan the content, but the chrome doesn't pan.
I think we really want those mouse events to happen in the parent process!  We can't rely on content to send those events over.  It effectively removes the most important benefit to e10s: a responsive application.
(In reply to comment #4)
> I think we really want those mouse events to happen in the parent process!  We
> can't rely on content to send those events over.  It effectively removes the
> most important benefit to e10s: a responsive application.

Absolutely - but we need the shadow-tree work for that to work.
tracking-fennec: --- → 2.0+
OS: Linux → All
Hardware: x86 → All
Bit almost usable with cjones PuppetWidget implementation
Attachment #458561 - Attachment is obsolete: true
Depends on: 583302
Blocks: 583302
No longer depends on: 583302
Posted image Glitch (obsolete) —
This happens in a build with Oleg's updated patch on top of m-c with http://hg.mozilla.org/users/cjones_mozilla.com/mcmq applied up to LAST-REMOTE-FRAME-PATH.  To reproduce, flip browser.tabs.remote to true, then load google.com and click its <input> (which actually seems to get focus and key events correctly, yay!).  I'm not sure what's up with the translucent blue overlay; is that a frontend issue or something bug 583976 will fix?

(I know this patch is WIP, just want to point it out.)
I'm assuming that's the blue overlay we use in the front end to highlight a focused element. We must not be calculating the size of the element correctly.
Blocks: 585630
Some things to remember to remove:
* isOffscreenBrowser
* dispatching mouse events
* activating remote frame
* dispatching key events
Blocks: 539948
tracking-fennec: 2.0+ → 2.0b1+
Blocks: 540850
Blocks: 575286
Blocks: 534574
Assignee: mark.finkle → webapps
Blocks: 581104
Blocks: 581297
Blocks: 585922
Blocks: 565519
Blocks: 559437
Blocks: 542735
Blocks: 511987
Blocks: 500732
Blocks: 479862
Blocks: 588504
Blocks: 588446
fennec+<browser> seems to work as well in windows as on linux except for annoying assertion failures in prefs triggering a windows dialog.
Stover - Can you keep an updated patch in this bug? Something we could apply to m-b when ready. I want to avoid the merge pain we had with e10s.
Posted patch updated to m-b 4653534ec23c (obsolete) — Splinter Review
I took the liberty of marking the patches I knew to be obsolete.

Mark: I think the only thing much different in your patch was hiding browser elements on prefscreen/etc, is that right?  If not, go ahead and de-obsolete yours. :)

This is a diff of mobile-browser trunk and http://hg.mozilla.org/users/bstover_mozilla.com/mobile2/.
Attachment #455369 - Attachment is obsolete: true
Attachment #456952 - Attachment is obsolete: true
Attachment #467097 - Attachment is obsolete: true
Blocks: 590535
Blocks: 588856
Blocks: 588848
Blocks: 588238
Blocks: 570335
Blocks: 586288
Posted patch updated to m-b 4fb363178ae2 (obsolete) — Splinter Review
Latest diff (need rollup patch v3 from 590294).  As far as frontend goes, zooming, viewport setting, and default zoom are all supported. Pinch zoom and animated zoom need to be added back.

Whoever is interested in reviewing this: it would be wise to familiarize yourselves with this patch now. BrowserView is basically gone, and browser now has zoom and scroll APIs. Nothing is well commented yet, but I think this is pretty close to what we want.
Attachment #468758 - Attachment is obsolete: true
Attachment #470909 - Flags: feedback?(mbrubeck)
Attachment #470909 - Flags: feedback?(mark.finkle)
Patch was accidentally reversed.
Attachment #470911 - Flags: feedback?(mbrubeck)
Attachment #470911 - Flags: feedback?(mark.finkle)
Attachment #470911 - Flags: feedback?(21)
Attachment #470909 - Attachment is obsolete: true
Attachment #470909 - Flags: feedback?(mbrubeck)
Attachment #470909 - Flags: feedback?(mark.finkle)
Attachment #470911 - Attachment is patch: true
Attachment #470911 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 470911 [details] [diff] [review]
updated to m-b 4fb363178ae2 (not reversed)


>+/*BrowserView.Util = {
>   ensureMozScrolledAreaEvent: function ensureMozScrolledAreaEvent(aBrowser, aWidth, aHeight) {
>     let message = {};
>     message.target = aBrowser;
>     message.name = "Browser:MozScrolledAreaChanged";
>     message.json = { width: aWidth, height: aHeight };
> 
>     Browser._browserView.updateScrolledArea(message);
>   }
>+}; */

Thinking about keep this around? Why not axe it too?

>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js

>+    try {
>+      if (aEvent.view != window) {
>+        // XXX we'd really like to do this to stop dragging code, but at least in
>+        // non-e10s this is a problem.  Since we catch the simulated mouseup and
>+        // mousedown events, we eat up the ones we just generated.
>+        // evt.stopPropagation();
>+        // evt.preventDefault();
>+      }
>+    } catch (e) {};

Is this a "stop kinetic panning on tap" type of fix? If so, just remove it and we'll file a new bug.
Or is this a regression from current behavior?


(Lots of "dragger" code in here. I need to see how it relates to the browser.js code)


>   _targetIsContent: function _targetIsContent(aEvent) {

>+    return aEvent.view !== window || aEvent.target.tagName == "browser";

I think | aEvent.target.localName | is preferred since localName is always lowercase

>   _defaultDragger: {
>     isDraggable: function isDraggable(target, scroller) {

>-      return sX.value > rect.width || sY.value > rect.height;
>+      return { xDraggable: sX.value > rect.width, yDraggable: sY.value > rect.height };

draggableX and draggableY would fit our naming conventions better

        return { draggableX: sX.value > rect.width, draggableY: sY.value > rect.height };
>     // hide element highlight
>-    document.getElementById("tile-container").customClicker.panBegin();
>+    //document.getElementById("tile-container").customClicker.panBegin();

Just remove it? We could file a bug to re-implement the highlighter as CSS or something. Maybe someone could work on it in parallel?

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>           // give the form spacer area back to the content
>           // XXX this should probably be removed with layers
>           Browser.forceChromeReflow();
>           Browser.contentScrollboxScroller.scrollBy(0, 0);
>-          Browser._browserView.onAfterVisibleMove();

Hmm, if we can only remove the onAfterVisibleMove, we should update the comment too. Can't we kill the forceChromeReflow now too?

>diff --git a/chrome/content/bindings/browser.js b/chrome/content/bindings/browser.js

>+      case "Content:SetResolution":
>+        let cwu = Util.getWindowUtils(content);
>+        cwu.setResolution(json.zoomLevel, json.zoomLevel);
>+        sendAsyncMessage("Content:SetResolution:Return", { zoomLevel: json.zoomLevel });

Why do we need to send this back? We don't change the value, so I assume there must be some timing issue?

>+        break;
>+
>+      case "Content:SetDisplayportArea": {
>+        let displayport = new Rect(json.x, json.y, json.w, json.h).translateInside(this._contentArea);
>+        if (displayport.isEmpty())
>+          break;
>+
>+        let cwu = Util.getWindowUtils(content);
>+        cwu.setDisplayPort(displayport.x, displayport.y, displayport.width, displayport.height);
>+        sendAsyncMessage("Content:SetDisplayportArea:Return");

In this case I assume we send something back so the chrome knows we actually made a change, and not | break | early? Although, we could track the _contentArea in chrome. Where would we store it though? Tab class maybe. Storing it here encapsulates it better I guess.

Hmm, actually, we'd be storing it in the browser XBL, not the fennec front-end code....

>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
>@@ -150,16 +150,35 @@
>               alert(aMessage.json.message);
>               break;
> 
>             case "Prompt:Confirm":
>               return confirm(aMessage.json.message);
> 
>             case "Prompt:Prompt":
>               return prompt(aMessage.json.text, aMessage.json.value);
>+
>+            case "MozScrolledAreaChanged":
>+              this._widthInCSSPx = aMessage.json.width;
>+              this._heightInCSSPx = aMessage.json.height;
>+              this._viewportWidthInCSSPx = aMessage.json.viewportWidth;
>+              this._viewportHeightInCSSPx = aMessage.json.viewportHeight;
>+              this._updateDisplayport();
>+              break;
>+
>+            case "Content:SetDisplayportArea:Return":
>+              // XXX explain behavior
>+              this._flushingFastScroll = false;
>+              this._pendingPixelsX = 0;
>+              this._pendingPixelsY = 0;
>+              break;
>+
>+            case "Content:SetResolution:Return":
>+              this._zoomLevel = aMessage.json.zoomLevel;
>+              break;
>          }
>         ]]></body>
>       </method>
> 
>       <method name="_getLinkType">
>         <parameter name="aLink" />
>         <body><![CDATA[
>           let type = "";
>@@ -262,16 +281,20 @@
>                 ];
> 
>                 if (this._browser.contentWindowId != json.windowId) {
>                   this._browser.contentWindowId = json.windowId;
>                   this._browser._documentURI = json.documentURI;
>                   this._browser._searchEngines = [];
>                 }
> 
>+                this._browser._zoomLevel = 1;
>+
>+                this._browser._updateDisplayport();
>+
>                 this._notify(Components.interfaces.nsIWebProgress.NOTIFY_LOCATION,
>                              "onLocationChange",
>                              args);
> 
>                 break;
> 
>               case "WebProgress:StatusChange":
>                 args = [
>@@ -378,32 +401,173 @@
> 
>             this.pageReport.push(obj);
>             this.pageReport.reported = false;
>             this.updatePageReport();
>           ]]>
>         </body>
>       </method>

>+      <property name="widthInCSSPx"
>+      <property name="heightInCSSPx"
>+      <property name="widthInDevicePx"
>+      <property name="heightInDevicePx"

These names make me want to weep for days. We must endeavor to improve them - or drop the ones that can just be calc'ed. And how do either of these "widths" for example relate to the browser.width property itself?

>+      <property name="zoomLevel"
>+                onget="return this._zoomLevel;"
>+                readonly="true"/>
>+
>+      <property name="defaultZoomLevel"
>+                onget="return this._defaultZoomLevel;"
>+                onset="return this._setDefaultZoomLevel(val);"/>

This is weird. I would expect to set the | browser.zoomLevel = x | to change the zoom. The meaning of "default" is lost on me as an API.

>+      <property name="viewportWidthInCSSPx"
>+      <property name="viewportHeightInCSSPx"

More weeping

>+      <method name="_updateDisplayport">
>+        <body>
>+          <![CDATA[
>+            let frameLoader = this._frameLoader;
>+            this.messageManager.sendAsyncMessage("Content:SetDisplayportArea", {
>+              x: frameLoader.viewportScrollX / this._zoomLevel - 200,
>+              y: frameLoader.viewportScrollY / this._zoomLevel - 400,
>+              w: this._viewportWidthInCSSPx + 400,
>+              h: this._viewportHeightInCSSPx + 800

What's with the hard coded numbers here?

>+      <method name="_updateCSSViewport">

Why is this _updateCSSViewport, but above we have _updateDisplayport? Is the displayport not related to CSS the same way as the viewport? If the viewport is always CSS then using "CSS" is redundant and makes my head hurt

>+      <method name="setZoomLevel">

Why is this not the setter for "zoomLevel"

>+      <method name="flushScroll">

What's this all about and why do I as a dev using the API care about it?

>+      <method name="scrollBy">

>+            this.contentWindow.scrollBy(x, y);

>+      <method name="scrollTo">

>+            this.contentWindow.scrollTo(x, y);

>+      <method name="getPosition">

>+            let cwu = this.contentWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
>+                      getInterface(Components.interfaces.nsIDOMWindowUtils);

Why are we freely using | contentWindow | in this code? OH! We're in the non-remote part of the binding and you are adding sync'ed APIs

>       <constructor>

>           // Prompt remoting
>           ["Alert", "Confirm", "Prompt"].forEach(function(name) {
>-            this.messageManager.addMessageListener("Prompt:Alert" + name, this);
>+            this.messageManager.addMessageListener("Prompt:" + name, this);
>           }, this);

Hmm?

>+            // XXX comment this behavior
>+            this._pendingPixelsX += x;
>+            this._pendingPixelsY += y;

Yeah, pendingPixels kinda frightens me

>+            if (this._flushingFastScroll == false &&
>+                (Math.abs(this._pendingPixelsX) >= 150 || Math.abs(this._pendingPixelsY) >= 250)) {
>+              this._flushingFastScroll = true;
>+              this._updateDisplayport();

More hard coded numbers

>+      <method name="scrollTo">

Since this is implemented, can we drop the "Content:ScrollTo" message?
What about scrollBy? It seems to be missing from the remote binding

-- more to come ---
Blocks: 574315
> >+/*BrowserView.Util = {
> >   ensureMozScrolledAreaEvent: function ensureMozScrolledAreaEvent(aBrowser, aWidth, aHeight) {
> >     let message = {};
> >     message.target = aBrowser;
> >     message.name = "Browser:MozScrolledAreaChanged";
> >     message.json = { width: aWidth, height: aHeight };
> > 
> >     Browser._browserView.updateScrolledArea(message);
> >   }
> >+}; */
> 
> Thinking about keep this around? Why not axe it too?

I'm not sure if we still need this code, but I thought we might have to have it for XUL files in non-remote browsers. I haven't tried them yet.

> Is this a "stop kinetic panning on tap" type of fix? If so, just remove it and
> we'll file a new bug.
> Or is this a regression from current behavior?

Regression, I think. I believe we were canceling our own generated events to browsers now that they weren't invisible anymore. I need to go back and look at this again.

> (Lots of "dragger" code in here. I need to see how it relates to the browser.js
> code)

There's no refactoring going on; it all related to using browser elements to display content. I'll look through it.

> Just remove it? We could file a bug to re-implement the highlighter as CSS or
> something. Maybe someone could work on it in parallel?

Do we want to reimplement this in CSS right now? I wasn't going to change the functionality here. I'd be happy to have someone else take a look at this.

> Hmm, if we can only remove the onAfterVisibleMove, we should update the comment
> too. Can't we kill the forceChromeReflow now too?

I believe so!

> Why do we need to send this back? We don't change the value, so I assume there
> must be some timing issue?

The zoom level can't change until setResolution has been called. I'll explain below.

> >+        sendAsyncMessage("Content:SetDisplayportArea:Return");
> 
> In this case I assume we send something back so the chrome knows we actually
> made a change, and not | break | early? Although, we could track the
> _contentArea in chrome. Where would we store it though? Tab class maybe.
> Storing it here encapsulates it better I guess.
> 
> Hmm, actually, we'd be storing it in the browser XBL, not the fennec front-end
> code....

Yes, all this lives in the browser binding now. In this case, the return call was an attempt to make sure set display port wasn't called too many times from chrome. Setting the display port is not the fastest operation, so I don't call set display port if there's already a pending one.

I think this is actually not effective so I will remove it.

Before we move on, I better lay down some terminology on ya :)

* CSS viewport: this is what content thinks the viewport is (window.scrollX, window.scrollY, window.innerWidth, window.innerHeight) and relates to how the page is laid out. The dimensions of the viewport don't typically change after the page has been loaded. When talking about the CSS viewport, we using only think of CSS pixels and not device pixels.

* projection viewport: what the user sees on her screen. Its width and height in device pixels are proportional to the CSS viewport by the zoomlevel factor (put another way, its width and height in CSS pixels are the same as the CSS viewport). The position of the viewport does not necessarily line up with the CSS viewport at all (in fact, right now in this patch the CSS viewport's position is never synced up).

* display viewport (or displayport): an internal viewport that describes what area of the browser is currently cached on the chrome side (so that we instantly see it if we pan).

> >+      <property name="widthInCSSPx"
> >+      <property name="heightInCSSPx"
> >+      <property name="widthInDevicePx"
> >+      <property name="heightInDevicePx"
> 
> These names make me want to weep for days. We must endeavor to improve them -
> or drop the ones that can just be calc'ed. And how do either of these "widths"
> for example relate to the browser.width property itself?

These properties relate to the width and height of the document itself and has nothing to do with the viewport. browser.width tells us how many device pixels the browser element takes up, which is the same thing as the projection viewport's width in device pixels if content is fitted to the screen.

Better names are welcomed, but I find "widthInCSSPx" easier to understand than "width / zoomLevel" ("hmm...which way does the scale go again? what units are width in? device px?").

Unfortunately there are so many permutations of dimensions, viewports, and areas here that you will be better off naming variables in more detail than less.

> This is weird. I would expect to set the | browser.zoomLevel = x | to change
> the zoom. The meaning of "default" is lost on me as an API.

setZoomLevel does not happen immediately. This code is not necessarily true:

browser.zoomLevel = 2;
browser.zoomLevel == 2;  // almost definitely not true

I originally had this as a setter, but I think it masks what is really happening. So here's where the terminology is going to come in handy, brace yourself: the projection viewport (in device pixels) doesn't actually change until setResolution is called in the content process. So, even though the content is immediately scaled the zoom level hasn't changed yet.

Does that make sense? We might have to get a whiteboard out.

> >+      <property name="viewportWidthInCSSPx"
> >+      <property name="viewportHeightInCSSPx"
> 
> More weeping

Weeping is fine, but please tell me what you'd like to see here. Again, I think it's important that something indicating this is in CSS px is very important so you know what you are dealing with.

I honestly think these names are better than things like "setVisibleRect" or "getCriticalRect" or "getZoomRectForElement" because these names don't tell you the units (and I *still* sometimes forget what a "visible rect" vs a "critical rect" is, we should start using more standard terminology and define it well somewhere). Maybe Matt has some ideas here?

this.messageManager.sendAsyncMessage("Content:SetDisplayportArea", {
> >+              x: frameLoader.viewportScrollX / this._zoomLevel - 200,
> >+              y: frameLoader.viewportScrollY / this._zoomLevel - 400,
> >+              w: this._viewportWidthInCSSPx + 400,
> >+              h: this._viewportHeightInCSSPx + 800
> 
> What's with the hard coded numbers here?

Yes, this needs to probably be proportional to the browser's dimensions.

> Why is this _updateCSSViewport, but above we have _updateDisplayport? Is the
> displayport not related to CSS the same way as the viewport? If the viewport is
> always CSS then using "CSS" is redundant and makes my head hurt

See above. Not redundant.

> Why is this not the setter for "zoomLevel"

See above. Zoom level setting not immediate.

> >+      <method name="flushScroll">
> What's this all about and why do I as a dev using the API care about it?

This is currently being called in dragEnd to recenter the display viewport (that is, the cached pixels for content). I agree we should nix it and maybe add a timer in browser binding to sync up the display viewport after the scrolling values have settled down.

> >           // Prompt remoting
> >           ["Alert", "Confirm", "Prompt"].forEach(function(name) {
> >-            this.messageManager.addMessageListener("Prompt:Alert" + name, this);
> >+            this.messageManager.addMessageListener("Prompt:" + name, this);
> >           }, this);
> 
> Hmm?

Bad merge, sorry.

> >+            // XXX comment this behavior
> >+            this._pendingPixelsX += x;
> >+            this._pendingPixelsY += y;
> 
> Yeah, pendingPixels kinda frightens me

It's for caching content pixels (the display viewport).

> Since this is implemented, can we drop the "Content:ScrollTo" message?

Content:ScrollTo deals with the CSS viewport as opposed to the projection viewport. We should make sure once calls to scrollBy settle down, that the CSS viewport and the projection viewport are synced up.
I think I want to rename display viewport to something better named for what we are using it for in Fennec (cache viewport?).
Just some feedback on naming and units. In the current non-e10s <browser> we have the concept of zooming (text zoom and non-text zoom). In the non-text zoom, I think we are changing the "device" pixel width and height - just as we are in this patch. The main thing to consider is we don't use any terminology or multiple properties to give the API users the device pixels. The API is cleaner, imo, because we assume CSS pixels and drop the "CSS" suffix/identifier.
Oh, it turns out the InputHandler changes are needed because a scrollbox has been removed. customDragger no longer lives on a scrollbox, and InputHandler didn't jive with that.

I will clean this up.
Posted patch latest patch (obsolete) — Splinter Review
Most of code review comments addressed here. InputHandler now correctly works with non-remote and remote browsers using an overlay that sits on top of browser. This way we can tell what events we generated and what events we sent to browser.
Attachment #470911 - Attachment is obsolete: true
Attachment #471334 - Flags: feedback?(mbrubeck)
Attachment #471334 - Flags: feedback?(mark.finkle)
Attachment #471334 - Flags: feedback?(21)
Attachment #470911 - Flags: feedback?(mbrubeck)
Attachment #470911 - Flags: feedback?(mark.finkle)
Attachment #470911 - Flags: feedback?(21)
Posted patch latest patch (obsolete) — Splinter Review
Now working:
* animated zoom
* tap to highlight
* form helper zoom
* input handler fixes

Not sure what is missing functionality wise at this point.
Attachment #471334 - Attachment is obsolete: true
Attachment #471634 - Flags: feedback?(mbrubeck)
Attachment #471634 - Flags: feedback?(mark.finkle)
Attachment #471634 - Flags: feedback?(21)
Attachment #471334 - Flags: feedback?(mbrubeck)
Attachment #471334 - Flags: feedback?(mark.finkle)
Attachment #471334 - Flags: feedback?(21)
(In reply to comment #25)
> Not sure what is missing functionality wise at this point.

I'm not sure, if this should work already: panning of XUL and HTML iframe/div still doesn't work. This is bug #586288 which is dependend on this bug.
> I'm not sure, if this should work already: panning of XUL and HTML iframe/div
> still doesn't work. This is bug #586288 which is dependend on this bug.

Panning HTML iframe/div is in a separate bug that does depend on this one (not a beta 1 blocker): https://bugzilla.mozilla.org/show_bug.cgi?id=586288

I don't believe XUL panning is working yet either, will add that to my list.
Blocks: 588727
Blocks: 589799
Why is this bug blocking bug 585922?
No idea. Mark?
(In reply to comment #28)
> Why is this bug blocking bug 585922?

We thought it might be a rendering problem, but it seems to have been a regression from a non-rendering issue.
No longer blocks: 585922
Blocks: 594330
Posted patch Latest patch (obsolete) — Splinter Review
Latest diff. I will start adding new patches / bugs after this review so that we can have at least this diff reviewed.
Attachment #460512 - Attachment is obsolete: true
Attachment #463085 - Attachment is obsolete: true
Attachment #471634 - Attachment is obsolete: true
Attachment #473141 - Flags: review?(mbrubeck)
Attachment #473141 - Flags: review?(mark.finkle)
Attachment #471634 - Flags: feedback?(mbrubeck)
Attachment #471634 - Flags: feedback?(mark.finkle)
Attachment #471634 - Flags: feedback?(21)
Blocks: 594475
Comment on attachment 473141 [details] [diff] [review]
Latest patch

First batch of review comments:

>+++ b/chrome/content/AnimatedZoom.js

>+const animatedZoom = {

The convention in other Fennec code is to capitalize global objects, even if
they are not constructors.

>+    this.beginTime = Date.now();

Use window.mozAnimationStartTime instead of Date.now().

>+    if (this.zoomRect) {
>+      this.zoomFrom = this.zoomRect;
>+    }
>+    else {

We're not that consistent about it, but I think Fennec style is to have the
"else" and both curly braces on the same line.

>+++ b/chrome/content/InputHandler.js

>+      if (counter < 1) {
>+        // update browser to interpolated rectangle
>+        let rect = this.zoomFrom.blend(this.zoomTo, Math.min(counter, 1));

Math.min is redundant here, since we know counter < 1.

>+      }
>+      else {
>+        // last cycle already rendered final scaled image, now clean up
>+        this.finish();
>+      }
>+    }
>+    catch(e) {
>+      this.finish();
>+      throw e;
>     }

"else" and "catch" on same line as braces (if we actually care about that).

>+    let oldDragger = this._dragger;

This is not used.

>+    if (this._dragger && this._dragger.isDraggable(targetScrollbox, targetScrollInterface))
>       this._doDragStart(aEvent);
>       // ...
>+      if (this._dragger) {
>         // do not allow axis locking if panning is only possible in one direction
>+        let draggable = this._dragger.isDraggable(targetScrollbox, targetScrollInterface);
>+        dragData.locked = !draggable.x || !draggable.y;
>       }

In the first "if" statement above, I think you need to check the .x and .y
properties of the isDraggable result.

You could move "let draggable" outside of the first "if" statement and then
use it in both places... assuming that the result won't change between those
two lines.

>     // save the initial gesture start point as reference
>     [this._pinchStartX, this._pinchStartY] =
>         Browser.transformClientToBrowser(aEvent.clientX, aEvent.clientY);
>+
>+    let scrollX = {}, scrollY = {};
>+    getBrowser().getPosition(scrollX, scrollY);
>+    this._pinchStartX += scrollX.value;
>+    this._pinchStartY += scrollY.value;

Should these be adjusted for scale?  I'll do some tests to try to find out...

>+++ b/chrome/content/browser.js

>+  getPageZoomLevel: function getPageZoomLevel() {
>+    let browserW = this._browser._widthInCSSPx;
>+    return this._browser.getBoundingClientRect().width / browserW;
>+  },

Should this be moved to browser.xml?  (The only reason I suggest this is to
prevent leakage of the "private" _widthInCSSPx property.)
Depends on: 595052
Comment on attachment 473141 [details] [diff] [review]
Latest patch

A quick look through the patch shows a merge problem with cset http://hg.mozilla.org/mobile-browser/rev/0a7c68052c8a

This patch backs out that cset
Sorry, that patch hasn't been merged over yet. This must have landed while I was doing that diff.
Blocks: 595159
Blocks: 588624
No longer blocks: 542735
Blocks: 559044
No longer blocks: 588504
Comment on attachment 473141 [details] [diff] [review]
Latest patch

Okay, here comes the rest of it.  Lots of comments, but nothing I would block the landing for.  Looks great, and removes over 2000 lines!

>+      <method name="getPosition">
>+        <parameter name="scrollX"/>
>+        <parameter name="scrollY"/>

It seems like most or all uses of this method would be simplified if it
returned [x,y] as a JavaScript array, rather than using XPCOM-style outparams.

Or if there's a reason to keep the outparams, maybe we could make them
optional, and *also* return their values in an array.  Something like:

  let x = scrollX||{}, y = scrollY||{};
  // ...get x and x...
  return [x.value, y.value];

>+      <field name="_widthInCSSPx">0</field>
>+      <field name="_heightInCSSPx">0</field>
>+      <property name="_widthInDevicePx"

Can we name these _documentWidth... and _documentHeight... (or maybe the
shorter _pageWidth/pageHeight...)?  The names didn't make it clear to me
(width of what?).

>+      <field name="_zoomLevel">1</field>
>+      <property name="scale"
>+                onget="return this._zoomLevel;"
>+                readonly="true"/>

I find it a little odd how "zoomLevel" and "scale" are used interchangeably.
Shall we standardize on "scale" everywhere, since that's what the platform
uses?  Not a big deal, though - it does not actually impede my understanding.

>+      <method name="setScale">

Seems like this could be a setter on the "scale" property.  As implemented, it
always updates the "scale" property immediately (or throws an exception).

>+      <!-- This determines what percentage of cached pixels are not yet visible before the cache
>+           is refreshed. For instance, if we recached at 50% and there are originally a total of
>+           400px offscreen, we'd refresh once 200 of those pixels have been scrolled into
>+           view. -->

This would be less ambiguous if you used any number besides 50% in your example.

>+     <field name="_recacheRatio">0</field>

Having this set to zero gives me totally irrational fears of divide by zero
errors.  Can it default to 1 just to ease my mind?  (And, well, maybe make it
more robust against changes to the constructor code... but I'm stretching
now.)

>+++ b/chrome/content/browser-ui.js

>   show: function show(aRects) {

Repeated use of getBrowser().scale in this function - want to add a local
"scale" variable (for conciseness, and to make it clear we are not changing it
from one line to the next)?

>+    }, new Rect(0, 0, 0, 0)).map(function(val) { return val * getBrowser().scale; })

If you want to get cute here you can use a JavaScript 1.8 closure expression:

    .map(function(val) val * getBrowser().scale)

>       let x = (marginLeft + marginRight + margin + aCaretRect.x - aElementRect.x) < viewAreaWidth
>                ? aElementRect.x - margin - marginLeft
>                : aCaretRect.x - viewAreaWidth + margin + marginRight;
>       // use the adjustet Caret Y minus a margin four our visible rect
>       let y = harmonizedCaretY - margin;
> 
>+      let scrollX = {}, scrollY = {};
>+      getBrowser().getPosition(scrollX, scrollY);
>+      let vis = new Rect(scrollX.value, scrollY.value, window.innerWidth, window.innerHeight);
>+      x *= getBrowser().scale;
>+      y *= getBrowser().scale;

Move the last two lines up above the blank line - or maybe just combine them
into the "let x" and "let y" statements.

Repeated use of getBrowser().scale again; use a local variable?

>+++ b/chrome/content/bindings/browser.js

>+    // Warning, total hack ahead. All of the real-browser related scrolling code
>+    // lies in a pretend scrollbox here. Let's not land this as-is. Maybe it's time
>+    // to redo all the dragging code.
>+    this.contentScrollbox = container;
>+    this.contentScrollboxScroller = {

Do we need to fix this before landing?

>   // cmd_scrollBottom does not work in Fennec (Bug 590535).
>+  scrollContentToBottom: function scrollContentToBottom() {
>+    this.contentScrollboxScroller.scrollTo(0, Number.MAX_VALUE);
>+    this.pageScrollboxScroller.scrollTo(0, Number.MAX_VALUE);
>   },

For some reason this causes the urlbar to pop up when scrolling to bottom.
I'm not sure if this is worth fixing in a followup, or if we can just get rid
of this function and use cmd_scrollBottom directly now.

>+    // XXX change
>     rect = Rect.fromRect(Browser.contentScrollbox.getBoundingClientRect()).map(Math.round);
>     if (doffset.y < 0 && rect.bottom < height)
>       y = Math.max(doffset.y, rect.bottom - height);
>     if (doffset.y > 0 && rect.top > 0)
>       y = Math.min(doffset.y, rect.top);

XXX?

>   mouseDown: function mouseDown(aX, aY) {
>     // Ensure that the content process has gets an activate event
>+    let browser = getBrowser();
>     let fl = browser.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader;
>+    browser.focus();

Get rid of "let browser" here.

>   updateViewportMetadata: function updateViewportMetadata(metaData) {
>     let browser = this._browser;
>     if (!browser)
>       return;
> 
>+    this.metaData = metaData;
>     this.updateViewportSize();
>   },

Since this function now does nothing but call updateViewportSize, let's get
rid of it.  (The caller can just set tab.metaData and then call
updateViewportSize, or we can add an optional parameter to
updateViewportSize.)

>+  /**
>+   * XXX document me
>+   */
>   resetZoomLevel: function resetZoomLevel() {
>+    let browser = this._browser;
>+    this._defaultZoomLevel = browser.scale;
>+  },

Can we make the function a one-liner?  For the documentation, something like
"record the initial zoom level when a page first loads".  (Or we could just
inline this whole thing since it's only called in one place.)

>+  /**
>+   * XXX document me
>+   */
>+  updateDefaultZoomLevel: function updateDefaultZoomLevel() {

How about:
/** Re-calculate the default zoom level when the page size changes. */
Attachment #473141 - Flags: review?(mbrubeck) → review+
Depends on: 595403
Blocks: 591379
Comment on attachment 473141 [details] [diff] [review]
Latest patch


>diff --git a/chrome/content/AnimatedZoom.js b/chrome/content/AnimatedZoom.js
>-function AnimatedZoom(aBrowserView) {
>-  this.bv = aBrowserView;
>+const animatedZoom = {

Use "AnimatedZoom"

>-  this.snapshotRect = this.bv.getVisibleRect().inflate(2);
>+    Browser.hideSidebars();
>+    Browser.hideTitlebar();
>+    Browser.forceChromeReflow();

Can Browser.forceChromeReflow be removed altogether?

>+  updateTo: function(nextRect) {
>+    let zoomRatio = window.innerWidth / nextRect.width;
>+    let zoomLevel = getBrowser().scale * zoomRatio;
>+    // XXX using the underlying frameLoader APIs is undesirable and is not a
>+    // pattern to propagate. The browser binding should be taking care of this!
>+    // There is some bug that I have not yet discovered that make browser.scrollTo
>+    // not behave correctly and there is no intelligence in browser.scale to keep
>+    // the actual resolution changes small.

Bug filed for this?

>+    getBrowser()._frameLoader.setViewportScale(zoomLevel, zoomLevel);
>+    getBrowser()._frameLoader.scrollViewportTo(nextRect.left * zoomRatio, nextRect.top * zoomRatio);

Matt commented on the getBrower() uses. I am not sure the performance hit is large, but a local is fine by me too

>+  finish: function() {
>+    window.removeEventListener("MozBeforePaint", this, false);
>+    Browser.setVisibleRect(this.zoomTo || this.zoomRect);

Should we favor .zoomTo before .zoomRect? Does it make a difference?

>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js

>-    if (targetScrollInterface && this._dragger.isDraggable(targetScrollbox, targetScrollInterface))
>+    if (this._dragger && this._dragger.isDraggable(targetScrollbox, targetScrollInterface))
>       this._doDragStart(aEvent);

isDraggable always returns something now. Don't you need to check isDraggable(..).x and .y  ?

>-    return false;
>+    return target && target.id == "inputhandler-overlay";

Hmm, I need to look at what "inputhanlder-overlay" is all about

>   _pinchStart: function _pinchStart(aEvent) {
>-    let bv = Browser._browserView;
>     // start gesture if it's not taking place already, or over a XUL element
>-    if (this._pinchZoom || (aEvent.target instanceof XULElement) || !bv.allowZoom)
>+    if (this._pinchZoom || (aEvent.target instanceof XULElement) || !Browser.selectedTab.allowZoom)

You use getBrowser() in lots of other places. Is one way nicer than the other?

>     // hide element highlight
>-    document.getElementById("tile-container").customClicker.panBegin();
>+    // XXX ugh, this is awful. None of this code should be in InputHandler.
>+    document.getElementById("inputhandler-overlay").customClicker.panBegin();


I think all of this element highlight code should be in content.css, handle via CSS now 

>   _pinchUpdate: function _pinchUpdate(aEvent) {
>-    this._pinchZoomLevel = Browser._browserView.clampZoomLevel(this._pinchZoomLevel);
>+    this._pinchZoomLevel = Browser.selectedTab.clampZoomLevel(this._pinchZoomLevel);

You use getBrowser() in the function above

>+    getBrowser().getPosition(scrollX, scrollY);
>+    pX += scrollX.value;
>+    pY += scrollY.value;

And in this one too

>     // redraw zoom canvas according to new zoom rect
>     let rect = Browser._getZoomRectForPoint(2 * this._pinchStartX - pX,
>                                             2 * this._pinchStartY - pY,
>                                             this._pinchZoomLevel);

Seems like all of this zooming and transforming should be in the <browser> binding now. I think Matt suggested something similar

>diff --git a/chrome/content/Util.js b/chrome/content/Util.js
>+  /**
>+   * Determines whether a home page override is needed.
>+   * Returns:
>+   *  "new profile" if this is the first run with a new profile.
>+   *  "new version" if this is the first run with a build with a different
>+   *                      Gecko milestone (i.e. right after an upgrade).
>+   *  "none" otherwise.
>+   */
>+  needHomepageOverride: function needHomepageOverride() {
>+    let savedmstone = null;
>+    try {
>+      savedmstone = Services.prefs.getCharPref("browser.startup.homepage_override.mstone");
>+    } catch (e) {}
>+
>+    if (savedmstone == "ignore")
>+      return "none";
>+
>+#expand    let ourmstone = "__MOZ_APP_VERSION__";
>+
>+    if (ourmstone != savedmstone) {
>+      Services.prefs.setCharPref("browser.startup.homepage_override.mstone", ourmstone);
>+
>+      return (savedmstone ? "new version" : "new profile");
>+    }
>+
>+    return "none";
>+  },
>+

Remove this (bad merge)

>diff --git a/chrome/content/bindings/browser.js b/chrome/content/bindings/browser.js

> let ContentScroll =  {
>   init: function() {
>     addMessageListener("Content:ScrollTo", this);
>     addMessageListener("Content:ScrollBy", this);
>+    addMessageListener("Content:SetResolution", this);
>+    addMessageListener("Content:SetCacheViewport", this);
>+    addMessageListener("Content:SetCssViewportSize", this);

is "SetCacheViewport" analogous to "SetCssViewportSize"? It might be easier to see the corelation if you drop "Cache" and use "Display"

"SetCacheViewport" -> "SetDisplayViewport"  ?

>+      case "MozScrolledAreaChanged": {
>+        let doc = aEvent.originalTarget;
>+        let win = doc.defaultView;
>+        // XXX need to make some things in Util as its own module!
>+        let scrollOffset = Util.getScrollOffset(win);
>+        if (win.parent != win) // We are only interested in root scroll pane changes
>+          return;

We typically use "content" like this:

if (content != doc.defaultView)

then remove the local "win" since you don't seem to use if anywhere else

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml

>+            case "MozScrolledAreaChanged":
>+              this._widthInCSSPx = aMessage.json.width;
>+              this._heightInCSSPx = aMessage.json.height;

widthInCSSPx and heightInCSSPx seem to be the document width and height. If so, let's call it that: _documentWidth and _documentHeight. Users of <browser> would like to use those and we have other contentDocumentXxx properties.

>+      <!-- Dimensions of content window -->
>+      <property name="viewportWidthInCSSPx"
>+                onget="return this._viewportWidthInCSSPx;"
>+                readonly="true"/>
>+      <property name="viewportHeightInCSSPx"
>+                onget="return this._viewportHeightInCSSPx;"
>+                readonly="true"/>

contentWindowWidth and contentWindowHeight (change the _name too?)

>+
>+      <!-- Dimensions of content document -->
>+      <field name="_widthInCSSPx">0</field>
>+      <field name="_heightInCSSPx">0</field>
>+      <property name="_widthInDevicePx"
>+                onget="return this._widthInCSSPx * this._zoomLevel;"
>+                readonly="true"/>
>+      <property name="_heightInDevicePx"
>+                onget="return this._heightInCSSPx * this._zoomLevel;"
>+                readonly="true"/>

contentDocumentWidth and contentDocumentHeight

>+      <!-- This determines what percentage of cached pixels are not yet visible before the cache
>+           is refreshed. For instance, if we recached at 50% and there are originally a total of
>+           400px offscreen, we'd refresh once 200 of those pixels have been scrolled into
>+           view. -->
>+      <field name="_recacheRatio">0</field>

recacheRatio -> cacheRatio ? cache is cache

>+
>+      <!-- The cache viewport is what parts of content is cached in the parent process for
>+           fast scrolling. This syncs that up with the current projection viewport. -->
>+      <method name="_updateCacheViewport">

Maybe my comment above about SetCacheViewport -> SetDisplayViewport is not a good one. "Cache" is just so ambiguous

>+        <body>
>+          <![CDATA[
>+            let cacheX = this._pendingThresholdX / this._recacheRatio;
>+            let cacheY = this._pendingThresholdY / this._recacheRatio;
>+            let bcr = this.getBoundingClientRect();
>+
>+            let frameLoader = this._frameLoader;
>+            this.messageManager.sendAsyncMessage("Content:SetCacheViewport", {
>+              x: (frameLoader.viewportScrollX + bcr.width  / 2 - cacheX) / this._zoomLevel,
>+              y: (frameLoader.viewportScrollY + bcr.height / 2 - cacheY) / this._zoomLevel,
>+              w: (cacheX * 2) / this._zoomLevel,
>+              h: (cacheY * 2) / this._zoomLevel,
>+              zoomLevel: this._zoomLevel
>+            });
>+
>+            this._pendingPixelsX = 0;
>+            this._pendingPixelsY = 0;
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <!-- Synchronize the CSS viewport with the projection viewport. -->
>+      <method name="_updateCSSViewport">
>+        <body>
>+          <![CDATA[
>+            let frameLoader = this._frameLoader;
>+            this.messageManager.sendAsyncMessage("Content:ScrollTo", {
>+              x: frameLoader.viewportScrollX / this._zoomLevel,
>+              y: frameLoader.viewportScrollY / this._zoomLevel
>+            });

If viewport ~ contentWindow then maybe "_updateCSSViewport" -> "_updateWindowPosition" ? Seem like you are updating the scroll position

>+      <!-- Sets the scale of CSS pixels to device pixels. Does not affect page layout. -->
>+      <method name="setScale">

Echoing Matt's "scale" and "zoom" issue. Maybe "scale" is a better term for the long run. Less ambigous with the textZoom and fullZoom used in Fx

>+      <!-- Sets size of CSS viewport, which affects how page is layout. -->
>+      <method name="setCssViewportSize">

"setWindowSize" ?

>+      <!-- Scroll viewport by (x, y) device pixels. -->
>+      <method name="scrollBy">

>+      <!-- Scroll viewport to (x, y) offset of document in device pixels. -->
>+      <method name="scrollTo">

>+      <!-- Get position of viewport in device pixels. -->
>+      <method name="getPosition">

You mention "device pixels" in the comment - good, good

>       <constructor>

>+          let prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                              .getService(Components.interfaces.nsIPrefService)
>+                                              .QueryInterface(Components.interfaces.nsIPrefBranch2);

Can we use Services.prefs here?

>+          this._recacheRatio = Math.max(.01, Math.min(1, prefService.getIntPref("toolkit.browser.recacheRatio") / 100));

I still like _cacheRatio and "toolkit.browser.cacheRatio"

>+      <method name="scrollBy">

>+      <method name="scrollTo">

>+      <method name="getPosition">

Add comment about "device pixels"

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

> var TapHighlightHelper = {
Just saying again, I'd love to see this handled via CSS now

>   /** Zoom and move viewport so that element is legible and touchable. */
>   _zoom: function _formHelperZoom(aElementRect, aCaretRect) {
>-      let zoomLevel = bv.getZoomLevel();
>-      let enableZoom = bv.allowZoom && Services.prefs.getBoolPref("formhelper.autozoom");
>+      let zoomLevel = getBrowser().scale;
>+      let enableZoom = Browser.selectedTab.allowZoom && Services.prefs.getBoolPref("formhelper.autozoom");

Hmm, makes me want to see .allowZoom on <browser> binding

>+      let scrollX = {}, scrollY = {};
>+      getBrowser().getPosition(scrollX, scrollY);
>+      let vis = new Rect(scrollX.value, scrollY.value, window.innerWidth, window.innerHeight);
>+      x *= getBrowser().scale;
>+      y *= getBrowser().scale;

Matt already pointed out the need for a local here

>+      if (enableZoom && getBrowser().scale != zoomLevel) {
>+        // don't use browser functions they are bogus for this case
>+        let zoomRatio = zoomLevel / getBrowser().scale;

Same

>+        getBrowser().scrollBy(x - vis.x, y - vis.y);

Same

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>+    let needOverride = Util.needHomepageOverride();
>+    if (needOverride == "new profile")
>+      this.initNewProfile();
>+
>+    let container = document.getElementById("browsers");
>+    // XXX change
>+
>+    /* handles dispatching clicks on browser into clicks in content or zooms */
>+    let inputHandlerOverlay = document.getElementById("inputhandler-overlay");
>+    inputHandlerOverlay.customClicker = new ContentCustomClicker();
>+    inputHandlerOverlay.customKeySender = new ContentCustomKeySender();
>+    inputHandlerOverlay.customDragger = new Browser.MainDragger();
>+
>+    // Warning, total hack ahead. All of the real-browser related scrolling code
>+    // lies in a pretend scrollbox here. Let's not land this as-is. Maybe it's time
>+    // to redo all the dragging code.
>+    this.contentScrollbox = container;
>+    this.contentScrollboxScroller = {
>+      scrollBy: function(x, y) {
>+        getBrowser().scrollBy(x, y);
>+      },
>+
>+      scrollTo: function(x, y) {
>+        getBrowser().scrollTo(x, y);
>+      },
>+
>+      getPosition: function(scrollX, scrollY) {
>+        getBrowser().getPosition(scrollX, scrollY);
>+      }
>+    };

Remove. Bad merge

>-    // If this is an intial window launch the commandline handler passes us the default
>-    // page as an argument
>-    let defaultURL = this.getHomePage();
>-    if (window.arguments && window.arguments[0])
>-      defaultURL = window.arguments[0];
>-
>-    this.addTab(defaultURL, true);
>+    // Command line arguments/initial homepage
>+    let whereURI = this.getHomePage();
>+    if (needOverride == "new profile")
>+        whereURI = "about:firstrun";
>+
>+    // If this is an intial window launch (was a nsICommandLine passed via window params)
>+    // we execute some logic to load the initial launch page
>+    if (window.arguments && window.arguments[0]) {
>+      if (window.arguments[0] instanceof Ci.nsICommandLine) {
>+        try {
>+          var cmdLine = window.arguments[0];
>+
>+          // Check for and use a single commandline parameter
>+          if (cmdLine.length == 1) {
>+            // Assume the first arg is a URI if it is not a flag
>+            var uri = cmdLine.getArgument(0);
>+            if (uri != "" && uri[0] != '-') {
>+              whereURI = cmdLine.resolveURI(uri);
>+              if (whereURI)
>+                whereURI = whereURI.spec;
>+            }
>+          }
>+
>+          // Check for the "url" flag
>+          var uriFlag = cmdLine.handleFlagWithParam("url", false);
>+          if (uriFlag) {
>+            whereURI = cmdLine.resolveURI(uriFlag);
>+            if (whereURI)
>+              whereURI = whereURI.spec;
>+          }
>+        } catch (e) {}
>+      }
>+      else {
>+        // This window could have been opened by nsIBrowserDOMWindow.openURI
>+        whereURI = window.arguments[0];
>+      }
>+    } 
>+
>+    this.addTab(whereURI, true);

Revert the changes. Bad merge

>+  initNewProfile: function initNewProfile() {
>+  },
>+

Remove. Bad merge

>   _getZoomRectForPoint: function _getZoomRectForPoint(x, y, zoomLevel) {

>+    x = x * getBrowser().scale;
>+    y = y * getBrowser().scale;

>+    let zoomRatio = zoomLevel / getBrowser().scale;

>+    return result.translateInside(new Rect(0, 0, getBrowser()._widthInDevicePx, getBrowser()._heightInDevicePx));

Local for getBrowser()

>   setVisibleRect: function setVisibleRect(rect) {
>+    let zoomLevel = getBrowser().scale * zoomRatio;

>+    getBrowser().setScale(this.selectedTab.clampZoomLevel(zoomLevel));
>+    getBrowser().scrollTo(scrollX, scrollY);

Local for getBrowser()

>   /** Return offset that pans controls away from screen. Updates doffset with leftovers. */
>   _panControlsAwayOffset: function(doffset) {
>     let height = document.getElementById("tile-stack").getBoundingClientRect().height;
>+    // XXX change
>     rect = Rect.fromRect(Browser.contentScrollbox.getBoundingClientRect()).map(Math.round);
>     if (doffset.y < 0 && rect.bottom < height)
>       y = Math.max(doffset.y, rect.bottom - height);
>     if (doffset.y > 0 && rect.top > 0)
>       y = Math.min(doffset.y, rect.top);

"// XXX Change" -> To what? Is a bug filed?

>   updateViewportMetadata: function updateViewportMetadata(metaData) {

>-    if (metaData.autoSize) {
>-      if (metaData.defaultZoom == 1.0) {
>-        browser.classList.add("window-width");
>-        browser.classList.add("window-height");
>-      } else {
>-        browser.classList.add("viewport-width");
>-        browser.classList.add("viewport-height");
>-      }

Do we need to replicate the "window-*" class stuff somewhere else. I saw we killed the "viewport-*" classes, but what about "window-*"

>-      browser.style.width = viewportW + "px";
>-      browser.style.height = viewportH + "px";
>+      browser.setCssViewportSize(viewportW, viewportH);

This is an example of where a more understandable API name would be helpful. I think "setWindowSize", or even "setViewportSize" would be better. We don't need the "CSS" part

>     // Local XUL documents are not firing MozScrolledAreaChanged
>-    let contentDocument = browser.contentDocument;
>+    /*let contentDocument = browser.contentDocument;
>     if (contentDocument && contentDocument instanceof XULDocument) {
>       let width = contentDocument.documentElement.scrollWidth;
>       let height = contentDocument.documentElement.scrollHeight;
>       BrowserView.Util.ensureMozScrolledAreaEvent(browser, width, height);
>-    }
>+    } */

We need to make sure the XUL docs are painting OK, if they are let's just remove this. I assume about:config works fine, so let's remove this

>   startLoading: function startLoading() {
>     if (this._loading) throw "Already Loading!";
> 
>     this._loading = true;
>   },

>   endLoading: function endLoading() {
>     if (!this._loading) throw "Not Loading!";
>     this._loading = false;
>   },

Please file a bug to remove these methods. We shouldn't need them anymore. There is too much other code depending on Tab.isLoading, which uses _loading, so let's do this in a new bug.

>   _createBrowser: function _createBrowser(aURI) {
>+
>+    let self = this;
>+    browser.messageManager.addMessageListener("MozScrolledAreaChanged", function() {
>+      self.updateDefaultZoomLevel();
>+    });

Why do we need this? We already have a listener for "MozScrolledAreaChanged" in the browser binding (you add it). Do we need to call updateDefaultZoomLevel for every "MozScrolledAreaChanged" or only the first? Me thinks updateDefaultZoomLevel should be in <browser>
>+  clampZoomLevel: function clampZoomLevel(zl) {

>   resetZoomLevel: function resetZoomLevel() {

>+  updateDefaultZoomLevel: function updateDefaultZoomLevel() {

>+  isDefaultZoomLevel: function isDefaultZoomLevel() {

>+  getDefaultZoomLevel: function getDefaultZoomLevel() {

>+  getPageZoomLevel: function getPageZoomLevel() {

>+  get allowZoom() {

All of these seem like nice candidates for moving to <browser> binding. File a new bug for that?

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>+              <!-- Content viewport -->
>+                <html:div>
>+                  <html:div id="browsers"/>
>+                  <html:canvas id="content-overlay" style="display: none; position: absolute; z-index: 1000; left: 0; top: 0;"/>
>+                </html:div>
>+                <html:div id="inputhandler-overlay" style="z-index: 1001" tabindex="-1"/>

I guess <html:div> is ok for a container
Tell me more about "inputhandler-overlay" and I have already mention my desire to kill "content-overlay" (file a bug?)

>diff --git a/chrome/content/content.css b/chrome/content/content.css


>diff --git a/chrome/content/content.js b/chrome/content/content.js

I might file a bug to move Coalescer code into another class.

Can we remove isOffscreenBrowser usage as well? in "Browser:Blur" and "Browser:Focus"

 docShell.isOffScreenBrowser = false;

We don't need this anymore


>diff --git a/components/BrowserCLH.js b/components/BrowserCLH.js

Revert all changes in this file. Bad merge

>diff --git a/components/Makefile.in b/components/Makefile.in

Revert changes. Bad merge

Let's see a new patch before landing. And let's discuss Matt and my feedback before landing.
Attachment #473141 - Flags: review?(mark.finkle) → review+
(In reply to comment #35)
> >   mouseDown: function mouseDown(aX, aY) {
> >     // Ensure that the content process has gets an activate event
> >+    let browser = getBrowser();
> >     let fl = browser.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader;
> >+    browser.focus();
> 
> Get rid of "let browser" here.

I'm not sure why I wrote that.  Cancel that comment.
Comment on attachment 473141 [details] [diff] [review]
Latest patch

>+  isDefaultZoomLevel: function isDefaultZoomLevel() {
>+    return getBrowser().scale == this._defaultZoomLevel;
>+  },

replace getBrowser() with this._browser
> Should this be moved to browser.xml?  (The only reason I suggest this is to
> prevent leakage of the "private" _widthInCSSPx property.)

> Can we name these _documentWidth... and _documentHeight... (or maybe the
> shorter _pageWidth/pageHeight...)?  The names didn't make it clear to me
> (width of what?).

Named these properties to contentDocumentWidth/Height and contentWindowWidth/Height.

>>+    // Warning, total hack ahead. All of the real-browser related scrolling code
>>+    // lies in a pretend scrollbox here. Let's not land this as-is. Maybe it's time
>>+    // to redo all the dragging code.
>>+    this.contentScrollbox = container;
>>+    this.contentScrollboxScroller = {
>
> Do we need to fix this before landing?

Yes.

> Since this function now does nothing but call updateViewportSize, let's get
> rid of it.  (The caller can just set tab.metaData and then call
> updateViewportSize, or we can add an optional parameter to
> updateViewportSize.)

I'd rather leave them separate. Sometimes we want to call updateViewportSize without changing metaData, and I think it's more clear that changing the metadata is a different function.

> Can Browser.forceChromeReflow be removed altogether?

No, sometimes we use this so that things don't artifact when they are first shown. For instance, we do this for the URL bar when we float it so that it doesn't flicker when you pan sidebars out.

> Bug filed for this?

No, need to file this.

> Should we favor .zoomTo before .zoomRect? Does it make a difference?

Yes, it makes a difference, but I didn't document it... Ugh...

> Hmm, I need to look at what "inputhanlder-overlay" is all about

So inputhandler-overlay's only purpose is to deflect events off the browser, and when our generated events hit the browser we can tell that they aren't native events.

>>   _pinchStart: function _pinchStart(aEvent) {
>>-    let bv = Browser._browserView;
>>     // start gesture if it's not taking place already, or over a XUL element
>>-    if (this._pinchZoom || (aEvent.target instanceof XULElement) || !bv.allowZoom)
>>+    if (this._pinchZoom || (aEvent.target instanceof XULElement) || !Browser.selectedTab.allowZoom)
>
> You use getBrowser() in lots of other places. Is one way nicer than the other?

Hmm? allowZoom is a property of the tab, not the browser.

> I think all of this element highlight code should be in content.css, handle via
> CSS now 

CSS is still not nearly fast enough when I tap, so I think this hack needs to stay. Also, if we do change this to CSS it would be nice if we could somehow apply changes to the content that are in device px as opposed to CSS px so that border is always the same size when you tap on something.

>>   _pinchUpdate: function _pinchUpdate(aEvent) {
>>-    this._pinchZoomLevel = Browser._browserView.clampZoomLevel(this._pinchZoomLevel);
>>+    this._pinchZoomLevel = Browser.selectedTab.clampZoomLevel(this._pinchZoomLevel);
>
> You use getBrowser() in the function above

Browser is different than tab.

> Seems like all of this zooming and transforming should be in the <browser>
> binding now. I think Matt suggested something similar

I think there is definitely some methods we could add in browser to simplify this stuff, but it should probably be a followup.

> is "SetCacheViewport" analogous to "SetCssViewportSize"? It might be easier to
> see the corelation if you drop "Cache" and use "Display"
> 
> "SetCacheViewport" -> "SetDisplayViewport"  ?

No, cache viewport determines how many pixels are cached in parent process. CSS viewport affects layout. Display viewport is what is visible on the screen.

> recacheRatio -> cacheRatio ? cache is cache

I agree term is bad, but there is already a cache. This regards invalidating the current cache, not referencing the actual cache itself.

> Maybe my comment above about SetCacheViewport -> SetDisplayViewport is not a
> good one. "Cache" is just so ambiguous

Do you have a good word that separates the cache of pixels from the CSS viewport and the display viewport? RenderedViewport?

> Can we use Services.prefs here?

Are we allowed to in browser bindings?

> Hmm, makes me want to see .allowZoom on <browser> binding

OK, but then all metadata stuff should probably go there.

> Do we need to replicate the "window-*" class stuff somewhere else. I saw we
> killed the "viewport-*" classes, but what about "window-*"

I don't think so. I think I cleaned up the code for this a little since we always have to loop through tabs on resize anyways.

> We need to make sure the XUL docs are painting OK, if they are let's just
> remove this. I assume about:config works fine, so let's remove this

The problem is panning I think. Need to file a bug for this.

> Why do we need this? We already have a listener for "MozScrolledAreaChanged" in
> the browser binding (you add it). Do we need to call updateDefaultZoomLevel for
> every "MozScrolledAreaChanged" or only the first? Me thinks
> updateDefaultZoomLevel should be in <browser>

The browser binding knows nothing about default zoom levels. We could change this, but I thought previously you were against it? Anyways, this could be cleaned up a little.
Attachment #474942 - Flags: review?(mbrubeck)
Attachment #474942 - Flags: review?(mark.finkle)
Attachment #473141 - Attachment is obsolete: true
Attachment #474942 - Flags: review?(mbrubeck) → review+
Depends on: 596326
Comment on attachment 474942 [details] [diff] [review]
Review comments addressed

>+pref("toolkit.browser.cachePixelX", 800);
>+pref("toolkit.browser.cachePixelY", 2000);
>+pref("toolkit.browser.recacheRatio", 60);

Still not a fan of the name and I need to better understand what "cache" means in browser.xml now


>   setVisibleRect: function setVisibleRect(rect) {

>+    let zoomRatio = window.innerWidth / rect.width;
>+    let zoomLevel = getBrowser().scale * zoomRatio;

>+    getBrowser().scale = this.selectedTab.clampZoomLevel(zoomLevel);
>+    getBrowser().scrollTo(scrollX, scrollY);

Need a local?
Attachment #474942 - Flags: review?(mark.finkle) → review+
Blocks: 594974
pushed:
http://hg.mozilla.org/mobile-browser/rev/ea70f3df7037
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 596969
Depends on: 596952
Depends on: 597018
Depends on: 597081
verified FIXED on builds:

Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b6pre) Gecko/20100916 Namoroka/4.0b7pre
Fennec/2.0b1pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b6pre) Gecko/20100916
Namoroka/4.0b7pre Fennec/2.0b1pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
Depends on: 597153
Assignee: webapps → mozaakash
Assignee: mozaakash → webapps
Flags: in-litmus? → in-litmus?(mozaakash)
Deleted tile caching subgroups and testcase within it. I don't think there's much else here for manual testcases.
Flags: in-litmus?(mozaakash) → in-litmus-
You need to log in before you can comment on or make changes to this bug.