Bug 1180295 (dynamic-toolbar-2)

Redo Fennec dynamic toolbar implementation

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Depends on 2 bugs)

unspecified
Firefox 43
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(28 attachments, 5 obsolete attachments)

20.43 KB, text/html
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
40 bytes, text/x-review-board-request
rbarker
: review+
Details
The dynamic toolbar implementation in Fennec has been rather troublesome in that there are a fair number of edge cases that cause pain both to us Gecko developers and to content authors. I would like to come up with a consistent model for how to implement this across all of our products and implement it in Fennec.
I wrote up some design stuff as I was thinking about this over the last couple of days. I have a fairly good idea of what needs to be done so I'm going to start hacking. Feedback is welcome.
For the record, I like model 3 (it feels like the easiest to implement and reasonably easy to use). I like how what you describe avoids the awkward oscillation situation we have in fennec, but still allows for the toolbar to become permanently visible again, unlike in FirefoxOS.

Have you thought much about how these APIs would be used in FirefoxOS/Firefox.html?
Yeah, model 3 is what I'm working on implementing. And the design should apply to FirefoxOS and browser.html as well. The content frame is an iframe instead of a LayerView which is a negligible difference from Fennec since it can still be moved around as needed using a transform. We can continue to use scrollgrab for scrolling the outer container before the inner container.
I'm going to post WIPs of what I have so far so blassey can see the code I'm deleting (will be relevant for bug 1181804)
Model 3 looks good to me.

Could this mechanism be triggered anytime from the main thread of the parent process?

For example, in browser.html, if the browser is idle for about ~20
seconds, we want to show the toolbar back. We would need, from the
main thread of the parent process to 1) trigger a CSS animation to
move the toolbar, 2) use the compositor API to shift the position
fixed elements, and then do what's described in (e).

This should work, right?
Yes, it should be doable. There's a bit of machinery we'll need to synchronize the parent process changes with the child process changes so that they happen in the same frame of composition - we don't need that for Fennec so I won't be doing it as part of this bug, but it should be doable. BenWa pointed me to bug 1129223 which was solving the same problem - the discussion in that bug with the sequence numbers sounds like exactly what we want and although that didn't get fully implemented it shouldn't be hard to do.
Just an update: I've been working on this over the past week and things are looking good. My current patch queue is at https://github.com/staktrace/gecko-dev/commits/fennec - at this point there are two outstanding issues that I still want to fix before it's ready for a try push and wider manual testing. Hopefully I'll have that ready early next week.
So the patch queue I have is pretty much ready for review. I'm just rebasing it and making sure it still works on master. There wasn't a good way to split the patches so that each part left Fennec in a "good" state, so I did the next best thing: I split the patches so that each part builds and does one conceptual thing. However you need all the patches in order to get a properly working implementation of the new toolbar. If you are missing some of the patches Fennec should still start and run but some aspects of the toolbar may not work properly.

The general structure of the patch queue is (1) remove some stuff that is only used by the current toolbar implementation, (2) add the new toolbar implementation and plumb it in, (3) update the various features that interact with the toolbar, so that they work with the new implementation, (4) remove more stuff that is only used by the current toolbar implementation.

I used http://people.mozilla.org/~kgupta/bug/1180295.html as my test page for most of this, plus ad-hoc testing on various sites. The test page has instructions on things to try and things to watch out for. Note that the new implementation does differ in behaviour from the old implementation in a few (relatively minor) ways. In some cases, if people care enough to file bugs for it, I can do the extra work needed to make the behaviour match the old implementation. In other cases it may not be possible considering the API requirements for the upcoming transition to the C++ APZ.

Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=371c8a163a62
Bug 1180295 - Rip out the FixedMarginsChanged message and all the code that depends on it. r?rbarker
Attachment #8642829 - Flags: review?(rbarker)
Bug 1180295 - Introduce the skeleton of a DynamicToolbarAnimator class alongside LayerMarginsAnimator. r?rbarker
Attachment #8642832 - Flags: review?(rbarker)
Bug 1180295 - Store the viewport width and height as integers instead of floats in ImmutableViewportMetrics. r?rbarker
Attachment #8642839 - Flags: review?(rbarker)
Bug 1180295 - Stop clipping the content while the toolbar is in the process of sliding off. r?rbarker
Attachment #8642841 - Flags: review?(rbarker)
Bug 1180295 - Ensure that on rotation/resize the CSS viewport is resized to the right size. r?rbarker
Attachment #8642847 - Flags: review?(rbarker)
Bug 1180295 - Ensure short pages are dealt with appropriately, so that the toolbar can be made visible but not hidden. r?rbarker
Attachment #8642848 - Flags: review?(rbarker)
Bug 1180295 - Update the ZoomedView calculations to account for the new dynamic toolbar model. r?rbarker
Attachment #8642852 - Flags: review?(rbarker)
Attachment #8642834 - Flags: review?(rbarker)
Comment on attachment 8642834 [details]
MozReview Request: Bug 1180295 - Start plumbing the outputs of DynamicToolbarAnimator into BrowserApp. r?rbarker

