Closed Bug 1180295 (dynamic-toolbar-2) Opened 9 years ago Closed 9 years ago

Redo Fennec dynamic toolbar implementation

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

All
Android
defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(28 files, 5 obsolete files)

(deleted), text/html
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), text/x-review-board-request
rbarker
: review+
Details
(deleted), 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 - Rip out call to setContentDocumentFixedPositionMargins. r?rbarker
Attachment #8642830 - Flags: review?(rbarker)
Bug 1180295 - Rip out the Fennec code to set the screen render offset. r?rbarker
Attachment #8642831 - Flags: review?(rbarker)
Bug 1180295 - Introduce the skeleton of a DynamicToolbarAnimator class alongside LayerMarginsAnimator. r?rbarker
Attachment #8642832 - Flags: review?(rbarker)
Bug 1180295 - Start plumbing inputs to the DynamicToolbarAnimator. r?rbarker
Attachment #8642833 - Flags: review?(rbarker)
Bug 1180295 - Start plumbing the outputs of DynamicToolbarAnimator into BrowserApp. r?rbarker
Attachment #8642834 - Flags: review?(rbarker)
Bug 1180295 - Hook up touch-based scrolling to the new DynamicToolbarAnimator. r?rbarker
Attachment #8642835 - Flags: review?(rbarker)
Bug 1180295 - Disconnect scrolling notifications going to LayerMarginsAnimator. r?rbarker
Attachment #8642836 - Flags: review?(rbarker)
Bug 1180295 - Hook up toolbar show/hide animations to the DynamicToolbarAnimator. r?rbarker
Attachment #8642837 - 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 - Hook up the fixed-position layer margins to the DynamicToolbarAnimator. r?rbarker
Attachment #8642840 - 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 - Update fullscreen code to just pin the toolbar in the hidden state. r?rbarker
Attachment #8642842 - Flags: review?(rbarker)
Bug 1180295 - Update actionbar show/hide code to use the new dynamic toolbar code. r?rbarker
Attachment #8642843 - Flags: review?(rbarker)
Bug 1180295 - Stop exposing the old LayerMarginsAnimator from LayerView. r?rbarker
Attachment #8642844 - Flags: review?(rbarker)
Bug 1180295 - Remove the unneeded margin code from JavaPanZoomController and Axis. r?rbarker
Attachment #8642845 - Flags: review?(rbarker)
Bug 1180295 - Delete the LayerMarginsAnimator class and its dangling entrails. r?rbarker
Attachment #8642846 - 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 - Ensure we don't scroll past the end of the page. r?rbarker
Attachment #8642849 - Flags: review?(rbarker)
Bug 1180295 - Implement seamless snapping to the stable state. r?rbarker
Attachment #8642850 - Flags: review?(rbarker)
Bug 1180295 - Make the text selection handles position correctly. r?rbarker
Attachment #8642851 - Flags: review?(rbarker)
Bug 1180295 - Update the ZoomedView calculations to account for the new dynamic toolbar model. r?rbarker
Attachment #8642852 - Flags: review?(rbarker)
Bug 1180295 - Update the FormAssistPopup to account for the new dynamic toolbar model. r?rbarker
Attachment #8642853 - Flags: review?(rbarker)
Bug 1180295 - Fix up the overscroll edge effect. r?rbarker
Attachment #8642854 - Flags: review?(rbarker)
Bug 1180295 - Remove the margins information from ImmutableViewportMetrics. r?rbarker
Attachment #8642855 - Flags: review?(rbarker)
Bug 1180295 - Fix layerview positioning when dynamic toolbar is turned off. r?rbarker
Attachment #8642856 - Flags: review?(rbarker)
Attachment #8631610 - Attachment is obsolete: true
Attachment #8631611 - Attachment is obsolete: true
Attachment #8631612 - Attachment is obsolete: true
Attachment #8631613 - Attachment is obsolete: true
Attachment #8631614 - Attachment is obsolete: true
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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Depends on: 1203058
Alias: dynamic-toolbar-2
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: