window.scrollTo() cannot ensure the compatibility when dynamic toolbar is enabled. [Environment] * http://hg.mozilla.org/mozilla-central/rev/768af8d8fad4 [Step to reproduce] # Please open the testcase. This testcase meets following requirements: * Fennec dynamic toolbar is enabled. * the page height is longer than the display height. * Calling window.scrollTo() when dynamic toolbar is shown. (In this testcase, I call it in the "load" event handler.) [Result of testcase] The result of window.scrollTo() is different from when disable dynamic toolbar. I seem this is a site-compatibility bug.

Clean up the testcase.

The only way we could fix this is by offsetting the actual browser when the toolbar comes down instead of overlaying the toolbar on top of the browser (it would still need to work in a similar fashion to how it does now, but fixed margins would be applied to the bottom of the screen instead of the top, we'd have to tell the compositor to offset rendering, and we'd need to reconcile the differing coordinates in Java-land). This would be a major change from how it works currently, but is feasible. I'd estimate it taking a concentrated week, maybe two, at the expense of everything else. I'm not convinced this is much of an issue atm (anchors still work fine), so it's not something I'm going to take on right now unless someone tells me this is a priority.

I've changed my mind on this, with situations like bug 856497 appearing... Should finish this before the end of the week.

This is a total refactor of the dynamic toolbar so that the page is offset instead of overlapped. It's hard to summarise the changes here (and sorry it isn't more patches, but I really had a hard time trying to think of a way not to do this all at once), but the major point is that the GLC is now in total control of the metrics, including the margins, and the browser chrome responds to changes in these metrics; instead of the flaky two-way sync I had in the previous code. This better separation enables a pretty hefty removal of code from BrowserApp, BrowserToolbar, BrowserToolbarLayout and even some simplification in GLC itself :) r?kats for the whole lot, r?nrc for the CompositorParent change.

Comment on attachment 739828 [details] [diff] [review] Refactor dynamic toolbar Review of attachment 739828 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +3566,5 @@ > allowZoom: metadata.allowZoom, > defaultZoom: metadata.defaultZoom || metadata.scaleRatio, > minZoom: metadata.minZoom || 0, > maxZoom: metadata.maxZoom || 0, > + isRTL: metadata.isRTL || false, You need not OR operator. This part will work if it's `isRTL: metadata.isRTL`. @@ +5170,5 @@ > ViewportMetadata.prototype = { > width: null, > height: null, > defaultZoom: null, > minZoom: null, You should add `isRTL` to ViewportMetadata.prototype.

Comment on attachment 739828 [details] [diff] [review] Refactor dynamic toolbar Review of attachment 739828 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the CompositorParent change with the added comment. ::: gfx/layers/ipc/CompositorParent.h @@ +178,5 @@ > virtual void SetFirstPaintViewport(const nsIntPoint& aOffset, float aZoom, const nsIntRect& aPageRect, const gfx::Rect& aCssPageRect); > virtual void SetPageRect(const gfx::Rect& aCssPageRect); > virtual void SyncViewportInfo(const nsIntRect& aDisplayPort, float aDisplayResolution, bool aLayersUpdated, > nsIntPoint& aScrollOffset, float& aScaleX, float& aScaleY, > + gfx::Margin& aFixedLayerMargins, float& aOffsetX, float& aOffsetY); Could you add a comment to SyncViewportInfo saying what it does and what all the args are for? I would prefer to pass the scale and offset as gfx::Points rather than pairs of floats, but given that this looks like a thin wrapper around an Android function, I can understand if you don't want to do that.

Sorry, the way I offset in the last version caused reftest failures, so I've reworked that part of it (so re-requesting r=nrc) Not much has changed Java side, I've removed some more code I forgot to remove the first time and fixed some issues when toggling the feature on/off without restarting.

> ::: gfx/layers/ipc/CompositorParent.h > @@ +178,5 @@ > > virtual void SetFirstPaintViewport(const nsIntPoint& aOffset, float aZoom, const nsIntRect& aPageRect, const gfx::Rect& aCssPageRect); > > virtual void SetPageRect(const gfx::Rect& aCssPageRect); > > virtual void SyncViewportInfo(const nsIntRect& aDisplayPort, float aDisplayResolution, bool aLayersUpdated, > > nsIntPoint& aScrollOffset, float& aScaleX, float& aScaleY, > > + gfx::Margin& aFixedLayerMargins, float& aOffsetX, float& aOffsetY); > > Could you add a comment to SyncViewportInfo saying what it does and what all > the args are for? I would prefer to pass the scale and offset as gfx::Points > rather than pairs of floats, but given that this looks like a thin wrapper > around an Android function, I can understand if you don't want to do that. Will do this - and no reason it can't be a gfx::Point, will do that too.

