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)
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.
Assignee | ||
Comment 1•9 years ago
|
||
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.
Comment 2•9 years ago
|
||
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?
Assignee | ||
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Comment 9•9 years ago
|
||
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?
Assignee | ||
Comment 10•9 years ago
|
||
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.
Assignee | ||
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 12•9 years ago
|
||
try |
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
Assignee | ||
Comment 13•9 years ago
|
||
Bug 1180295 - Rip out the FixedMarginsChanged message and all the code that depends on it. r?rbarker
Attachment #8642829 -
Flags: review?(rbarker)
Assignee | ||
Comment 14•9 years ago
|
||
Bug 1180295 - Rip out call to setContentDocumentFixedPositionMargins. r?rbarker
Attachment #8642830 -
Flags: review?(rbarker)
Assignee | ||
Comment 15•9 years ago
|
||
Bug 1180295 - Rip out the Fennec code to set the screen render offset. r?rbarker
Attachment #8642831 -
Flags: review?(rbarker)
Assignee | ||
Comment 16•9 years ago
|
||
Bug 1180295 - Introduce the skeleton of a DynamicToolbarAnimator class alongside LayerMarginsAnimator. r?rbarker
Attachment #8642832 -
Flags: review?(rbarker)
Assignee | ||
Comment 17•9 years ago
|
||
Bug 1180295 - Start plumbing inputs to the DynamicToolbarAnimator. r?rbarker
Attachment #8642833 -
Flags: review?(rbarker)
Assignee | ||
Comment 18•9 years ago
|
||
Bug 1180295 - Start plumbing the outputs of DynamicToolbarAnimator into BrowserApp. r?rbarker
Attachment #8642834 -
Flags: review?(rbarker)
Assignee | ||
Comment 19•9 years ago
|
||
Bug 1180295 - Hook up touch-based scrolling to the new DynamicToolbarAnimator. r?rbarker
Attachment #8642835 -
Flags: review?(rbarker)
Assignee | ||
Comment 20•9 years ago
|
||
Bug 1180295 - Disconnect scrolling notifications going to LayerMarginsAnimator. r?rbarker
Attachment #8642836 -
Flags: review?(rbarker)
Assignee | ||
Comment 21•9 years ago
|
||
Bug 1180295 - Hook up toolbar show/hide animations to the DynamicToolbarAnimator. r?rbarker
Attachment #8642837 -
Flags: review?(rbarker)
Assignee | ||
Comment 22•9 years ago
|
||
Bug 1180295 - Store the viewport width and height as integers instead of floats in ImmutableViewportMetrics. r?rbarker
Attachment #8642839 -
Flags: review?(rbarker)
Assignee | ||
Comment 23•9 years ago
|
||
Bug 1180295 - Hook up the fixed-position layer margins to the DynamicToolbarAnimator. r?rbarker
Attachment #8642840 -
Flags: review?(rbarker)
Assignee | ||
Comment 24•9 years ago
|
||
Bug 1180295 - Stop clipping the content while the toolbar is in the process of sliding off. r?rbarker
Attachment #8642841 -
Flags: review?(rbarker)
Assignee | ||
Comment 25•9 years ago
|
||
Bug 1180295 - Update fullscreen code to just pin the toolbar in the hidden state. r?rbarker
Attachment #8642842 -
Flags: review?(rbarker)
Assignee | ||
Comment 26•9 years ago
|
||
Bug 1180295 - Update actionbar show/hide code to use the new dynamic toolbar code. r?rbarker
Attachment #8642843 -
Flags: review?(rbarker)
Assignee | ||
Comment 27•9 years ago
|
||
Bug 1180295 - Stop exposing the old LayerMarginsAnimator from LayerView. r?rbarker
Attachment #8642844 -
Flags: review?(rbarker)
Assignee | ||
Comment 28•9 years ago
|
||
Bug 1180295 - Remove the unneeded margin code from JavaPanZoomController and Axis. r?rbarker
Attachment #8642845 -
Flags: review?(rbarker)
Assignee | ||
Comment 29•9 years ago
|
||
Bug 1180295 - Delete the LayerMarginsAnimator class and its dangling entrails. r?rbarker
Attachment #8642846 -
Flags: review?(rbarker)
Assignee | ||
Comment 30•9 years ago
|
||
Bug 1180295 - Ensure that on rotation/resize the CSS viewport is resized to the right size. r?rbarker
Attachment #8642847 -
Flags: review?(rbarker)
Assignee | ||
Comment 31•9 years ago
|
||
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)
Assignee | ||
Comment 32•9 years ago
|
||
Bug 1180295 - Ensure we don't scroll past the end of the page. r?rbarker
Attachment #8642849 -
Flags: review?(rbarker)
Assignee | ||
Comment 33•9 years ago
|
||
Bug 1180295 - Implement seamless snapping to the stable state. r?rbarker
Attachment #8642850 -
Flags: review?(rbarker)
Assignee | ||
Comment 34•9 years ago
|
||
Bug 1180295 - Make the text selection handles position correctly. r?rbarker
Attachment #8642851 -
Flags: review?(rbarker)
Assignee | ||
Comment 35•9 years ago
|
||
Bug 1180295 - Update the ZoomedView calculations to account for the new dynamic toolbar model. r?rbarker
Attachment #8642852 -
Flags: review?(rbarker)
Assignee | ||
Comment 36•9 years ago
|
||
Bug 1180295 - Update the FormAssistPopup to account for the new dynamic toolbar model. r?rbarker
Attachment #8642853 -
Flags: review?(rbarker)
Assignee | ||
Comment 37•9 years ago
|
||
Bug 1180295 - Fix up the overscroll edge effect. r?rbarker
Attachment #8642854 -
Flags: review?(rbarker)
Assignee | ||
Comment 38•9 years ago
|
||
Bug 1180295 - Remove the margins information from ImmutableViewportMetrics. r?rbarker
Attachment #8642855 -
Flags: review?(rbarker)
Assignee | ||
Comment 39•9 years ago
|
||
Bug 1180295 - Fix layerview positioning when dynamic toolbar is turned off. r?rbarker
Attachment #8642856 -
Flags: review?(rbarker)
Assignee | ||
Updated•9 years ago
|
Attachment #8631610 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8631611 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8631612 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8631613 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8631614 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8642834 -
Flags: review?(rbarker)
Comment 40•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8642832 -
Flags: review?(rbarker)
Comment 41•9 years ago
|
||
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 42•9 years ago
|
||
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)
Assignee | ||
Comment 43•9 years ago
|
||
(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.
Assignee | ||
Comment 44•9 years ago
|
||
(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...
Assignee | ||
Comment 45•9 years ago
|
||
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
Assignee | ||
Comment 46•9 years ago
|
||
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
Assignee | ||
Comment 47•9 years ago
|
||
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
Assignee | ||
Comment 48•9 years ago
|
||
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)
Assignee | ||
Comment 49•9 years ago
|
||
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
Assignee | ||
Comment 50•9 years ago
|
||
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)
Assignee | ||
Comment 51•9 years ago
|
||
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
Assignee | ||
Comment 52•9 years ago
|
||
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
Assignee | ||
Comment 53•9 years ago
|
||
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
Assignee | ||
Comment 54•9 years ago
|
||
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
Assignee | ||
Comment 55•9 years ago
|
||
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
Assignee | ||
Comment 56•9 years ago
|
||
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
Assignee | ||
Comment 57•9 years ago
|
||
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
Assignee | ||
Comment 58•9 years ago
|
||
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)
Assignee | ||
Comment 59•9 years ago
|
||
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
Assignee | ||
Comment 60•9 years ago
|
||
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
Assignee | ||
Comment 61•9 years ago
|
||
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
Assignee | ||
Comment 62•9 years ago
|
||
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
Assignee | ||
Comment 63•9 years ago
|
||
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
Assignee | ||
Comment 64•9 years ago
|
||
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
Assignee | ||
Comment 65•9 years ago
|
||
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
Assignee | ||
Comment 66•9 years ago
|
||
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
Assignee | ||
Comment 67•9 years ago
|
||
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
Assignee | ||
Comment 68•9 years ago
|
||
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
Assignee | ||
Comment 69•9 years ago
|
||
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
Assignee | ||
Comment 70•9 years ago
|
||
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
Assignee | ||
Comment 71•9 years ago
|
||
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 72•9 years ago
|
||
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 73•9 years ago
|
||
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 74•9 years ago
|
||
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 75•9 years ago
|
||
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 76•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8642851 -
Flags: review?(rbarker) → review+
Comment 77•9 years ago
|
||
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!
Updated•9 years ago
|
Attachment #8642850 -
Flags: review?(rbarker) → review+
Comment 78•9 years ago
|
||
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!
Updated•9 years ago
|
Attachment #8642849 -
Flags: review?(rbarker) → review+
Comment 79•9 years ago
|
||
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 80•9 years ago
|
||
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 81•9 years ago
|
||
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 82•9 years ago
|
||
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 83•9 years ago
|
||
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 84•9 years ago
|
||
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 85•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8642842 -
Flags: review?(rbarker) → review+
Comment 86•9 years ago
|
||
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!
Updated•9 years ago
|
Attachment #8642841 -
Flags: review?(rbarker) → review+
Comment 87•9 years ago
|
||
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 88•9 years ago
|
||
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 89•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8642839 -
Flags: review?(rbarker) → review+
Comment 90•9 years ago
|
||
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!
Updated•9 years ago
|
Attachment #8642837 -
Flags: review?(rbarker) → review+
Comment 91•9 years ago
|
||
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!
Assignee | ||
Comment 92•9 years ago
|
||
(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 93•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8642834 -
Flags: review?(rbarker) → review+
Comment 94•9 years ago
|
||
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!
Updated•9 years ago
|
Attachment #8642833 -
Flags: review?(rbarker) → review+
Comment 95•9 years ago
|
||
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 96•9 years ago
|
||
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 97•9 years ago
|
||
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 98•9 years ago
|
||
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 99•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8642835 -
Flags: review?(rbarker) → review+
Comment 100•9 years ago
|
||
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!
Assignee | ||
Comment 101•9 years ago
|
||
try |
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.
Assignee | ||
Comment 102•9 years ago
|
||
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
Assignee | ||
Comment 103•9 years ago
|
||
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.
Comment 104•9 years ago
|
||
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
Comment 105•9 years ago
|
||
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
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Assignee | ||
Updated•8 years ago
|
Alias: dynamic-toolbar-2
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•