Last Comment Bug 703141 - screen.width/height and window.innerWidth/Height are incorrect in
: screen.width/height and window.innerWidth/Height are incorrect in
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P3 normal (vote)
: ---
Assigned To: Chris Lord [:cwiiis]
:
Mentors:
Depends on: 709813
Blocks: metaviewport 701830 702952
  Show dependency treegraph
 
Reported: 2011-11-16 16:31 PST by Matt Brubeck (:mbrubeck)
Modified: 2013-06-16 10:16 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Part 1 - Introduce viewport/displayport split (81.12 KB, patch)
2011-11-22 09:02 PST, Chris Lord [:cwiiis]
bugmail: review-
Details | Diff | Splinter Review
Part 2 - Add zooming (65.57 KB, patch)
2011-11-22 09:06 PST, Chris Lord [:cwiiis]
bugmail: review-
Details | Diff | Splinter Review
Part 3 - Fix zoom origin (6.12 KB, patch)
2011-11-22 09:07 PST, Chris Lord [:cwiiis]
pwalton: review+
Details | Diff | Splinter Review
Part 4 - Clamp to pixel values when rendering (3.91 KB, patch)
2011-11-22 09:08 PST, Chris Lord [:cwiiis]
pwalton: review+
Details | Diff | Splinter Review
Part 5 - Fix plugin positioning (10.19 KB, patch)
2011-11-22 09:10 PST, Chris Lord [:cwiiis]
snorp: review+
Details | Diff | Splinter Review
Part 6 - Fix redraw event flooding (11.72 KB, patch)
2011-11-22 09:12 PST, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Part 7 (?) - Fix spurious relayouting (2.17 KB, patch)
2011-11-22 09:14 PST, Chris Lord [:cwiiis]
bugmail: review-
Details | Diff | Splinter Review
Part 1 - Introduce viewport/displayport split (80.69 KB, patch)
2011-11-23 03:53 PST, Chris Lord [:cwiiis]
bugmail: review+
Details | Diff | Splinter Review
Part 2/3 - Add zooming (65.71 KB, patch)
2011-11-23 03:54 PST, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Part 2/3 - Add zooming (70.61 KB, patch)
2011-11-23 07:14 PST, Chris Lord [:cwiiis]
bugmail: review+
pwalton: review+
Details | Diff | Splinter Review
Part 5 - Fix plugin positioning (12.24 KB, patch)
2011-11-23 07:15 PST, Chris Lord [:cwiiis]
bugmail: review+
pwalton: review+
Details | Diff | Splinter Review
Part 6 - Fix redraw event flooding (11.83 KB, patch)
2011-11-23 07:16 PST, Chris Lord [:cwiiis]
bugmail: review+
Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2011-11-16 16:31:04 PST
In native Fennec, Gecko does not know the correct screen size.  Instead, screen.width always returns 1024.

The real screen size is needed to implement <meta name="viewport" content="width=device-width, height=device-height"> for bug 694901.  This bug also breaks CSS media queries like "min-device-width: 1000px".
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-17 10:05:01 PST
How did we fix this for XUL Fennec?
Comment 2 Chris Lord [:cwiiis] 2011-11-17 10:09:11 PST
This is fixed in the correct-display-port patch in pcwalton's pan-zoom queue - we're hoping to land it next week, pending ironing a few issues out.
Comment 3 Wesley Johnston (:wesj) 2011-11-17 10:13:57 PST
No idea about what pcwalton is doing, but in XUL Fennec we set the CSSViewport of the page (I think?), which sets a boolean variable in presShell, which we check when the user asks for the window size:

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#3692
Comment 4 Chris Lord [:cwiiis] 2011-11-22 09:02:43 PST
Created attachment 576176 [details] [diff] [review]
Part 1 - Introduce viewport/displayport split

This patch refactors the code to make some of the value names and ownership
clearer, and to add the idea of a 'viewport' within a 'displayport'. The
displayport is the area of the page which is visible to the underlying buffer
and the viewport is the area of the page which is visible through the
application window.

This breaks zooming and has issues with checkerboarding.
Comment 5 Chris Lord [:cwiiis] 2011-11-22 09:06:13 PST
Created attachment 576177 [details] [diff] [review]
Part 2 - Add zooming

This patch reinstates pinch-zooming and adds CSS re-scaling so that after
zooming, the page is rendered at the scaled resolution and you get clear text.
Comment 6 Chris Lord [:cwiiis] 2011-11-22 09:07:18 PST
Created attachment 576178 [details] [diff] [review]
Part 3 - Fix zoom origin

This fixes the zoom always appearing to happen from the top-left instead of the gesture's focal point.
Comment 7 Chris Lord [:cwiiis] 2011-11-22 09:08:52 PST
Created attachment 576179 [details] [diff] [review]
Part 4 - Clamp to pixel values when rendering

To stop bad-looking text/fuzzy images, try to draw aligned to the pixel grid.
This doesn't always seem to work when zoomed in, but not sure how important that is (the browser area is redrawn after panning stops).
Comment 8 Chris Lord [:cwiiis] 2011-11-22 09:10:37 PST
Created attachment 576181 [details] [diff] [review]
Part 5 - Fix plugin positioning

