1365161 - Dynamic toolbar would not get composition page size when root content document was not scrollable

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect, P3)

Description

[Randall Barker [:rbarker]]

The dynamic toolbar animator was using AsyncPanZoomController::NotifyLayersUpdated to be notified when the composition page size changed. Unfortunately pages that were not scrollable would not receive this notification which would result in the toolbar not hiding when going into fullscreen mode.

Comment 1

[Randall Barker [:rbarker]]

Attachment: Bug 1365161 - Ensure dynamic toolbar static snapshot visibility stays in sync with the real toolbar chrome

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

Comment 2

[Randall Barker [:rbarker]]

Comment on attachment 8867988 [details]
Bug 1365161 - Ensure dynamic toolbar static snapshot visibility stays in sync with the real toolbar chrome

Found an issue where top fixed elements will flash in the wrong location for a single frame with this patch. Canceling review until I can resolve.

Comment 3

[Randall Barker [:rbarker]]

Comment on attachment 8867988 [details]
Bug 1365161 - Ensure dynamic toolbar static snapshot visibility stays in sync with the real toolbar chrome

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

Comment 4

[Randall Barker [:rbarker]]

Comment on attachment 8867988 [details]
Bug 1365161 - Ensure dynamic toolbar static snapshot visibility stays in sync with the real toolbar chrome

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

Comment 5

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

Comment on attachment 8867988 [details]
Bug 1365161 - Ensure dynamic toolbar static snapshot visibility stays in sync with the real toolbar chrome

https://reviewboard.mozilla.org/r/139522/#review143714

r+ for java bits

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8867988 [details]
Bug 1365161 - Ensure dynamic toolbar static snapshot visibility stays in sync with the real toolbar chrome

https://reviewboard.mozilla.org/r/139522/#review144624

LGTM. Up to you if you want to do the suggested refactoring.

::: gfx/layers/composite/AsyncCompositionManager.cpp:963
(Diff revision 3)
> -                mLayersUpdated = false;
> +                  mLayersUpdated = false;
> -              }
> +                }
> -              mIsFirstPaint = false;
> +                // If this is not actually the root content then the animator is not getting updated in AsyncPanZoomController::NotifyLayersUpdated
> +                // because the root content document is not scrollable. So update it here so it knows if the root composition size has changed.
> +                if (!metrics.IsRootContent()) {
> +                  CSSToScreenScale scale = ViewTargetAs<ScreenPixel>(metrics.GetZoom().ToScaleFactor(),

Maybe factor this code out into an AndroidDynamicToolbarAnimator::MaybeUpdateMetrics(const FrameMetrics&) function (which would then be called from here and NotifyLayersUpdated)?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cecb61b2bb6c
Ensure dynamic toolbar static snapshot visibility stays in sync with the real toolbar chrome r=botond,jchen

Comment 8

[Randall Barker [:rbarker]]

Comment on attachment 8867988 [details]
Bug 1365161 - Ensure dynamic toolbar static snapshot visibility stays in sync with the real toolbar chrome

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cecb61b2bb6c