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".

How did we fix this for XUL Fennec?

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.

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

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.

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.

This fixes the zoom always appearing to happen from the top-left instead of the gesture's focal point.

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).

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.

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.

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.

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 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 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 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 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.

(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.

(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.

Address review comments, rebase.

Squash parts 2 and 3, rebase, address review comments.

hmm, I missed the float comparison refactoring comments, will address...

This addresses both kats' and pcwalton's comments.

The last plugin positioning patch didn't get things write when the zoom level wasn't 1.0 - rebased and fixed.

Rebased.

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 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 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 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.

(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.

(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.

(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 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.

(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.

Pushed parts 1-6 to birch. We can file a follow-up bug for part 7 if necessary. Thanks for the reviews! http://hg.mozilla.org/projects/birch/rev/8fcdb0e7131d http://hg.mozilla.org/projects/birch/rev/8af9955c59d0 http://hg.mozilla.org/projects/birch/rev/6ab52c277bcb http://hg.mozilla.org/projects/birch/rev/1938db9adce4 http://hg.mozilla.org/projects/birch/rev/3f0f2ffed076

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...

(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 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 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.