This fixes plugin positioning, which is broken by the first patch. Note, that it isn't perfect, as plugin reposition calls aren't synchronised with Gecko draw calls, but I'm not sure it ever was? We can improve this later.
Comment 9 Chris Lord [:cwiiis] 2011-11-22 09:12:34 PST
Created attachment 576184 [details] [diff] [review]
Part 6 - Fix redraw event flooding

Earlier patches mistakenly removed the redraw hint. This restores it, and
alters its behaviour to work correctly with regards to the viewport. This
should help mitigate some checker-boarding and performance issues when panning
and zooming.
Comment 10 Chris Lord [:cwiiis] 2011-11-22 09:14:37 PST
Created attachment 576186 [details] [diff] [review]
Part 7 (?) - Fix spurious relayouting

This works around bug #524925, by not changing the viewport offset around the edges of the page. This should reduce relayouting, which in turn reduces redrawing, which should mean Gecko can draw faster and we get less checker-boarding.

I'd rather not apply this patch though, to be honest. Bug #524925 is being worked on and I don't think it makes a significant enough difference.
Comment 11 Chris Lord [:cwiiis] 2011-11-22 09:16:39 PST
Issues still to address;

- I think when all these patches are applied, there are issues switching between tabs
- The viewportExcess isn't zero'd when loading new content, so when zoomed, sometimes new pages don't appear to be scrolled to 0,0
- There may or may not be issues when zooming with flash content visible - I've not tried this out yet
Comment 12 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-22 11:07:47 PST
Comment on attachment 576176 [details] [diff] [review]
Part 1 - Introduce viewport/displayport split

