1416319 - Provide DynamicToolbarAnimator API

Status: RESOLVED FIXED

(GeckoView :: Toolbar, enhancement)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

For the session to support the Fennec dynamic toolbar, the session class should provide a DynamicToolbarAnimator getter, e.g. `LayerSession.getDynamicToolbarAnimator()`.

We should clean up DynamicToolbarAnimator so its only public methods are documented APIs, and then make Fennec code use the public API, e.g. `mGeckoSession.DynamicToolbarAnimator().setToolbarChromeProxy()`.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1416319 - 1. Move content-document-is-displayed flag to native code;

One fix I forgot to make in bug 1416310 is to change the code to reset
the flag in LayerView. I think it's better if we just moved the whole
thing to native code, since we don't really use the flag in Java.

Review commit: https://reviewboard.mozilla.org/r/201788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201788/

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1416319 - 2. Remove usage of MetricsListener by FormAssistPopup;

This code is supposed to reposition the form assist popup if the toolbar
appears or disappears while the popup is visible, but we can move that
to a resize event listener in JS, so we don't have to maintain
the MetricsListener API for DynamicToolbarAnimator.

Review commit: https://reviewboard.mozilla.org/r/201790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201790/

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1416319 - 3. Clean up DynamicToolbarAnimator API;

The plan is to incorporate DynamicToolbarAnimator into the official
GeckoView API and expose getter/setter in LayerSession. This patch
cleans up the class to make sure only public APIs are public, and to
assert they are called on the UI thread.

Review commit: https://reviewboard.mozilla.org/r/201792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201792/

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1416319 - 4. Store DynamicToolbarAnimator in LayerSession;

Add getter/setter for DynamicToolbarAnimator in LayerSession in lieu of
LayerView. It is then LayerSession's responsibility to keep track of how
changes in the toolbar affects other things like the window and viewport
bounds.

Review commit: https://reviewboard.mozilla.org/r/201794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201794/

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1416319 - 5. Update DynamicToolbarAnimator usage in other classes;

Use DynamicToolbarAnimator through the session in places where we need
to interact with the toolbar or get the current toolbar height.

Review commit: https://reviewboard.mozilla.org/r/201796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201796/

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1416319 - 6. Remove DynamicToolbarAnimator from LayerView;

Remove all DynamicToolbarAnimator-related code from LayerView now that
they are in LayerSession.

Review commit: https://reviewboard.mozilla.org/r/201798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201798/

Comment 7

[Randall Barker [:rbarker]]

Comment on attachment 8930652 [details]
Bug 1416319 - 1. Move content-document-is-displayed flag to native code;

https://reviewboard.mozilla.org/r/201788/#review207084

Comment 8

[Randall Barker [:rbarker]]

Comment on attachment 8930653 [details]
Bug 1416319 - 2. Remove usage of MetricsListener by FormAssistPopup;

https://reviewboard.mozilla.org/r/201790/#review207086

Comment 9

[Randall Barker [:rbarker]]

Comment on attachment 8930657 [details]
Bug 1416319 - 6. Remove DynamicToolbarAnimator from LayerView;

https://reviewboard.mozilla.org/r/201798/#review207108

Comment 10

[Randall Barker [:rbarker]]

Comment on attachment 8930656 [details]
Bug 1416319 - 5. Update DynamicToolbarAnimator usage in other classes;

https://reviewboard.mozilla.org/r/201796/#review207112

Comment 11

[Randall Barker [:rbarker]]

Comment on attachment 8930655 [details]
Bug 1416319 - 4. Store DynamicToolbarAnimator in LayerSession;

https://reviewboard.mozilla.org/r/201794/#review207114

Comment 12

[Randall Barker [:rbarker]]

Comment on attachment 8930654 [details]
Bug 1416319 - 3. Clean up DynamicToolbarAnimator API;

https://reviewboard.mozilla.org/r/201792/#review207118

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1416319 - 7. Don't expose viewport metrics from LayerSession;

Provide a set of coordinates APIs in LayerSession instead of exposing
the raw viewport metrics, which are hard to use. This also lets us
remove ImmutableViewportMetrics completely.

The new APIs provide rectangular bounds in client or surface
coordinates, and matrices to transform those coordinates to screen
coordinates. This is done because the transformation to screen
coordinates could (in the future) involve skew, rotation, etc, so it's
up to the application to decide how to handle non-rectangular screen
bounds.