https://reviewboard.mozilla.org/r/14921/#review13963

::: mobile/android/base/BrowserApp.java:1237
(Diff revision 1)
> +                ViewHelper.setTranslationY(mLayerView, mBrowserChrome.getHeight());

Can mLayerView be null here? Does it matter?
Attachment #8642832 - Flags: review?(rbarker)
Comment on attachment 8642832 [details]
MozReview Request: Bug 1180295 - Introduce the skeleton of a DynamicToolbarAnimator class alongside LayerMarginsAnimator. r?rbarker

https://reviewboard.mozilla.org/r/14917/#review13959

::: mobile/android/base/gfx/DynamicToolbarAnimator.java:56
(Diff revision 1)
> +     * that needs to be travelled while scrolling before the translation will

"that needs to be travelled" is repeated.
Comment on attachment 8642843 [details]
MozReview Request: Bug 1180295 - Update actionbar show/hide code to use the new dynamic toolbar code. r?rbarker

https://reviewboard.mozilla.org/r/14937/#review13969

::: mobile/android/base/BrowserApp.java:3935
(Diff revision 1)
>      public void endActionModeCompat() {

It apears that ending the action mode is at least one frame too fast as on my Nexus 4, I see unitialized surface at the top of the layer view after the action bar disapears for a frame.
Attachment #8642843 - Flags: review?(rbarker)
(In reply to Randall Barker [:rbarker] from comment #40)
> (Diff revision 1)
> > +                ViewHelper.setTranslationY(mLayerView, mBrowserChrome.getHeight());
> 
> Can mLayerView be null here? Does it matter?

I don't think it can, but I see there is a null check for it a few lines above so maybe there's a codepath I'm missing. I can add a null check here too.

(In reply to Randall Barker [:rbarker] from comment #41)
> ::: mobile/android/base/gfx/DynamicToolbarAnimator.java:56
> (Diff revision 1)
> > +     * that needs to be travelled while scrolling before the translation will
> 
> "that needs to be travelled" is repeated.

Whoops, fixed locally.

(In reply to Randall Barker [:rbarker] from comment #42)
> ::: mobile/android/base/BrowserApp.java:3935
> (Diff revision 1)
> >      public void endActionModeCompat() {
> 
> It apears that ending the action mode is at least one frame too fast as on
> my Nexus 4, I see unitialized surface at the top of the layer view after the
> action bar disapears for a frame.

Hm, good catch. I see that as well, will see if I can fix it.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #43)
> > It apears that ending the action mode is at least one frame too fast as on
> > my Nexus 4, I see unitialized surface at the top of the layer view after the
> > action bar disapears for a frame.
> 
> Hm, good catch. I see that as well, will see if I can fix it.

I fixed this in my updated patchset. I ended up changing part 4 (the skeleton of DynamicToolbarAnimator) a little bit, but the bulk of the change was in part 21 (seamless snapping). The code should now be simpler to understand and more bug-free than it was before.

Hopefully MozReview does the Right Thing (TM) with the updated patches...
Comment on attachment 8642829 [details]
MozReview Request: Bug 1180295 - Rip out the FixedMarginsChanged message and all the code that depends on it. r?rbarker

Bug 1180295 - Rip out the FixedMarginsChanged message and all the code that depends on it. r?rbarker
Comment on attachment 8642830 [details]
MozReview Request: Bug 1180295 - Rip out call to setContentDocumentFixedPositionMargins. r?rbarker

Bug 1180295 - Rip out call to setContentDocumentFixedPositionMargins. r?rbarker
Comment on attachment 8642831 [details]
MozReview Request: Bug 1180295 - Rip out the Fennec code to set the screen render offset. r?rbarker

Bug 1180295 - Rip out the Fennec code to set the screen render offset. r?rbarker
Comment on attachment 8642832 [details]
MozReview Request: Bug 1180295 - Introduce the skeleton of a DynamicToolbarAnimator class alongside LayerMarginsAnimator. r?rbarker

Bug 1180295 - Introduce the skeleton of a DynamicToolbarAnimator class alongside LayerMarginsAnimator. r?rbarker
Attachment #8642832 - Flags: review?(rbarker)
Comment on attachment 8642833 [details]
MozReview Request: Bug 1180295 - Start plumbing inputs to the DynamicToolbarAnimator. r?rbarker

Bug 1180295 - Start plumbing inputs to the DynamicToolbarAnimator. r?rbarker
Comment on attachment 8642834 [details]
MozReview Request: Bug 1180295 - Start plumbing the outputs of DynamicToolbarAnimator into BrowserApp. r?rbarker

Bug 1180295 - Start plumbing the outputs of DynamicToolbarAnimator into BrowserApp. r?rbarker
Attachment #8642834 - Flags: review?(rbarker)
Comment on attachment 8642835 [details]
MozReview Request: Bug 1180295 - Hook up touch-based scrolling to the new DynamicToolbarAnimator. r?rbarker

Bug 1180295 - Hook up touch-based scrolling to the new DynamicToolbarAnimator. r?rbarker
Comment on attachment 8642836 [details]
MozReview Request: Bug 1180295 - Disconnect scrolling notifications going to LayerMarginsAnimator. r?rbarker

Bug 1180295 - Disconnect scrolling notifications going to LayerMarginsAnimator. r?rbarker
Comment on attachment 8642837 [details]
MozReview Request: Bug 1180295 - Hook up toolbar show/hide animations to the DynamicToolbarAnimator. r?rbarker

Bug 1180295 - Hook up toolbar show/hide animations to the DynamicToolbarAnimator. r?rbarker
Comment on attachment 8642839 [details]
MozReview Request: Bug 1180295 - Store the viewport width and height as integers instead of floats in ImmutableViewportMetrics. r?rbarker

Bug 1180295 - Store the viewport width and height as integers instead of floats in ImmutableViewportMetrics. r?rbarker
Comment on attachment 8642840 [details]
MozReview Request: Bug 1180295 - Hook up the fixed-position layer margins to the DynamicToolbarAnimator. r?rbarker

Bug 1180295 - Hook up the fixed-position layer margins to the DynamicToolbarAnimator. r?rbarker
Comment on attachment 8642841 [details]
MozReview Request: Bug 1180295 - Stop clipping the content while the toolbar is in the process of sliding off. r?rbarker

Bug 1180295 - Stop clipping the content while the toolbar is in the process of sliding off. r?rbarker
Comment on attachment 8642842 [details]
MozReview Request: Bug 1180295 - Update fullscreen code to just pin the toolbar in the hidden state. r?rbarker

Bug 1180295 - Update fullscreen code to just pin the toolbar in the hidden state. r?rbarker
Comment on attachment 8642843 [details]
MozReview Request: Bug 1180295 - Update actionbar show/hide code to use the new dynamic toolbar code. r?rbarker

Bug 1180295 - Update actionbar show/hide code to use the new dynamic toolbar code. r?rbarker
Attachment #8642843 - Flags: review?(rbarker)
Comment on attachment 8642844 [details]
MozReview Request: Bug 1180295 - Stop exposing the old LayerMarginsAnimator from LayerView. r?rbarker

Bug 1180295 - Stop exposing the old LayerMarginsAnimator from LayerView. r?rbarker
Comment on attachment 8642845 [details]
MozReview Request: Bug 1180295 - Remove the unneeded margin code from JavaPanZoomController and Axis. r?rbarker

Bug 1180295 - Remove the unneeded margin code from JavaPanZoomController and Axis. r?rbarker
Comment on attachment 8642846 [details]
MozReview Request: Bug 1180295 - Delete the LayerMarginsAnimator class and its dangling entrails. r?rbarker

Bug 1180295 - Delete the LayerMarginsAnimator class and its dangling entrails. r?rbarker
Comment on attachment 8642847 [details]
MozReview Request: Bug 1180295 - Ensure that on rotation/resize the CSS viewport is resized to the right size. r?rbarker

Bug 1180295 - Ensure that on rotation/resize the CSS viewport is resized to the right size. r?rbarker
Comment on attachment 8642848 [details]
MozReview Request: Bug 1180295 - Ensure short pages are dealt with appropriately, so that the toolbar can be made visible but not hidden. r?rbarker

Bug 1180295 - Ensure short pages are dealt with appropriately, so that the toolbar can be made visible but not hidden. r?rbarker
Comment on attachment 8642849 [details]
MozReview Request: Bug 1180295 - Ensure we don't scroll past the end of the page. r?rbarker

Bug 1180295 - Ensure we don't scroll past the end of the page. r?rbarker
Comment on attachment 8642850 [details]
MozReview Request: Bug 1180295 - Implement seamless snapping to the stable state. r?rbarker

Bug 1180295 - Implement seamless snapping to the stable state. r?rbarker
Comment on attachment 8642851 [details]
MozReview Request: Bug 1180295 - Make the text selection handles position correctly. r?rbarker

Bug 1180295 - Make the text selection handles position correctly. r?rbarker
Comment on attachment 8642852 [details]
MozReview Request: Bug 1180295 - Update the ZoomedView calculations to account for the new dynamic toolbar model. r?rbarker

Bug 1180295 - Update the ZoomedView calculations to account for the new dynamic toolbar model. r?rbarker
Comment on attachment 8642853 [details]
MozReview Request: Bug 1180295 - Update the FormAssistPopup to account for the new dynamic toolbar model. r?rbarker

Bug 1180295 - Update the FormAssistPopup to account for the new dynamic toolbar model. r?rbarker
Comment on attachment 8642854 [details]
MozReview Request: Bug 1180295 - Fix up the overscroll edge effect. r?rbarker

Bug 1180295 - Fix up the overscroll edge effect. r?rbarker
Comment on attachment 8642855 [details]
MozReview Request: Bug 1180295 - Remove the margins information from ImmutableViewportMetrics. r?rbarker

Bug 1180295 - Remove the margins information from ImmutableViewportMetrics. r?rbarker
Comment on attachment 8642856 [details]
MozReview Request: Bug 1180295 - Fix layerview positioning when dynamic toolbar is turned off. r?rbarker

Bug 1180295 - Fix layerview positioning when dynamic toolbar is turned off. r?rbarker
Comment on attachment 8642856 [details]
MozReview Request: Bug 1180295 - Fix layerview positioning when dynamic toolbar is turned off. r?rbarker

https://reviewboard.mozilla.org/r/14963/#review14299

Ship It!
Attachment #8642856 - Flags: review?(rbarker) → review+
Comment on attachment 8642855 [details]
MozReview Request: Bug 1180295 - Remove the margins information from ImmutableViewportMetrics. r?rbarker

https://reviewboard.mozilla.org/r/14961/#review14301

Ship It!
Attachment #8642855 - Flags: review?(rbarker) → review+
Comment on attachment 8642854 [details]
MozReview Request: Bug 1180295 - Fix up the overscroll edge effect. r?rbarker

https://reviewboard.mozilla.org/r/14959/#review14303

Ship It!
Attachment #8642854 - Flags: review?(rbarker) → review+
Comment on attachment 8642853 [details]
MozReview Request: Bug 1180295 - Update the FormAssistPopup to account for the new dynamic toolbar model. r?rbarker

https://reviewboard.mozilla.org/r/14957/#review14305

Ship It!
Attachment #8642853 - Flags: review?(rbarker) → review+
Comment on attachment 8642852 [details]
MozReview Request: Bug 1180295 - Update the ZoomedView calculations to account for the new dynamic toolbar model. r?rbarker

https://reviewboard.mozilla.org/r/14955/#review14309

Ship It!
Attachment #8642852 - Flags: review?(rbarker) → review+
Attachment #8642851 - Flags: review?(rbarker) → review+
Comment on attachment 8642851 [details]
MozReview Request: Bug 1180295 - Make the text selection handles position correctly. r?rbarker

https://reviewboard.mozilla.org/r/14953/#review14315

Ship It!
Attachment #8642850 - Flags: review?(rbarker) → review+
Comment on attachment 8642850 [details]
MozReview Request: Bug 1180295 - Implement seamless snapping to the stable state. r?rbarker

https://reviewboard.mozilla.org/r/14951/#review14319

Ship It!
Attachment #8642849 - Flags: review?(rbarker) → review+
Comment on attachment 8642849 [details]
MozReview Request: Bug 1180295 - Ensure we don't scroll past the end of the page. r?rbarker

https://reviewboard.mozilla.org/r/14949/#review14321

Ship It!
Comment on attachment 8642848 [details]
MozReview Request: Bug 1180295 - Ensure short pages are dealt with appropriately, so that the toolbar can be made visible but not hidden. r?rbarker

https://reviewboard.mozilla.org/r/14947/#review14323

Ship It!
Attachment #8642848 - Flags: review?(rbarker) → review+
Comment on attachment 8642847 [details]
MozReview Request: Bug 1180295 - Ensure that on rotation/resize the CSS viewport is resized to the right size. r?rbarker

https://reviewboard.mozilla.org/r/14945/#review14325

Ship It!
Attachment #8642847 - Flags: review?(rbarker) → review+
Comment on attachment 8642846 [details]
MozReview Request: Bug 1180295 - Delete the LayerMarginsAnimator class and its dangling entrails. r?rbarker

https://reviewboard.mozilla.org/r/14943/#review14327

Ship It!
Attachment #8642846 - Flags: review?(rbarker) → review+
Comment on attachment 8642845 [details]
MozReview Request: Bug 1180295 - Remove the unneeded margin code from JavaPanZoomController and Axis. r?rbarker

https://reviewboard.mozilla.org/r/14941/#review14329

Ship It!
Attachment #8642845 - Flags: review?(rbarker) → review+
Comment on attachment 8642844 [details]
MozReview Request: Bug 1180295 - Stop exposing the old LayerMarginsAnimator from LayerView. r?rbarker

https://reviewboard.mozilla.org/r/14939/#review14331

Ship It!
Attachment #8642844 - Flags: review?(rbarker) → review+
Comment on attachment 8642843 [details]
MozReview Request: Bug 1180295 - Update actionbar show/hide code to use the new dynamic toolbar code. r?rbarker

https://reviewboard.mozilla.org/r/14937/#review14333

Ship It!
Attachment #8642843 - Flags: review?(rbarker) → review+
Attachment #8642842 - Flags: review?(rbarker) → review+
Comment on attachment 8642842 [details]
MozReview Request: Bug 1180295 - Update fullscreen code to just pin the toolbar in the hidden state. r?rbarker

https://reviewboard.mozilla.org/r/14935/#review14335

Ship It!
Attachment #8642841 - Flags: review?(rbarker) → review+
Comment on attachment 8642841 [details]
MozReview Request: Bug 1180295 - Stop clipping the content while the toolbar is in the process of sliding off. r?rbarker

https://reviewboard.mozilla.org/r/14933/#review14337

Ship It!
Comment on attachment 8642840 [details]
MozReview Request: Bug 1180295 - Hook up the fixed-position layer margins to the DynamicToolbarAnimator. r?rbarker

https://reviewboard.mozilla.org/r/14931/#review14345

::: mobile/android/base/gfx/DynamicToolbarAnimator.java:325
(Diff revision 2)
> +    void populateFixedPositionMargins(ViewTransform aTransform, ImmutableViewportMetrics aMetrics) {

Should this function be marked explicitly private? I only ask because we seem to always label everyting in Java code. I assume this is just a sytle thing?
Attachment #8642840 - Flags: review?(rbarker)
Comment on attachment 8642840 [details]
MozReview Request: Bug 1180295 - Hook up the fixed-position layer margins to the DynamicToolbarAnimator. r?rbarker

https://reviewboard.mozilla.org/r/14931/#review14347

Ship It!
Attachment #8642840 - Flags: review+
Attachment #8642839 - Flags: review?(rbarker) → review+
Comment on attachment 8642839 [details]
MozReview Request: Bug 1180295 - Store the viewport width and height as integers instead of floats in ImmutableViewportMetrics. r?rbarker

https://reviewboard.mozilla.org/r/14929/#review14349

Ship It!
Attachment #8642837 - Flags: review?(rbarker) → review+
Comment on attachment 8642837 [details]
MozReview Request: Bug 1180295 - Hook up toolbar show/hide animations to the DynamicToolbarAnimator. r?rbarker

https://reviewboard.mozilla.org/r/14927/#review14351

Ship It!
(In reply to Randall Barker [:rbarker] from comment #88)
> > +    void populateFixedPositionMargins(ViewTransform aTransform, ImmutableViewportMetrics aMetrics) {
> 
> Should this function be marked explicitly private? I only ask because we
> seem to always label everyting in Java code. I assume this is just a sytle
> thing?

This function is package-scoped, because it is called from GeckoLayerClient. I can't make it private, but I also don't want to make it public because I don't want random non-gfx code calling it.
Comment on attachment 8642836 [details]
MozReview Request: Bug 1180295 - Disconnect scrolling notifications going to LayerMarginsAnimator. r?rbarker

https://reviewboard.mozilla.org/r/14925/#review14353

Ship It!
Attachment #8642836 - Flags: review?(rbarker) → review+
Attachment #8642834 - Flags: review?(rbarker) → review+
Comment on attachment 8642834 [details]
MozReview Request: Bug 1180295 - Start plumbing the outputs of DynamicToolbarAnimator into BrowserApp. r?rbarker

https://reviewboard.mozilla.org/r/14921/#review14359

Ship It!
Attachment #8642833 - Flags: review?(rbarker) → review+
Comment on attachment 8642833 [details]
MozReview Request: Bug 1180295 - Start plumbing inputs to the DynamicToolbarAnimator. r?rbarker

https://reviewboard.mozilla.org/r/14919/#review14361

Ship It!
Comment on attachment 8642832 [details]
MozReview Request: Bug 1180295 - Introduce the skeleton of a DynamicToolbarAnimator class alongside LayerMarginsAnimator. r?rbarker

https://reviewboard.mozilla.org/r/14917/#review14363

Ship It!
Attachment #8642832 - Flags: review?(rbarker) → review+
Comment on attachment 8642831 [details]
MozReview Request: Bug 1180295 - Rip out the Fennec code to set the screen render offset. r?rbarker

https://reviewboard.mozilla.org/r/14915/#review14365

Ship It!
Attachment #8642831 - Flags: review?(rbarker) → review+
Comment on attachment 8642830 [details]
MozReview Request: Bug 1180295 - Rip out call to setContentDocumentFixedPositionMargins. r?rbarker

https://reviewboard.mozilla.org/r/14913/#review14367

Ship It!
Attachment #8642830 - Flags: review?(rbarker) → review+
Comment on attachment 8642829 [details]
MozReview Request: Bug 1180295 - Rip out the FixedMarginsChanged message and all the code that depends on it. r?rbarker

https://reviewboard.mozilla.org/r/14911/#review14369

Ship It!
Attachment #8642829 - Flags: review?(rbarker) → review+
Attachment #8642835 - Flags: review?(rbarker) → review+
Comment on attachment 8642835 [details]
MozReview Request: Bug 1180295 - Hook up touch-based scrolling to the new DynamicToolbarAnimator. r?rbarker

https://reviewboard.mozilla.org/r/14923/#review14371

Ship It!
I did a new try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=27595b09688e and there's a set of new failures in both Android 4.0 API 11+ and Android 2.3 API9. The 9 failing tests are all tests that get compared == to about:blank, and they're all failing because the about:blank page is 48 pixels shorter than the test page. It's likely this is caused by my patches and since it's happening on API9 I should probably at least try to investigate.
I did a bunch of try pushes to figure out what was going on with the tests. It looks API9 also has a problem with the window size, as I noted in bug 1192616. The new failures seem to be fairly easy to reproduce on try, but I make those tests dump the display list by adding a pref(layout.display-list.dump,true) to the reftest.list file, then the tests pass. In [1] there's a full set of failures, in [2] I added the display-list dumping to two tests, and those two tests passed and the rest continued to fail. In [3] I added the dumping to all the failing tests and they are all passing.

From the other logging I put in it doesn't appear to be an issue with the CSS viewport, since the CSS viewport is never changed after it's initially set. I think it's just more Fennec-reftests wackiness.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c90d5f4c0ea5
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6a060b518c4
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=54ac4930eacd
Latest try push is green except for things that are hidden on m-c, or tests that were already broken.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94a2f98d825a

I'll land this once inbound reopens.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a44573aaee7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5d30f02cc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e291287590c
https://hg.mozilla.org/integration/mozilla-inbound/rev/44e23aca35f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8faf8d59274
https://hg.mozilla.org/integration/mozilla-inbound/rev/b675e378dadf
https://hg.mozilla.org/integration/mozilla-inbound/rev/0700729ed007
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a90ad6126e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed49a9cb859
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f960e690b57
https://hg.mozilla.org/integration/mozilla-inbound/rev/99247cd12b2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b95c4c965912
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c97901677b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee26da22cfc
https://hg.mozilla.org/integration/mozilla-inbound/rev/76a30651e5b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/d388bd547a70
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e30c1bf4a3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d26d591f402
https://hg.mozilla.org/integration/mozilla-inbound/rev/893b1381755a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8573249b176e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a739dbce5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e88f6a19cf80
https://hg.mozilla.org/integration/mozilla-inbound/rev/3afafc10b12a
https://hg.mozilla.org/integration/mozilla-inbound/rev/64c4d2d55f15
https://hg.mozilla.org/integration/mozilla-inbound/rev/bece031080d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5078e84222d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b213c98f8a8
https://hg.mozilla.org/mozilla-central/rev/a44573aaee7c
https://hg.mozilla.org/mozilla-central/rev/cd5d30f02cc0
https://hg.mozilla.org/mozilla-central/rev/2e291287590c
https://hg.mozilla.org/mozilla-central/rev/44e23aca35f3
https://hg.mozilla.org/mozilla-central/rev/f8faf8d59274
https://hg.mozilla.org/mozilla-central/rev/b675e378dadf
https://hg.mozilla.org/mozilla-central/rev/0700729ed007
https://hg.mozilla.org/mozilla-central/rev/69a90ad6126e
https://hg.mozilla.org/mozilla-central/rev/2ed49a9cb859
https://hg.mozilla.org/mozilla-central/rev/2f960e690b57
https://hg.mozilla.org/mozilla-central/rev/99247cd12b2b
https://hg.mozilla.org/mozilla-central/rev/b95c4c965912
https://hg.mozilla.org/mozilla-central/rev/b3c97901677b
https://hg.mozilla.org/mozilla-central/rev/0ee26da22cfc
https://hg.mozilla.org/mozilla-central/rev/76a30651e5b5
https://hg.mozilla.org/mozilla-central/rev/d388bd547a70
https://hg.mozilla.org/mozilla-central/rev/8e30c1bf4a3b
https://hg.mozilla.org/mozilla-central/rev/0d26d591f402
https://hg.mozilla.org/mozilla-central/rev/893b1381755a
https://hg.mozilla.org/mozilla-central/rev/8573249b176e
https://hg.mozilla.org/mozilla-central/rev/f2a739dbce5f
https://hg.mozilla.org/mozilla-central/rev/e88f6a19cf80
https://hg.mozilla.org/mozilla-central/rev/3afafc10b12a
https://hg.mozilla.org/mozilla-central/rev/64c4d2d55f15
https://hg.mozilla.org/mozilla-central/rev/bece031080d6
https://hg.mozilla.org/mozilla-central/rev/5078e84222d7
https://hg.mozilla.org/mozilla-central/rev/2b213c98f8a8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Depends on: 1203058
You need to log in before you can comment on or make changes to this bug.