>-        } catch (Exception e) { 
>-            Log.i(LOGTAG, "handleMessage throws " + e + " for message: " + event);
>+        } catch (Exception e) {
>+            Log.e(LOGTAG, "handleMessage throws " + e + " for message: " + event);
>+            for (StackTraceElement elem : e.getStackTrace())
>+                Log.e(LOGTAG, elem.toString());

You can do this with Log.e(LOGTAG, "blah", e); instead of having to print out the stack trace elements manually.

>+++ b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
> import org.mozilla.gecko.gfx.LayerRenderer;
> import org.mozilla.gecko.gfx.SingleTileLayer;
>-import org.mozilla.gecko.ui.ViewportController;
>+import org.mozilla.gecko.gfx.ViewportMetrics;

Unncessary import.

>+        if (metrics.widthPixels != mScreenSize.width ||
>+            metrics.heightPixels != mScreenSize.height) {
>+            mScreenSize = new IntSize(metrics.widthPixels, metrics.heightPixels);
>+            Log.i("Gecko", "!! Screen-size changed to " + mScreenSize);

Please follow the log format from a6a429a48e60.

>diff --git a/mobile/android/base/gfx/LayerController.java b/mobile/android/base/gfx/LayerController.java
>-    private RectF mVisibleRect;                 /* The current visible region. */
>-    private IntSize mScreenSize;                /* The screen size of the viewport. */
>-    private IntSize mPageSize;                  /* The current page size. */
>+    private ViewportMetrics mViewportMetrics;   /* The current viewport metrics. */

I would prefer calling this "mUserViewport" or some such to make it clearer that this is the user-visible viewport rather than the gecko viewport.

>diff --git a/mobile/android/base/gfx/LayerRenderer.java b/mobile/android/base/gfx/LayerRenderer.java
>     private Rect getPageRect() {
>         LayerController controller = mView.getController();
>-        float zoomFactor = controller.getZoomFactor();
>-        RectF visibleRect = controller.getVisibleRect();
>+        float zoomFactor = 1.0f//controller.getZoomFactor();

There is a missing semicolon on this line; it doesn't compile. Please make sure that birch compiles after each applied patch.

>+        Rect viewport = controller.getVisibleRect();

This should be getViewport(). There is another call somewhere in this file that should also be getViewport() (it fails to compile).

>diff --git a/mobile/android/base/gfx/PlaceholderLayerClient.java b/mobile/android/base/gfx/PlaceholderLayerClient.java
> public class PlaceholderLayerClient extends LayerClient {
>     private Context mContext;
>-    private IntSize mPageSize;
>+    ViewportMetrics mViewport;

This should be private.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>   set selectedTab(aTab) {
>+    // Inform Java of the changed viewport when changing tabs. We do this
>+    // before switching tabs so that the next redraw will have the correct
>+    // viewport.

This comment seems out of place since there is no code nearby that does this.

r- for the compilation fail. For the other things they can be fixed either in this patch or as a follow-up patch on this bug to avoid rebasing nightmares.
Comment 13 Patrick Walton (:pcwalton) 2011-11-22 12:38:19 PST
Comment on attachment 576179 [details] [diff] [review]
Part 4 - Clamp to pixel values when rendering

Review of attachment 576179 [details] [diff] [review]:
-----------------------------------------------------------------

Is it still accurate that this doesn't work when zoomed in?

::: mobile/android/chrome/content/browser.js
@@ +1051,5 @@
>        this._viewport.offsetY = aViewport.offsetY;
>        this.viewportExcess.y = excessY;
>        transformChanged = true;
>      }
> +    if (Math.abs(aViewport.zoom - this._viewport.zoom) >= 1e-6) {

Please factor this out into something like FloatUtils.fuzzyEquals().
Comment 14 Patrick Walton (:pcwalton) 2011-11-22 12:42:16 PST
Comment on attachment 576178 [details] [diff] [review]
Part 3 - Fix zoom origin

Review of attachment 576178 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/gfx/ViewportMetrics.java
@@ +205,5 @@
> +    /* This will set the zoom factor and re-scale page-size and viewport offset
> +     * accordingly. The given focus will remain at the same point on the screen
> +     * after scaling.
> +     */
> +    public void scaleTo(float zoomFactor, PointF focus) {

I'd call this parameter `newZoomFactor`, because the `zoomFactor / mZoomFactor` line below is confusing.
Comment 15 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-22 14:09:00 PST
Comment on attachment 576177 [details] [diff] [review]
Part 2 - Add zooming

>diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
>-                mTileLayer.setOrigin(mGeckoViewport.getDisplayportOrigin());
>+                mTileLayer.setOrigin(PointUtils.round(mGeckoViewport.getDisplayportOrigin()));
>+                mTileLayer.setZoomFactor(1.0f / mGeckoViewport.getZoomFactor());

I think that in order to have consistent semantics, either this "setZoomFactor" should be renamed to "setDrawScale" or you should pass in the raw zoom factor and then take the reciprocal in Layer.java somewhere. I think the best approach might actually be to only do the reciprocal thing in Layer.java's transform() method, directly in the glScalef call. Failing that, the Layer.java variables should also be renamed to mDrawScale/mNewDrawScale or some such.

(Actually, after reading through the rest of the patch, I'm not even sure there should be a reciprocal here... /me confused. Doesn't the LayerRenderer also apply a scale transform for the root layer in setupPageTransform?).

>-
>+                    controller.getView().post(new Runnable() {
>+                        @Override

There is a post() method on controller that delegates the call to the View, so you could just do controller.post(..) and it would have the same effect.

>diff --git a/mobile/android/base/gfx/LayerRenderer.java b/mobile/android/base/gfx/LayerRenderer.java
>-        Rect pageRect = getPageRect();
>+        Layer rootLayer = controller.getRoot();
>+
>+        /* Update layers */
>+        if (rootLayer != null) rootLayer.update(gl);
>+        mShadowLayer.update(gl);
>+        mCheckerboardLayer.update(gl);
>+        mFPSLayer.update(gl);

Do the scrollbar layers need to be added here as well?

>diff --git a/mobile/android/base/gfx/PlaceholderLayerClient.java b/mobile/android/base/gfx/PlaceholderLayerClient.java
>             bitmap.copyPixelsToBuffer(mBuffer.asIntBuffer());
>+
>+            if (mResetViewport) {
>+                mViewport.setPageSize(new FloatSize(mWidth, mHeight));
>+                if (getLayerController() != null) {
>+                    RectF screenRect = getLayerController().getViewport();
>+                    mViewport.setViewport(screenRect);
>+
>+                    getLayerController().setViewportMetrics(mViewport);
>+                }
>+            }
>+

Should this chunk also set mResetViewport back to false? It probably doesn't matter since these code paths are only executed once but to me it looks wrong this way because it doesn't follow the pattern of a set-and-clear flag.

>+        if (mResetViewport)
>+            mViewport.setViewport(layerController.getViewport());
>         layerController.setViewportMetrics(mViewport);

Ditto here.

>diff --git a/mobile/android/base/gfx/ViewportMetrics.java b/mobile/android/base/gfx/ViewportMetrics.java
>     public ViewportMetrics(JSONObject json) {
>         try {
>-            int x = json.getInt("x");
[snip]
>+            mViewportOffset = new PointF(offsetX, offsetY);
>+            mZoomFactor = zoom;
>         } catch (JSONException e) {
>             throw new RuntimeException(e);
>         }

Throwing a RuntimeException here is a bad idea. There are places like in endDrawing that call this constructor and catch a JSONException. I'd prefer this constructor always throw a JSONException, and then whoever is calling this constructor can catch it and throw a RuntimeException if necessary. (I realize this code was already there and it might need to be fixed in a different patch - just pointing it out so it's recorded before I forget about it.)

>diff --git a/mobile/android/base/ui/PanZoomController.java b/mobile/android/base/ui/PanZoomController.java
>     @Override
>     public boolean onScale(ScaleGestureDetector detector) {
>-        /*
>-        mState = PanZoomState.PINCHING;
[snip]
>+        Log.i("Fennec", "New zoom: " + newZoomFactor);
>+
>         return true;
>     }
> 
>     @Override
>     public boolean onScaleBegin(ScaleGestureDetector detector) {
>-        /*
>-        mState = PanZoomState.PINCHING;
[snip]
>+        Log.i("Fennec", "Old zoom: " + mController.getZoomFactor());
> 
>         GeckoApp.mAppContext.hidePluginViews();
>-        */
>+
>         return true;
>     }

The changes here seem like they will change the focal point of zooming, but I could be wrong. If there is no visible change in pinch-zoom behaviour then this is fine. However, the Log.i() lines should be changed to use LOGTAG as I mentioned in the previous review.
Comment 16 Chris Lord [:cwiiis] 2011-11-23 03:31:52 PST
(In reply to Kartikaya Gupta (:kats) from comment #12)
> Comment on attachment 576176 [details] [diff] [review] [diff] [details] [review]
> Part 1 - Introduce viewport/displayport split
> 
> >-        } catch (Exception e) { 
> >-            Log.i(LOGTAG, "handleMessage throws " + e + " for message: " + event);
> >+        } catch (Exception e) {
> >+            Log.e(LOGTAG, "handleMessage throws " + e + " for message: " + event);
> >+            for (StackTraceElement elem : e.getStackTrace())
> >+                Log.e(LOGTAG, elem.toString());
> 
> You can do this with Log.e(LOGTAG, "blah", e); instead of having to print
> out the stack trace elements manually.

Done

> >+++ b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
> > import org.mozilla.gecko.gfx.LayerRenderer;
> > import org.mozilla.gecko.gfx.SingleTileLayer;
> >-import org.mozilla.gecko.ui.ViewportController;
> >+import org.mozilla.gecko.gfx.ViewportMetrics;
> 
> Unncessary import.

Removed

> >+        if (metrics.widthPixels != mScreenSize.width ||
> >+            metrics.heightPixels != mScreenSize.height) {
> >+            mScreenSize = new IntSize(metrics.widthPixels, metrics.heightPixels);
> >+            Log.i("Gecko", "!! Screen-size changed to " + mScreenSize);
> 
> Please follow the log format from a6a429a48e60.

Done

> >diff --git a/mobile/android/base/gfx/LayerController.java b/mobile/android/base/gfx/LayerController.java
> >-    private RectF mVisibleRect;                 /* The current visible region. */
> >-    private IntSize mScreenSize;                /* The screen size of the viewport. */
> >-    private IntSize mPageSize;                  /* The current page size. */
> >+    private ViewportMetrics mViewportMetrics;   /* The current viewport metrics. */
> 
> I would prefer calling this "mUserViewport" or some such to make it clearer
> that this is the user-visible viewport rather than the gecko viewport.

Not done - LayerController has no knowledge of Gecko's separate viewport. This is very intentional, it shouldn't have to worry about differing coordinates or zoom factors (the one exception is in the aboutToCheckerboard code, where it uses the tile's origin, but this isn't so bad).

I'd like to keep the knowledge of the idea of any differing viewport strictly to GeckoSoftwareLayerClient - I don't think we want these details creeping out.

> >diff --git a/mobile/android/base/gfx/LayerRenderer.java b/mobile/android/base/gfx/LayerRenderer.java
> >     private Rect getPageRect() {
> >         LayerController controller = mView.getController();
> >-        float zoomFactor = controller.getZoomFactor();
> >-        RectF visibleRect = controller.getVisibleRect();
> >+        float zoomFactor = 1.0f//controller.getZoomFactor();
> 
> There is a missing semicolon on this line; it doesn't compile. Please make
> sure that birch compiles after each applied patch.
> 
> >+        Rect viewport = controller.getVisibleRect();
> 
> This should be getViewport(). There is another call somewhere in this file
> that should also be getViewport() (it fails to compile).

Both fixed, it compiles now (sorry!)

> >diff --git a/mobile/android/base/gfx/PlaceholderLayerClient.java b/mobile/android/base/gfx/PlaceholderLayerClient.java
> > public class PlaceholderLayerClient extends LayerClient {
> >     private Context mContext;
> >-    private IntSize mPageSize;
> >+    ViewportMetrics mViewport;
> 
> This should be private.

Done

> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> >   set selectedTab(aTab) {
> >+    // Inform Java of the changed viewport when changing tabs. We do this
> >+    // before switching tabs so that the next redraw will have the correct
> >+    // viewport.
> 
> This comment seems out of place since there is no code nearby that does this.

Removed, it's legacy from an older version of the patch.
Comment 17 Chris Lord [:cwiiis] 2011-11-23 03:43:20 PST
(In reply to Kartikaya Gupta (:kats) from comment #15)
> Comment on attachment 576177 [details] [diff] [review] [diff] [details] [review]
> Part 2 - Add zooming
> 
> >diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
> >-                mTileLayer.setOrigin(mGeckoViewport.getDisplayportOrigin());
> >+                mTileLayer.setOrigin(PointUtils.round(mGeckoViewport.getDisplayportOrigin()));
> >+                mTileLayer.setZoomFactor(1.0f / mGeckoViewport.getZoomFactor());
> 
> I think that in order to have consistent semantics, either this
> "setZoomFactor" should be renamed to "setDrawScale" or you should pass in
> the raw zoom factor and then take the reciprocal in Layer.java somewhere. I
> think the best approach might actually be to only do the reciprocal thing in
> Layer.java's transform() method, directly in the glScalef call. Failing
> that, the Layer.java variables should also be renamed to
> mDrawScale/mNewDrawScale or some such.
> 
> (Actually, after reading through the rest of the patch, I'm not even sure
> there should be a reciprocal here... /me confused. Doesn't the LayerRenderer
> also apply a scale transform for the root layer in setupPageTransform?).

The idea is that the LayerRenderer applies the transform for the viewport, and GeckoSoftwareLayerClient renders at a resolution that would match this and sets the reciprocal to un-zoom. I agree though, this is confusing by using 'zoom' twice to mean two different things.

I've changed TileLayer to have set/getResolution instead of ZoomFactor, and I've added some documentation above SetResolution. This also matches how the terms are used elsewhere in Gecko. The reciprocal is now taken in TileLayer.transform, where it makes more sense.

> >-
> >+                    controller.getView().post(new Runnable() {
> >+                        @Override
> 
> There is a post() method on controller that delegates the call to the View,
> so you could just do controller.post(..) and it would have the same effect.

Done

> >diff --git a/mobile/android/base/gfx/LayerRenderer.java b/mobile/android/base/gfx/LayerRenderer.java
> >-        Rect pageRect = getPageRect();
> >+        Layer rootLayer = controller.getRoot();
> >+
> >+        /* Update layers */
> >+        if (rootLayer != null) rootLayer.update(gl);
> >+        mShadowLayer.update(gl);
> >+        mCheckerboardLayer.update(gl);
> >+        mFPSLayer.update(gl);
> 
> Do the scrollbar layers need to be added here as well?

Probably :) Done.

> >diff --git a/mobile/android/base/gfx/PlaceholderLayerClient.java b/mobile/android/base/gfx/PlaceholderLayerClient.java
> >             bitmap.copyPixelsToBuffer(mBuffer.asIntBuffer());
> >+
> >+            if (mResetViewport) {
> >+                mViewport.setPageSize(new FloatSize(mWidth, mHeight));
> >+                if (getLayerController() != null) {
> >+                    RectF screenRect = getLayerController().getViewport();
> >+                    mViewport.setViewport(screenRect);
> >+
> >+                    getLayerController().setViewportMetrics(mViewport);
> >+                }
> >+            }
> >+
> 
> Should this chunk also set mResetViewport back to false? It probably doesn't
> matter since these code paths are only executed once but to me it looks
> wrong this way because it doesn't follow the pattern of a set-and-clear flag.
>
> >+        if (mResetViewport)
> >+            mViewport.setViewport(layerController.getViewport());
> >         layerController.setViewportMetrics(mViewport);
> 
> Ditto here.
> 

mResetViewport is set when PlaceholderLayerClient doesn't know the viewport metrics of the screenshot (this shouldn't happen). In this case, it gets metrics from the layer controller when set, but overrides the page size when the image is loaded.

This is confusingly named, so I've renamed it to mViewportUnknown to better reflect what it represents, and I've made the code clearer, making sure it really only overrides page-size.

> >diff --git a/mobile/android/base/gfx/ViewportMetrics.java b/mobile/android/base/gfx/ViewportMetrics.java
> >     public ViewportMetrics(JSONObject json) {
> >         try {
> >-            int x = json.getInt("x");
> [snip]
> >+            mViewportOffset = new PointF(offsetX, offsetY);
> >+            mZoomFactor = zoom;
> >         } catch (JSONException e) {
> >             throw new RuntimeException(e);
> >         }
> 
> Throwing a RuntimeException here is a bad idea. There are places like in
> endDrawing that call this constructor and catch a JSONException. I'd prefer
> this constructor always throw a JSONException, and then whoever is calling
> this constructor can catch it and throw a RuntimeException if necessary. (I
> realize this code was already there and it might need to be fixed in a
> different patch - just pointing it out so it's recorded before I forget
> about it.)

Agreed, fixed in patch 1 and rebased on.

> >diff --git a/mobile/android/base/ui/PanZoomController.java b/mobile/android/base/ui/PanZoomController.java
> >     @Override
> >     public boolean onScale(ScaleGestureDetector detector) {
> >-        /*
> >-        mState = PanZoomState.PINCHING;
> [snip]
> >+        Log.i("Fennec", "New zoom: " + newZoomFactor);
> >+
> >         return true;
> >     }
> > 
> >     @Override
> >     public boolean onScaleBegin(ScaleGestureDetector detector) {
> >-        /*
> >-        mState = PanZoomState.PINCHING;
> [snip]
> >+        Log.i("Fennec", "Old zoom: " + mController.getZoomFactor());
> > 
> >         GeckoApp.mAppContext.hidePluginViews();
> >-        */
> >+
> >         return true;
> >     }
> 
> The changes here seem like they will change the focal point of zooming, but
> I could be wrong. If there is no visible change in pinch-zoom behaviour then
> this is fine. However, the Log.i() lines should be changed to use LOGTAG as
> I mentioned in the previous review.

I've squashed patches 2 and 3, as they don't make sense without each other. Zoom behaviour is correct with this applied and the focal point appears to be correct.
Comment 18 Chris Lord [:cwiiis] 2011-11-23 03:53:03 PST
Created attachment 576445 [details] [diff] [review]
Part 1 - Introduce viewport/displayport split

Address review comments, rebase.
Comment 19 Chris Lord [:cwiiis] 2011-11-23 03:54:21 PST
Created attachment 576446 [details] [diff] [review]
Part 2/3 - Add zooming

Squash parts 2 and 3, rebase, address review comments.
Comment 20 Chris Lord [:cwiiis] 2011-11-23 03:57:38 PST
hmm, I missed the float comparison refactoring comments, will address...
Comment 21 Chris Lord [:cwiiis] 2011-11-23 07:14:17 PST
Created attachment 576490 [details] [diff] [review]
Part 2/3 - Add zooming

This addresses both kats' and pcwalton's comments.
Comment 22 Chris Lord [:cwiiis] 2011-11-23 07:15:40 PST
Created attachment 576491 [details] [diff] [review]
Part 5 - Fix plugin positioning

The last plugin positioning patch didn't get things write when the zoom level wasn't 1.0 - rebased and fixed.
Comment 23 Chris Lord [:cwiiis] 2011-11-23 07:16:36 PST
Created attachment 576492 [details] [diff] [review]
Part 6 - Fix redraw event flooding

Rebased.
Comment 24 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-23 07:44:39 PST
Comment on attachment 576490 [details] [diff] [review]
Part 2/3 - Add zooming

>diff --git a/mobile/android/base/gfx/PlaceholderLayerClient.java b/mobile/android/base/gfx/PlaceholderLayerClient.java
>+                mViewportUnknown = false;
>+            } catch (JSONException e) {
>+                Log.e("PlaceholderLayerClient", "Error parsing saved viewport!");

Another log line that needs to use LOGTAG. Sorry, I missed this on my first pass through.

r+ with the above change.
Comment 25 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-23 08:06:09 PST
Comment on attachment 576491 [details] [diff] [review]
Part 5 - Fix plugin positioning

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>+class PluginLayoutParams extends AbsoluteLayout.LayoutParams
>+{
>+    private static final int MAX_DIMENSION = 2048;
>+    private static final String LOGTAG = "GeckoApp.PluginLayoutParams";
> 
>-        public int originalX;
>-        public int originalY;
>-        public int originalWidth;
>-        public int originalHeight;
>+    public int originalX;
>+    public int originalY;
>+    public int originalWidth;
>+    public int originalHeight;
>+    public float originalResolution;

These should be private and renamed with "m" prefix (e.g. mOriginalX). (Again, this wasn't introduced in your patch but it should be fixed at some point, possibly in a follow-up).
Comment 26 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-23 08:31:59 PST
Comment on attachment 576492 [details] [diff] [review]
Part 6 - Fix redraw event flooding

>diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
>             if (mViewportRedrawTimer != null)
>                 return;
> 
>             mViewportRedrawTimer = new Timer();
>             mViewportRedrawTimer.schedule(new TimerTask() {
>                 @Override
>                 public void run() {
>                     /* We jump back to the UI thread to avoid possible races here. */
>                     getLayerController().getView().post(new Runnable() {
>                         @Override
>                         public void run() {
>                             mViewportRedrawTimer = null;
>                             adjustViewportWithThrottling();
>                         }
>                     });
>                 }
>-            }, MIN_VIEWPORT_CHANGE_DELAY);
>+            }, MIN_VIEWPORT_CHANGE_DELAY - timeDelta);

I noticed in the javadoc that View also has a postDelayed method, which would allow us to eliminate using a Timer/TimerTask here. Calling getLayerController().getView().postDelayed(..., ...) should be equivalent. We'd also have to add a boolean to track if the runnable was queued, to replace the mViewportRedrawTimer == null checks.

>diff --git a/mobile/android/base/gfx/LayerController.java b/mobile/android/base/gfx/LayerController.java
>     // Returns true if a checkerboard is about to be visible.
>     private boolean aboutToCheckerboard() {
>-        RectF adjustedTileRect =
>-            RectUtils.contract(getTileRect(), DANGER_ZONE_X, DANGER_ZONE_Y);
>-        return !adjustedTileRect.contains(new RectF(mViewportMetrics.getViewport()));
>+        // Reduce the size of the displayport by the 'danger zone', to decide
>+        // whether we're about to checkerboard.
>+
>+        // Note, we don't want to reduce the size of edges that are already
>+        // against the page boundaries, as there's nothing to display beyond
>+        // there.
>+        FloatSize pageSize = getPageSize();
>+        RectF adjustedTileRect = getTileRect();
>+
>+        if (adjustedTileRect.left > 0)
>+            adjustedTileRect.left += DANGER_ZONE_X;
>+        if (adjustedTileRect.right < pageSize.width)
>+            adjustedTileRect.right -= DANGER_ZONE_X;
>+
>+        if (adjustedTileRect.top > 0)
>+            adjustedTileRect.top += DANGER_ZONE_Y;
>+        if (adjustedTileRect.bottom < pageSize.height)
>+            adjustedTileRect.bottom -= DANGER_ZONE_Y;

This doubles the size of the danger zone compared to the code that was there before (since RectUtils.contract took off DANGER_ZONE/2 from either side). I don't have a problem with that, but just wanted to make sure that was intentional and/or doesn't degrade performance. I imagine it will lead to less checkerboarding as it pre-draws more frequently. If it is intentional, the commit message should probably mention that as well.
Comment 27 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-23 08:39:47 PST
Comment on attachment 576186 [details] [diff] [review]
Part 7 (?) - Fix spurious relayouting


>diff --git a/mobile/android/base/gfx/ViewportMetrics.java b/mobile/android/base/gfx/ViewportMetrics.java
>--- a/mobile/android/base/gfx/ViewportMetrics.java
>+++ b/mobile/android/base/gfx/ViewportMetrics.java
>@@ -103,30 +103,32 @@ public class ViewportMetrics {
>         Point optimumOffset =
>             new Point((int)Math.round((LayerController.TILE_WIDTH - mViewportRect.width()) / 2),
>                       (int)Math.round((LayerController.TILE_HEIGHT - mViewportRect.height()) / 2));
> 
[snip]
> 
>         return new PointF(optimumOffset);
>     }
> 

If there's a noticeable perf gain in doing this (and I imagine there would be, since reflows are expensive) then I think we should, and try to get bug 524925 fixed faster. However, I'm not sure rounding the coordinates, which is what happens with the left over code, is intentional. If it is, then maybe the return type of the getOptimumViewportOffset method should be changed to a Point to ensure that always happens.
Comment 28 Patrick Walton (:pcwalton) 2011-11-23 09:03:59 PST
(In reply to Kartikaya Gupta (:kats) from comment #27)
> If there's a noticeable perf gain in doing this (and I imagine there would
> be, since reflows are expensive) then I think we should, and try to get bug
> 524925 fixed faster. However, I'm not sure rounding the coordinates, which
> is what happens with the left over code, is intentional. If it is, then
> maybe the return type of the getOptimumViewportOffset method should be
> changed to a Point to ensure that always happens.

I suspect that the issue is actually that Cairo is slow and not that reflows are happening, for what it's worth. I haven't profiled though (can't get oprofile to work), so take this with a grain of salt.
Comment 29 Chris Lord [:cwiiis] 2011-11-23 10:43:34 PST
(In reply to Kartikaya Gupta (:kats) from comment #26)
> Comment on attachment 576492 [details] [diff] [review] [diff] [details] [review]
> Part 6 - Fix redraw event flooding
> 
> >diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
> >             if (mViewportRedrawTimer != null)
> >                 return;
> > 
> >             mViewportRedrawTimer = new Timer();
> >             mViewportRedrawTimer.schedule(new TimerTask() {
> >                 @Override
> >                 public void run() {
> >                     /* We jump back to the UI thread to avoid possible races here. */
> >                     getLayerController().getView().post(new Runnable() {
> >                         @Override
> >                         public void run() {
> >                             mViewportRedrawTimer = null;
> >                             adjustViewportWithThrottling();
> >                         }
> >                     });
> >                 }
> >-            }, MIN_VIEWPORT_CHANGE_DELAY);
> >+            }, MIN_VIEWPORT_CHANGE_DELAY - timeDelta);
> 
> I noticed in the javadoc that View also has a postDelayed method, which
> would allow us to eliminate using a Timer/TimerTask here. Calling
> getLayerController().getView().postDelayed(..., ...) should be equivalent.
> We'd also have to add a boolean to track if the runnable was queued, to
> replace the mViewportRedrawTimer == null checks.

Done

> >diff --git a/mobile/android/base/gfx/LayerController.java b/mobile/android/base/gfx/LayerController.java
> >     // Returns true if a checkerboard is about to be visible.
> >     private boolean aboutToCheckerboard() {
> >-        RectF adjustedTileRect =
> >-            RectUtils.contract(getTileRect(), DANGER_ZONE_X, DANGER_ZONE_Y);
> >-        return !adjustedTileRect.contains(new RectF(mViewportMetrics.getViewport()));
> >+        // Reduce the size of the displayport by the 'danger zone', to decide
> >+        // whether we're about to checkerboard.
> >+
> >+        // Note, we don't want to reduce the size of edges that are already
> >+        // against the page boundaries, as there's nothing to display beyond
> >+        // there.
> >+        FloatSize pageSize = getPageSize();
> >+        RectF adjustedTileRect = getTileRect();
> >+
> >+        if (adjustedTileRect.left > 0)
> >+            adjustedTileRect.left += DANGER_ZONE_X;
> >+        if (adjustedTileRect.right < pageSize.width)
> >+            adjustedTileRect.right -= DANGER_ZONE_X;
> >+
> >+        if (adjustedTileRect.top > 0)
> >+            adjustedTileRect.top += DANGER_ZONE_Y;
> >+        if (adjustedTileRect.bottom < pageSize.height)
> >+            adjustedTileRect.bottom -= DANGER_ZONE_Y;
> 
> This doubles the size of the danger zone compared to the code that was there
> before (since RectUtils.contract took off DANGER_ZONE/2 from either side). I
> don't have a problem with that, but just wanted to make sure that was
> intentional and/or doesn't degrade performance. I imagine it will lead to
> less checkerboarding as it pre-draws more frequently. If it is intentional,
> the commit message should probably mention that as well.

Unintentional. I've corrected it, and simplified/improved how it works.
Comment 30 Chris Lord [:cwiiis] 2011-11-23 10:44:10 PST
(In reply to Kartikaya Gupta (:kats) from comment #25)
> Comment on attachment 576491 [details] [diff] [review] [diff] [details] [review]
> Part 5 - Fix plugin positioning
> 
> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> >+class PluginLayoutParams extends AbsoluteLayout.LayoutParams
> >+{
> >+    private static final int MAX_DIMENSION = 2048;
> >+    private static final String LOGTAG = "GeckoApp.PluginLayoutParams";
> > 
> >-        public int originalX;
> >-        public int originalY;
> >-        public int originalWidth;
> >-        public int originalHeight;
> >+    public int originalX;
> >+    public int originalY;
> >+    public int originalWidth;
> >+    public int originalHeight;
> >+    public float originalResolution;
> 
> These should be private and renamed with "m" prefix (e.g. mOriginalX).
> (Again, this wasn't introduced in your patch but it should be fixed at some
> point, possibly in a follow-up).

Why fix tomorrow what you can fix today? :) Done.
Comment 31 Patrick Walton (:pcwalton) 2011-11-23 11:01:35 PST
Comment on attachment 576491 [details] [diff] [review]
Part 5 - Fix plugin positioning

Review of attachment 576491 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApp.java
@@ +1060,2 @@
>                  if (mGeckoLayout.indexOfChild(view) == -1) {
> +                    lp = PluginLayoutParams.create((int)x, (int)y, (int)w, (int)h, geckoViewport.getZoomFactor());

We don't round?

::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ +179,5 @@
>      public ViewportMetrics getGeckoViewportMetrics() {
>          // Return a copy, as we modify this inside the Gecko thread
> +        if (mGeckoViewport != null)
> +            return new ViewportMetrics(mGeckoViewport);
> +        else

else-after-return

::: mobile/android/base/gfx/LayerController.java
@@ +161,5 @@
>      public void scrollTo(PointF point) {
>          mViewportMetrics.setOrigin(point);
>          notifyLayerClientOfGeometryChange();
>          mPanZoomController.geometryChanged();
> +        GeckoApp.mAppContext.repositionPluginViews(false);

So originally I had envisioned it to be kind of a layering violation for the LayerController to mess with the GeckoApp directly instead of  through the layer client, but I won't r- the patch for this.
Comment 32 Chris Lord [:cwiiis] 2011-11-23 11:07:11 PST
(In reply to Patrick Walton (:pcwalton) from comment #31)
> Comment on attachment 576491 [details] [diff] [review] [diff] [details] [review]
> Part 5 - Fix plugin positioning
> 
> Review of attachment 576491 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoApp.java
> @@ +1060,2 @@
> >                  if (mGeckoLayout.indexOfChild(view) == -1) {
> > +                    lp = PluginLayoutParams.create((int)x, (int)y, (int)w, (int)h, geckoViewport.getZoomFactor());
> 
> We don't round?

I've left this as it was before, if it's a problem I'll let snorp handle it.

> ::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
> @@ +179,5 @@
> >      public ViewportMetrics getGeckoViewportMetrics() {
> >          // Return a copy, as we modify this inside the Gecko thread
> > +        if (mGeckoViewport != null)
> > +            return new ViewportMetrics(mGeckoViewport);
> > +        else
> 
> else-after-return

Removed, thanks.

> ::: mobile/android/base/gfx/LayerController.java
> @@ +161,5 @@
> >      public void scrollTo(PointF point) {
> >          mViewportMetrics.setOrigin(point);
> >          notifyLayerClientOfGeometryChange();
> >          mPanZoomController.geometryChanged();
> > +        GeckoApp.mAppContext.repositionPluginViews(false);
> 
> So originally I had envisioned it to be kind of a layering violation for the
> LayerController to mess with the GeckoApp directly instead of  through the
> layer client, but I won't r- the patch for this.

I agree, and really, this should happen in onDrawFrame, but I kept messing it up when I tried to put it there, so I settled for the quick-and-dirty fix. Let's address this in a later patch, plugin positioning is pretty rough at the moment anyway.
Comment 34 Steffen Wilberg 2011-11-23 13:19:22 PST
Comment on attachment 576492 [details] [diff] [review]
Part 6 - Fix redraw event flooding

>diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
>+                            Log.i(LOGTAG, "Viewport adjusted");
Now that's spammy...
Comment 35 Chris Lord [:cwiiis] 2011-11-24 01:51:50 PST
(In reply to Steffen Wilberg from comment #34)
> Comment on attachment 576492 [details] [diff] [review] [diff] [details] [review]
> Part 6 - Fix redraw event flooding
> 
> >diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
> >+                            Log.i(LOGTAG, "Viewport adjusted");
> Now that's spammy...

Just to note, a follow-up patch has removed this.
Comment 36 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-29 07:07:04 PST
Comment on attachment 576186 [details] [diff] [review]
Part 7 (?) - Fix spurious relayouting

r-'ing since we agreed not to land this patch for now (can be done via a follow-up bug) and I want to get it out of my review queue.
Comment 37 Patrick Walton (:pcwalton) 2012-01-01 12:45:26 PST
Comment on attachment 576490 [details] [diff] [review]
Part 2/3 - Add zooming

Review of attachment 576490 [details] [diff] [review]:
-----------------------------------------------------------------

r+ to get this out of my queue.

Note You need to log in before you can comment on or make changes to this bug.