Review commit: https://reviewboard.mozilla.org/r/202066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202066/

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1416319 - 8. Switch to using LayerSession coordinates APIs;

Use the LayerSession coordinates APIs instead of manually calculating
coordinates using viewport metrics and toolbar height, which is prone to
error.

Review commit: https://reviewboard.mozilla.org/r/202068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202068/

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930652 [details]
Bug 1416319 - 1. Move content-document-is-displayed flag to native code;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201788/diff/1-2/

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930653 [details]
Bug 1416319 - 2. Remove usage of MetricsListener by FormAssistPopup;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201790/diff/1-2/

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930654 [details]
Bug 1416319 - 3. Clean up DynamicToolbarAnimator API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201792/diff/1-2/

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930655 [details]
Bug 1416319 - 4. Store DynamicToolbarAnimator in LayerSession;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201794/diff/1-2/

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930656 [details]
Bug 1416319 - 5. Update DynamicToolbarAnimator usage in other classes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201796/diff/1-2/

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930657 [details]
Bug 1416319 - 6. Remove DynamicToolbarAnimator from LayerView;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201798/diff/1-2/

Comment 21

[Randall Barker [:rbarker]]

Comment on attachment 8930973 [details]
Bug 1416319 - 7. Don't expose viewport metrics from LayerSession;

https://reviewboard.mozilla.org/r/202066/#review207504

Comment 22

[Randall Barker [:rbarker]]

Comment on attachment 8930974 [details]
Bug 1416319 - 8. Switch to using LayerSession coordinates APIs;

https://reviewboard.mozilla.org/r/202068/#review207506

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930652 [details]
Bug 1416319 - 1. Move content-document-is-displayed flag to native code;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201788/diff/2-3/

Comment 24

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930653 [details]
Bug 1416319 - 2. Remove usage of MetricsListener by FormAssistPopup;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201790/diff/2-3/

Comment 25

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930654 [details]
Bug 1416319 - 3. Clean up DynamicToolbarAnimator API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201792/diff/2-3/

Comment 26

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930655 [details]
Bug 1416319 - 4. Store DynamicToolbarAnimator in LayerSession;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201794/diff/2-3/

Comment 27

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930656 [details]
Bug 1416319 - 5. Update DynamicToolbarAnimator usage in other classes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201796/diff/2-3/

Comment 28

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930657 [details]
Bug 1416319 - 6. Remove DynamicToolbarAnimator from LayerView;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201798/diff/2-3/

Comment 29

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930973 [details]
Bug 1416319 - 7. Don't expose viewport metrics from LayerSession;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202066/diff/1-2/

Comment 30

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8930974 [details]
Bug 1416319 - 8. Switch to using LayerSession coordinates APIs;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202068/diff/1-2/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/268ab0070722
1. Move content-document-is-displayed flag to native code; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/64b9d89c7944
2. Remove usage of MetricsListener by FormAssistPopup; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/ca03654dd10e
3. Clean up DynamicToolbarAnimator API; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/2912f471bcf1
4. Store DynamicToolbarAnimator in LayerSession; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/8debcdbf29a6
5. Update DynamicToolbarAnimator usage in other classes; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/16d7d0e4c2c4
6. Remove DynamicToolbarAnimator from LayerView; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/22648ca7dae8
7. Don't expose viewport metrics from LayerSession; r=rbarker
https://hg.mozilla.org/integration/autoland/rev/d5fec83fec85
8. Switch to using LayerSession coordinates APIs; r=rbarker

Comment 32

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/268ab0070722
https://hg.mozilla.org/mozilla-central/rev/64b9d89c7944
https://hg.mozilla.org/mozilla-central/rev/ca03654dd10e
https://hg.mozilla.org/mozilla-central/rev/2912f471bcf1
https://hg.mozilla.org/mozilla-central/rev/8debcdbf29a6
https://hg.mozilla.org/mozilla-central/rev/16d7d0e4c2c4
https://hg.mozilla.org/mozilla-central/rev/22648ca7dae8
https://hg.mozilla.org/mozilla-central/rev/d5fec83fec85

Comment 33

[Chris Peterson [:cpeterson]]

Moving toolbar bugs to the new GeckoView::Toolbar component.