Comment on attachment 739828 [details] [diff] [review] Refactor dynamic toolbar Review of attachment 739828 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good! I definitely like how a lot of the code got simpler. I'd like to see the comments addressed (specially the extraction of a MarginAnimator helper class) and do another review pass on the updated patch before r+'ing. ::: mobile/android/base/BrowserApp.java @@ +9,5 @@ > import org.mozilla.gecko.db.BrowserDB; > import org.mozilla.gecko.gfx.BitmapUtils; > import org.mozilla.gecko.gfx.ImmutableViewportMetrics; > import org.mozilla.gecko.gfx.LayerView; > +import org.mozilla.gecko.gfx.PanZoomTarget; See my comment in PanZoomTarget; I don't think BrowserApp should be using the PanZoomTarget interface for this. ::: mobile/android/base/BrowserToolbar.java @@ -495,5 @@ > > - private boolean canToolbarHide() { > - // Forbid the toolbar from hiding if hiding the toolbar would cause > - // the page to go into overscroll. > - LayerView layerView = GeckoApp.mAppContext.getLayerView(); You can remove the LayerView import from this file (BrowserToolbar.java) ::: mobile/android/base/TextSelection.java @@ +135,5 @@ > @Override > public void run() { > + mStartHandle.repositionWithViewport(mViewLeft, mViewTop, mViewZoom); > + mMiddleHandle.repositionWithViewport(mViewLeft, mViewTop, mViewZoom); > + mEndHandle.repositionWithViewport(mViewLeft, mViewTop, mViewZoom); Converting context.viewport.left to mViewLeft here is wrong, because this runs on a separate thread. context is final so context.viewport.left will not change, but mViewLeft can. Instead you should pull out a local final variable at the top of this function for "context.viewport.left - context.offset.x" and use that in all of the hunks that you changed in this file. Ditto for the other fields. ::: mobile/android/base/TextSelectionHandle.java @@ +150,5 @@ > > mGeckoPoint = new PointF(left, top); > if (mIsRTL != rtl) { > mIsRTL = rtl; > setImageLevel(mIsRTL ? IMAGE_LEVEL_RTL : IMAGE_LEVEL_LTR); Oh, interesting. File a follow-up to unify the text selection isRtl flag with the one you added. ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +80,5 @@ > + private boolean mMarginsPinned; > + /* The timer that handles showing/hiding margins */ > + private Timer mAnimationTimer; > + /* This interpolator is used for the above mentioned animation */ > + private DecelerateInterpolator mInterpolator; I think it makes more sense to move these new fields and many of the new methods out into a new helper class. For the most part they are not tightly bound to the existing code in terms of using existing private member variables/functions, and pulling it out will make it clearer and less coupled. The stuff I think should be moved are: MS_PER_FRAME, MARGIN_DURATION_ANIMATION, mMaxMargins, mMarginsPinned, mAnimationTimer, mInterpolator, setMaxMargins, animateMargins, showMargins, hideMargins, setMarginsPinned, and large chunks of scrollBy() (I think the mAnimationTimer bits and the !mMarginsPinned block). The new class could be called something like MarginAnimator and would live here in the gfx/ folder. I would probably try to create it in LayerView and pass in a reference to the GLC constructor, but I'm not entirely sure if that will work. Either way I think it makes sense to have a getMarginAnimator() function in LayerView next to the getLayerClient() and use that to call things like showMargins from BrowserApp. @@ +418,4 @@ > > + private void animateMargins(final float left, final float top, final float right, final float bottom, boolean immediately) { > + if (mAnimationTimer != null) { > + mAnimationTimer.cancel(); Also set mAnimationTimer to null here. @@ +443,5 @@ > + float progress = mInterpolator.getInterpolation( > + Math.min(1.0f, (SystemClock.uptimeMillis() - startTime) > + / (float)MARGIN_ANIMATION_DURATION)); > + > + ImmutableViewportMetrics oldMetrics = mViewportMetrics; Almost this entire function needs to be inside a synchronized (GeckoLayerClient.this) block so that other code doesn't come in and twiddle with mViewportMetrics in the middle of all this. @@ +873,5 @@ > + > + float newMarginLeft = oldMetrics.marginLeft; > + float newMarginTop = oldMetrics.marginTop; > + float newMarginRight = oldMetrics.marginRight; > + float newMarginBottom = oldMetrics.marginBottom; Move these inside the !mMarginsPinned block (this goes with my next comment) @@ +916,5 @@ > + .setMargins(newMarginLeft, newMarginTop, newMarginRight, newMarginBottom) > + .offsetViewportBy(dx, dy); > + > + // Set mViewportMetrics manually so the margin changes take. > + mViewportMetrics = newMetrics; Move the setMargins call and the mViewportMetrics direct assignment inside the !mMarginsPinned block. Leave the offsetViewportBy and setViewportMetrics call outside. Note that with the refactoring to extract a MarginAnimator class this means that the margin-related things will be moved but the offsetViewportBy call will stay here. ::: mobile/android/base/gfx/ImmutableViewportMetrics.java @@ +48,2 @@ > zoomFactor = 1.0f; > + isRTL = false; I'm not entirely convinced that isRTL needs to be stored in this class. Is it possible to just make it a parameter to the two functions that use it? I'm not sure if all the call sites have the RTL flag handy but it might be cleaner to do it that way. @@ +181,5 @@ > + FloatUtils.interpolate(marginTop, to.marginTop, t), > + FloatUtils.interpolate(marginRight, to.marginRight, t), > + FloatUtils.interpolate(marginBottom, to.marginBottom, t), > + FloatUtils.interpolate(zoomFactor, to.zoomFactor, t), > + t >= 0.5 ? to.isRTL : isRTL); Haha, nice :) ::: mobile/android/base/gfx/JavaPanZoomController.java @@ +21,5 @@ > > import android.graphics.PointF; > import android.graphics.RectF; > import android.os.Build; > +import android.os.SystemClock; Import doesn't seem to be used. ::: mobile/android/base/gfx/LayerRenderer.java @@ +428,5 @@ > + mPageContext = createPageContext(metrics, mRenderOffset); > + mScreenContext = createScreenContext(metrics, mRenderOffset); > + > + RectF pageRect = mFrameMetrics.getPageRect(); > + mAbsolutePageRect = RectUtils.round(pageRect); I don't understand what the impact of these changes to rounding behaviour will be. This is getting a free pass from me but you own this code now :). I'd like to retest bugs 765641 and 752959 after this lands to see if these changes fix those bugs. ::: mobile/android/base/gfx/PanZoomTarget.java @@ +16,5 @@ > > public void setAnimationTarget(ImmutableViewportMetrics viewport); > public void setViewportMetrics(ImmutableViewportMetrics viewport); > + public void scrollBy(float dx, float dy); > + public void stopped(); I'd prefer renaming this to panZoomStopped() @@ +28,5 @@ > + public void setOnMetricsChangedListener(PanZoomTarget.OnMetricsChangedListener listener); > + > + public interface OnMetricsChangedListener { > + public void onMetricsChanged(ImmutableViewportMetrics viewport); > + public void onPanZoomStopped(); This doesn't belong in PanZoomTarget, because it's not used by the PZC implementation(s). You can put this function directly on GeckoLayerClient without having it in this interface. ::: mobile/android/base/gfx/ScrollbarLayer.java @@ +187,5 @@ > float viewWidth = context.viewport.width(); > float viewHeight = context.viewport.height(); > > mBarRectF.set(mBarRect.left, viewHeight - mBarRect.top, mBarRect.right, viewHeight - mBarRect.bottom); > + mBarRectF.offset(context.offset.x, -context.offset.y); Should the context.offset.x be negated here? If not please add a comment explaining why not. ::: mobile/android/chrome/content/browser.js @@ +5170,5 @@ > ViewportMetadata.prototype = { > width: null, > height: null, > defaultZoom: null, > minZoom: null, +1 to what Tetsuharu said.

Comment on attachment 739828 [details] [diff] [review] Refactor dynamic toolbar Looks like you obsoleted the patch while I was reviewing. I think most/all of my comments still apply to the new version so I'll wait for an updated patch before re-reviewing.

Comment on attachment 740305 [details] [diff] [review] Refactor dynamic toolbar v2 Review of attachment 740305 [details] [diff] [review]: ----------------------------------------------------------------- I point out your patch a little, Could you see and consider the comment #6 ?

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #6) > Comment on attachment 739828 [details] [diff] [review] > Refactor dynamic toolbar > > Review of attachment 739828 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/chrome/content/browser.js > @@ +3566,5 @@ > > allowZoom: metadata.allowZoom, > > defaultZoom: metadata.defaultZoom || metadata.scaleRatio, > > minZoom: metadata.minZoom || 0, > > maxZoom: metadata.maxZoom || 0, > > + isRTL: metadata.isRTL || false, > > You need not OR operator. This part will work if it's `isRTL: > metadata.isRTL`. Sure - I guess it could be undefined instead of false, but the same is done above so I'll stick with the convention. > @@ +5170,5 @@ > > ViewportMetadata.prototype = { > > width: null, > > height: null, > > defaultZoom: null, > > minZoom: null, > > You should add `isRTL` to ViewportMetadata.prototype. Nice catch.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10) > Comment on attachment 739828 [details] [diff] [review] > Refactor dynamic toolbar > > Review of attachment 739828 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks pretty good! I definitely like how a lot of the code got simpler. I'd > like to see the comments addressed (specially the extraction of a > MarginAnimator helper class) and do another review pass on the updated patch > before r+'ing. > > ::: mobile/android/base/BrowserApp.java > @@ +9,5 @@ > > import org.mozilla.gecko.db.BrowserDB; > > import org.mozilla.gecko.gfx.BitmapUtils; > > import org.mozilla.gecko.gfx.ImmutableViewportMetrics; > > import org.mozilla.gecko.gfx.LayerView; > > +import org.mozilla.gecko.gfx.PanZoomTarget; > > See my comment in PanZoomTarget; I don't think BrowserApp should be using > the PanZoomTarget interface for this. > > ::: mobile/android/base/BrowserToolbar.java > @@ -495,5 @@ > > > > - private boolean canToolbarHide() { > > - // Forbid the toolbar from hiding if hiding the toolbar would cause > > - // the page to go into overscroll. > > - LayerView layerView = GeckoApp.mAppContext.getLayerView(); > > You can remove the LayerView import from this file (BrowserToolbar.java) Thanks - this first version also misses a load of variables at the top that can be removed, fixed this. > ::: mobile/android/base/TextSelection.java > @@ +135,5 @@ > > @Override > > public void run() { > > + mStartHandle.repositionWithViewport(mViewLeft, mViewTop, mViewZoom); > > + mMiddleHandle.repositionWithViewport(mViewLeft, mViewTop, mViewZoom); > > + mEndHandle.repositionWithViewport(mViewLeft, mViewTop, mViewZoom); > > Converting context.viewport.left to mViewLeft here is wrong, because this > runs on a separate thread. context is final so context.viewport.left will > not change, but mViewLeft can. Instead you should pull out a local final > variable at the top of this function for "context.viewport.left - > context.offset.x" and use that in all of the hunks that you changed in this > file. Ditto for the other fields. Thanks, will do. > ::: mobile/android/base/TextSelectionHandle.java > @@ +150,5 @@ > > > > mGeckoPoint = new PointF(left, top); > > if (mIsRTL != rtl) { > > mIsRTL = rtl; > > setImageLevel(mIsRTL ? IMAGE_LEVEL_RTL : IMAGE_LEVEL_LTR); > > Oh, interesting. File a follow-up to unify the text selection isRtl flag > with the one you added. Heh, I didn't see this... > ::: mobile/android/base/gfx/GeckoLayerClient.java > @@ +80,5 @@ > > + private boolean mMarginsPinned; > > + /* The timer that handles showing/hiding margins */ > > + private Timer mAnimationTimer; > > + /* This interpolator is used for the above mentioned animation */ > > + private DecelerateInterpolator mInterpolator; > > I think it makes more sense to move these new fields and many of the new > methods out into a new helper class. For the most part they are not tightly > bound to the existing code in terms of using existing private member > variables/functions, and pulling it out will make it clearer and less > coupled. The stuff I think should be moved are: MS_PER_FRAME, > MARGIN_DURATION_ANIMATION, mMaxMargins, mMarginsPinned, mAnimationTimer, > mInterpolator, setMaxMargins, animateMargins, showMargins, hideMargins, > setMarginsPinned, and large chunks of scrollBy() (I think the > mAnimationTimer bits and the !mMarginsPinned block). > > The new class could be called something like MarginAnimator and would live > here in the gfx/ folder. I would probably try to create it in LayerView and > pass in a reference to the GLC constructor, but I'm not entirely sure if > that will work. Either way I think it makes sense to have a > getMarginAnimator() function in LayerView next to the getLayerClient() and > use that to call things like showMargins from BrowserApp. This sounds reasonable. > @@ +418,4 @@ > > > > + private void animateMargins(final float left, final float top, final float right, final float bottom, boolean immediately) { > > + if (mAnimationTimer != null) { > > + mAnimationTimer.cancel(); > > Also set mAnimationTimer to null here. Well caught, thanks. > @@ +443,5 @@ > > + float progress = mInterpolator.getInterpolation( > > + Math.min(1.0f, (SystemClock.uptimeMillis() - startTime) > > + / (float)MARGIN_ANIMATION_DURATION)); > > + > > + ImmutableViewportMetrics oldMetrics = mViewportMetrics; > > Almost this entire function needs to be inside a synchronized > (GeckoLayerClient.this) block so that other code doesn't come in and twiddle > with mViewportMetrics in the middle of all this. Right, not sure how I missed this... Thanks again! > @@ +873,5 @@ > > + > > + float newMarginLeft = oldMetrics.marginLeft; > > + float newMarginTop = oldMetrics.marginTop; > > + float newMarginRight = oldMetrics.marginRight; > > + float newMarginBottom = oldMetrics.marginBottom; > > Move these inside the !mMarginsPinned block (this goes with my next comment) > > @@ +916,5 @@ > > + .setMargins(newMarginLeft, newMarginTop, newMarginRight, newMarginBottom) > > + .offsetViewportBy(dx, dy); > > + > > + // Set mViewportMetrics manually so the margin changes take. > > + mViewportMetrics = newMetrics; > > Move the setMargins call and the mViewportMetrics direct assignment inside > the !mMarginsPinned block. Leave the offsetViewportBy and setViewportMetrics > call outside. Note that with the refactoring to extract a MarginAnimator > class this means that the margin-related things will be moved but the > offsetViewportBy call will stay here. Sounds good. > ::: mobile/android/base/gfx/ImmutableViewportMetrics.java > @@ +48,2 @@ > > zoomFactor = 1.0f; > > + isRTL = false; > > I'm not entirely convinced that isRTL needs to be stored in this class. Is > it possible to just make it a parameter to the two functions that use it? > I'm not sure if all the call sites have the RTL flag handy but it might be > cleaner to do it that way. I like the idea of having isRTL in the metrics as there have been times I've wanted to use it this way in the past... I'm also not keen on having all the code that calls getMarginOffset needing to look up isRTL on the Tab either, though it is a shame that it gets used so infrequently here... I'm going to deal with your other comments first and we can have a think about this. > @@ +181,5 @@ > > + FloatUtils.interpolate(marginTop, to.marginTop, t), > > + FloatUtils.interpolate(marginRight, to.marginRight, t), > > + FloatUtils.interpolate(marginBottom, to.marginBottom, t), > > + FloatUtils.interpolate(zoomFactor, to.zoomFactor, t), > > + t >= 0.5 ? to.isRTL : isRTL); > > Haha, nice :) Maybe a little stupid... But meh :) > ::: mobile/android/base/gfx/JavaPanZoomController.java > @@ +21,5 @@ > > > > import android.graphics.PointF; > > import android.graphics.RectF; > > import android.os.Build; > > +import android.os.SystemClock; > > Import doesn't seem to be used. Thanks, removed in second version of patch already. > ::: mobile/android/base/gfx/LayerRenderer.java > @@ +428,5 @@ > > + mPageContext = createPageContext(metrics, mRenderOffset); > > + mScreenContext = createScreenContext(metrics, mRenderOffset); > > + > > + RectF pageRect = mFrameMetrics.getPageRect(); > > + mAbsolutePageRect = RectUtils.round(pageRect); > > I don't understand what the impact of these changes to rounding behaviour > will be. This is getting a free pass from me but you own this code now :). > I'd like to retest bugs 765641 and 752959 after this lands to see if these > changes fix those bugs. Kind of glad to own this code because I think it needs some serious tidying up... Also, the bug where you drag the page entirely off the screen causing the view to go white is because the scissor rect becomes invalid, we should check for this (note to self). Will file a follow-up bug when I'm done with this. > ::: mobile/android/base/gfx/PanZoomTarget.java > @@ +16,5 @@ > > > > public void setAnimationTarget(ImmutableViewportMetrics viewport); > > public void setViewportMetrics(ImmutableViewportMetrics viewport); > > + public void scrollBy(float dx, float dy); > > + public void stopped(); > > I'd prefer renaming this to panZoomStopped() I agree. > @@ +28,5 @@ > > + public void setOnMetricsChangedListener(PanZoomTarget.OnMetricsChangedListener listener); > > + > > + public interface OnMetricsChangedListener { > > + public void onMetricsChanged(ImmutableViewportMetrics viewport); > > + public void onPanZoomStopped(); > > This doesn't belong in PanZoomTarget, because it's not used by the PZC > implementation(s). You can put this function directly on GeckoLayerClient > without having it in this interface. Sounds good to me. > ::: mobile/android/base/gfx/ScrollbarLayer.java > @@ +187,5 @@ > > float viewWidth = context.viewport.width(); > > float viewHeight = context.viewport.height(); > > > > mBarRectF.set(mBarRect.left, viewHeight - mBarRect.top, mBarRect.right, viewHeight - mBarRect.bottom); > > + mBarRectF.offset(context.offset.x, -context.offset.y); > > Should the context.offset.x be negated here? If not please add a comment > explaining why not. It shouldn't because GL y coordinates increase going up the screen by default and we don't alter the projection matrix to reverse this. I'm a bit loathe to add a comment about this as that knowledge is assumed in pretty much all the GL code we have Java-side (including the line right above it where we do viewHeight - mBarRect.top). > ::: mobile/android/chrome/content/browser.js > @@ +5170,5 @@ > > ViewportMetadata.prototype = { > > width: null, > > height: null, > > defaultZoom: null, > > minZoom: null, > > +1 to what Tetsuharu said. Done.

(In reply to Chris Lord [:cwiiis] from comment #16) > > I'm not entirely convinced that isRTL needs to be stored in this class. Is > > it possible to just make it a parameter to the two functions that use it? > > I'm not sure if all the call sites have the RTL flag handy but it might be > > cleaner to do it that way. > > I like the idea of having isRTL in the metrics as there have been times I've > wanted to use it this way in the past... I'm also not keen on having all the > code that calls getMarginOffset needing to look up isRTL on the Tab either, > though it is a shame that it gets used so infrequently here... I'm going to > deal with your other comments first and we can have a think about this. Ok. I don't feel to strongly about this either way; in a way it does make sense to keep it in the metrics. My suggestion was mostly driven by the fact that it gets /used/ much less than it gets just passed around, and in my experience that usually indicates that it might be better placed elsewhere. > > Should the context.offset.x be negated here? If not please add a comment > > explaining why not. > > It shouldn't because GL y coordinates increase going up the screen by > default and we don't alter the projection matrix to reverse this. I'm a bit > loathe to add a comment about this as that knowledge is assumed in pretty > much all the GL code we have Java-side (including the line right above it > where we do viewHeight - mBarRect.top). Ok, good point. I didn't look at the surrounding context of that hunk very carefully.

Ok, this does everything we agreed on I think, and I worked out a couple of issues with margins at the very extremes of the page. Carrying r=nrc, will request r?kats.

Note, I've made this change to the patch to fix a crasher: diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java --- a/mobile/android/base/BrowserApp.java +++ b/mobile/android/base/BrowserApp.java @@ -555,19 +555,26 @@ abstract public class BrowserApp extends } private void setToolbarMargin(int margin) { ((RelativeLayout.LayoutParams) mGeckoLayout.getLayoutParams()).topMargin = margin; mGeckoLayout.requestLayout(); } @Override - public void onMetricsChanged(ImmutableViewportMetrics metrics) { - View toolbarLayout = mBrowserToolbar.getLayout(); - toolbarLayout.scrollTo(0, toolbarLayout.getHeight() - Math.round(metrics.marginTop)); + public void onMetricsChanged(ImmutableViewportMetrics aMetrics) { + if (mBrowserToolbar != null) { + final View toolbarLayout = mBrowserToolbar.getLayout(); + final int marginTop = Math.round(aMetrics.marginTop); + ThreadUtils.postToUiThread(new Runnable() { + public void run() { + toolbarLayout.scrollTo(0, toolbarLayout.getHeight() - marginTop); + } + }); + } } @Override public void onPanZoomStopped() { if (!isDynamicToolbarEnabled()) { return; }

Let's just remove ambiguity. This is the latest patch, rebased on top of current m-c, with the crasher fix included. Carrying r=nrc.

Comment on attachment 741293 [details] [diff] [review] Refactor dynamic toolbar v4 Review of attachment 741293 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comments addressed. If the changes seem nontrivial feel free to post an incremental patch with the changes and I can review that too. ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +62,5 @@ > private final ViewTransform mCurrentViewTransform; > private final RectF mCurrentViewTransformMargins; > > + /* Used as a temporary rect by scrollBy */ > + private PointF mScrollByOffset; s/rect/point/. Also make it final. But see my other comment below first; I'd rather just get rid of this entirely. @@ +789,5 @@ > + if (forceRedraw) { > + mForceRedraw = true; > + } > + mViewportMetrics = metrics; > + setViewportMetrics(metrics, notifyGecko); Rather than call the other setViewportMetrics here I would rather factor out the bottom of that function (everything after the mViewportMetrics assignment) into a private helper function | metricsChanged(boolean notifyGecko) | and call that here. @@ +807,5 @@ > + // Set mViewportMetrics manually so the margin changes take. > + mScrollByOffset.set(dx, dy); > + mViewportMetrics = mMarginsAnimator.scrollBy(mViewportMetrics, mScrollByOffset) > + .offsetViewportBy(mScrollByOffset.x, mScrollByOffset.y); > + setViewportMetrics(mViewportMetrics); (1) It's not at all obvious here that mScrollByOffset is an in/out parameter to mMarginsAnimator. I'd prefer just getting rid of mScrollByOffset entirely, passing in the dx,dy to mMarginsAnimator.scrollBy, and doing the offsetViewportBy call in the margins animator. (2) Also would prefer replacing this call to setViewportMetrics with a call to the metricsChanged function I described above. In any function where we need to set mViewportMetrics I think we should either do | mViewportMetrics = ... | or | setViewportMetrics(...) | but not both. @@ +891,5 @@ > > return layerPoint; > } > > + /** Implementationo f PanZoomTarget */ This comment is wrong now since this function isn't on the PanZoomTarget interface. ::: mobile/android/base/gfx/ImmutableViewportMetrics.java @@ +70,5 @@ > float aCssPageRectTop, float aCssPageRectRight, float aCssPageRectBottom, > float aViewportRectLeft, float aViewportRectTop, float aViewportRectRight, > + float aViewportRectBottom, float aMarginLeft, > + float aMarginTop, float aMarginRight, > + float aMarginBottom, float aZoomFactor, boolean aIsRTL) nit: now that the parameter names are shorter this can be wrapped into one less line if you move aMarginTop up to the end of the previous line and aMarginRight down onto the start of the next line. ::: mobile/android/base/gfx/LayerMarginsAnimator.java @@ +31,5 @@ > + private Timer mAnimationTimer; > + /* This interpolator is used for the above mentioned animation */ > + private DecelerateInterpolator mInterpolator; > + /* The GeckoLayerClient whose margins will be animated */ > + private GeckoLayerClient mTarget; Make mMaxMargins, mTarget and mInterpolator final. @@ +64,5 @@ > + > + if (immediately) { > + mTarget.forceRedraw(); > + ImmutableViewportMetrics newMetrics = mTarget.getViewportMetrics().setMargins(left, top, right, bottom); > + mTarget.forceViewportMetrics(newMetrics, true, false); (1) Can you remove the call to mTarget.forceRedraw() here and pass true for the last arg in the call to forceViewportMetrics? It seems like that would be equivalent (and a little cheaper since it will only incur one call to geometryChanged() instead of two). (2) The code to getViewportMetrics/forceViewportMetrics needs to be contained inside a single synchronized(mTarget.getLock()) block. @@ +84,5 @@ > + float progress = mInterpolator.getInterpolation( > + Math.min(1.0f, (SystemClock.uptimeMillis() - startTime) > + / (float)MARGIN_ANIMATION_DURATION)); > + > + synchronized(mTarget) { Use mTarget.getLock() instead.

Changes made and confirmed on IRC, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/684a5ca2efb7 This regresses a couple of small things that I've filed bug 865298 for.

Aurora uplift?

Backed out due to intermittent Android M3 crashes. https://hg.mozilla.org/integration/mozilla-inbound/rev/2ecb09efe325 https://tbpl.mozilla.org/php/getParsedLog.php?id=22236414&tree=Mozilla-Inbound

04-25 07:25:55.593 E/GeckoAppShell( 1799): java.lang.NullPointerException 04-25 07:25:55.593 E/GeckoAppShell( 1799): at org.mozilla.gecko.gfx.LayerMarginsAnimator$1.run(LayerMarginsAnimator.java:101) 04-25 07:25:55.593 E/GeckoAppShell( 1799): at java.util.Timer$TimerImpl.run(Timer.java:289) mAnimationTimer is null inside the TimerTask. Should be easy to guard against.

(In reply to Aaron Train [:aaronmt] from comment #24) > Aurora uplift? I think Chris earlier said no, too risky (and I agree with that).

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27) > (In reply to Aaron Train [:aaronmt] from comment #24) > > Aurora uplift? > I think Chris earlier said no, too risky (and I agree with that). Bug 856497 is tracked for 22.0.

(In reply to Scoobidiver from comment #28) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27) > > (In reply to Aaron Train [:aaronmt] from comment #24) > > > Aurora uplift? > > I think Chris earlier said no, too risky (and I agree with that). > Bug 856497 is tracked for 22.0. The only reliable way to fix that is to uplift this I think, but it is quite a big change... On the other hand, I'm more confident in this approach than the previous one, and it's at least Aurora and not Beta...

I would still rather not uplift. I'm not convinced bug 856497 is serious enough to warrant the risk. We should at least bake this on central for a week and see how many issues fall out of it before considering uplift.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #30) > I would still rather not uplift. I'm not convinced bug 856497 is serious > enough to warrant the risk. We should at least bake this on central for a > week and see how many issues fall out of it before considering uplift. Agreed. I'm even tempted to say that the feature should be turned off in 22 if we deem bug 856497 too serious - I'm not keen on the likely amount of work that merging a patch before it's had any real testing will cause me...

I think the feature should be turned off in 22 until we make the feature stable. On Aurora 22, I seem we need not turn off it. But on Beta/Release 22, turning off might be better. Maybe we should nurture this to a mature feature.

Added null check, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0a7e98a3c3eb

Lucasr pushed a follow-up to fix the bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5c07362a629

I also did a diff between the landings in comment 22 and comment 33 to see if anything else got lost, and noticed that the hunk in CompositorParent.cpp that dropped the const on the currentTransform declaration was also lost. Not sure if that was intentional or not, since the currentTransform isn't being modified so leaving it as const is fine.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #35) > I also did a diff between the landings in comment 22 and comment 33 to see > if anything else got lost, and noticed that the hunk in CompositorParent.cpp > that dropped the const on the currentTransform declaration was also lost. > Not sure if that was intentional or not, since the currentTransform isn't > being modified so leaving it as const is fine. The de-const hunk was purposeful, I didn't mean to leave it in the first time (a hang-over from v1 of the patch) - And thanks Lucas for following up on that, I obviously messed up manually applying the patch after a conflict and the tree wasn't open to push it at a reasonable time where I could watch it :/ Really need to sort out wiggle (or git...)

Tagging as P1, we need this before we ship this feature.

https://hg.mozilla.org/mozilla-central/rev/0a7e98a3c3eb https://hg.mozilla.org/mozilla-central/rev/c5c07362a629

Can we get a Beta uplift? This is resulting in Google pages being zoomed in on back. See bug 876071, bug 869399, bug 852986

(In reply to Aaron Train [:aaronmt] from comment #39) > Can we get a Beta uplift? This is resulting in Google pages being zoomed in > on back. > > See bug 876071, bug 869399, bug 852986 Let's talk about this in bug 852986 - this is way too large of a patch to resolve the zoom issue.

Untracking in favor of bug 852986