The snapshot for the new dynamic toolbar appears in the content when you are viewing a chrome custom tab and already had Fennec running. We need to make sure the state is managed correctly there when entering/exiting custom tabs.

I'm guessing this won't be an issue if we switch to GeckoView custom tabs? I'll check on Monday morning to be sure.

(In reply to Dylan Roeh (:droeh) from comment #2) > I'm guessing this won't be an issue if we switch to GeckoView custom tabs? > I'll check on Monday morning to be sure. Yeah, looks like this is the case, only GeckoApp-based custom tabs are affected. I took a quick shot at trying to figure out what I'd need to do to hide this in custom tabs, but without much luck. Randall, if you have any questions about custom tabs (or if you want to suggest an approach and hand it off to me), just let me know.

(In reply to Dylan Roeh (:droeh) from comment #3) > (In reply to Dylan Roeh (:droeh) from comment #2) > > I'm guessing this won't be an issue if we switch to GeckoView custom tabs? > > I'll check on Monday morning to be sure. > > Yeah, looks like this is the case, only GeckoApp-based custom tabs are > affected. > > I took a quick shot at trying to figure out what I'd need to do to hide this > in custom tabs, but without much luck. Randall, if you have any questions > about custom tabs (or if you want to suggest an approach and hand it off to > me), just let me know. In mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java hideToolbar(true) will hide the static toolbar immediately. You will then need to pin it with setPinned. You will need a new PinReason and will also need to unpin when returning control to Fennec.

Yeah, hideToolbar is what I was looking at... I couldn't get it to work, and unfortunately pinning doesn't seem to change that -- I still get the red bar with this patch that hides and pins in onResume and unpins in onPause.

Okay, it is probably because the real chrome toolbar is already hidden so the message never gets send to the animator to hide the static snapshot. As a test maybe do a showToolbar(true) and once the getCurrentToolbarHeight is > 0 do a hideToolbar(true)? If that works, will obviously need a better solution. Probably will need to add a forceToolbarHide.

Actually that isn't it. I'll need to look at what is going on when a get a chance.

Comment on attachment 8861163 [details] [diff] [review] Custom tabs dynamic toolbar fix (not working) This looks correct. I'll try and figure out why it isn't working.

Comment on attachment 8861493 [details] Bug 1358805 - part 1: Add new UiCompositorController message types TOOLBAR_SNAPSHOT_FAILED and IS_COMPOSITOR_CONTROLLER_OPEN https://reviewboard.mozilla.org/r/133466/#review136398 Need to make the same changes to the constants in LayerView. (Yes, I know you did it in part 2. You should have done it in this one)

Comment on attachment 8861493 [details] Bug 1358805 - part 1: Add new UiCompositorController message types TOOLBAR_SNAPSHOT_FAILED and IS_COMPOSITOR_CONTROLLER_OPEN https://reviewboard.mozilla.org/r/133466/#review136402 Actually even better would be split this patchset functionally. So for example introducing the IS_COMPOSITOR_CONTROLLER_OPEN message and the handling of that message should all be in one patch instead of split across three patches.

Comment on attachment 8861496 [details] Bug 1358805 - part 4: Allow custom tabs to pin the dynamic toolbar https://reviewboard.mozilla.org/r/133472/#review136400 ::: commit-message-27a22:1 (Diff revision 1) > +Bug 1358805 - part 4: Update UiCompositorControllerChild to repsond to IS_COMPOSITOR_CONTROLLER_OPEN message r=kats s/repsond/respond/

Comment on attachment 8861497 [details] Bug 1358805 - part 5: Update the dynamic toolbar animator to gracefully handle toolbar snapshot generation failure https://reviewboard.mozilla.org/r/133474/#review136410 Likewise all the code related to the toolbar snapshot failing should go into one patch. That includes the constants, the PostMessage call and the receiver/handler of the message.

Comment on attachment 8861498 [details] Bug 1358805 - part 6: Remove MOZ_ASSERT that would intermittently fail on custom tabs start up https://reviewboard.mozilla.org/r/133476/#review136414 ::: widget/android/nsWindow.cpp (Diff revision 1) > { > RefPtr<UiCompositorControllerChild> child; > if (LockedWindowPtr window{mWindow}) { > child = window->GetUiCompositorControllerChild(); > } > - MOZ_ASSERT(child); IIRC this assert was added specifically for a reason, see second comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1335895#c412. If we're removing the assert from here we should at least put the assert in the ResumeAndResize call to make sure we're not dropping those (important) messages.

Comment on attachment 8861496 [details] Bug 1358805 - part 4: Allow custom tabs to pin the dynamic toolbar https://reviewboard.mozilla.org/r/133472/#review136464 Looks good to me.

Comment on attachment 8861495 [details] Bug 1358805 - part 3: Keep the toolbar state between UI thread and compositor thread consistent once IPC is open https://reviewboard.mozilla.org/r/133470/#review136474 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java:157 (Diff revision 2) > + if (mToolbarChromeProxy != null) { > + mCompositor.sendToolbarAnimatorMessage(mToolbarChromeProxy.isToolbarChromeVisible() ? > + LayerView.REQUEST_SHOW_TOOLBAR_IMMEDIATELY : > + LayerView.REQUEST_HIDE_TOOLBAR_IMMEDIATELY); > + } else { > + mCompositor.sendToolbarAnimatorMessage(LayerView.REQUEST_HIDE_TOOLBAR_IMMEDIATELY); This would be clearer if you combined the hide cases. e.g. if (mToolbarChromeProxy != null && mToolbarChromeProxy.IsToolbarChromeVisible()) { // show } else { // hide }

Comment on attachment 8861497 [details] Bug 1358805 - part 5: Update the dynamic toolbar animator to gracefully handle toolbar snapshot generation failure https://reviewboard.mozilla.org/r/133474/#review136476 Next time I'm gonna be super strict about patch splitting. Be warned!

Comment on attachment 8861494 [details] Bug 1358805 - part 2: Allow DynamicToolbarAnimator to query if the UiCompositorController is open in the case it missed the open message https://reviewboard.mozilla.org/r/133468/#review136478 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java:161 (Diff revision 2) > mCompositor.setMaxToolbarHeight(mMaxToolbarHeight); > for (PinReason reason : pinFlags) { > mCompositor.setPinned(true, reason.mValue); > } > + } else if ((mCompositor != null) && !mCompositorControllerOpen) { > + mCompositor.sendToolbarAnimatorMessage(LayerView.IS_COMPOSITOR_CONTROLLER_OPEN); This warrants a comment

Comment on attachment 8861493 [details] Bug 1358805 - part 1: Add new UiCompositorController message types TOOLBAR_SNAPSHOT_FAILED and IS_COMPOSITOR_CONTROLLER_OPEN https://reviewboard.mozilla.org/r/133466/#review136482

Comment on attachment 8861497 [details] Bug 1358805 - part 5: Update the dynamic toolbar animator to gracefully handle toolbar snapshot generation failure https://reviewboard.mozilla.org/r/133474/#review136476 How does the current state of this patch set not meet what you want?

Pushed by rbarker@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2b347e2d970a part 1: Add new UiCompositorController message types TOOLBAR_SNAPSHOT_FAILED and IS_COMPOSITOR_CONTROLLER_OPEN r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/e18effd70e44 part 2: Allow DynamicToolbarAnimator to query if the UiCompositorController is open in the case it missed the open message r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e2c8e23cab part 3: Keep the toolbar state between UI thread and compositor thread consistent once IPC is open r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/2a700a8d9e0f part 4: Allow custom tabs to pin the dynamic toolbar r=droeh https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f7c7cea218 part 5: Update the dynamic toolbar animator to gracefully handle toolbar snapshot generation failure r=kats

(In reply to Randall Barker [:rbarker] from comment #30) > Comment on attachment 8861497 [details] > Bug 1358805 - part 5: Update the dynamic toolbar animator to gracefully > handle toolbar snapshot generation failure > > https://reviewboard.mozilla.org/r/133474/#review136476 > > How does the current state of this patch set not meet what you want? See comment 16 and comment 18. The constants in part 1 should have been added with the code that uses them, so as to be a functional unit of code.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #36) > (In reply to Randall Barker [:rbarker] from comment #30) > > Comment on attachment 8861497 [details] > > Bug 1358805 - part 5: Update the dynamic toolbar animator to gracefully > > handle toolbar snapshot generation failure > > > > https://reviewboard.mozilla.org/r/133474/#review136476 > > > > How does the current state of this patch set not meet what you want? > > See comment 16 and comment 18. The constants in part 1 should have been > added with the code that uses them, so as to be a functional unit of code. Did I not address those comments with the current patch set? You original comment seems imply I did not.

You left the constants in part 1. That part shouldn't even exist; the constants should be added in the parts that use them.

https://hg.mozilla.org/mozilla-central/rev/2b347e2d970a https://hg.mozilla.org/mozilla-central/rev/e18effd70e44 https://hg.mozilla.org/mozilla-central/rev/c2e2c8e23cab https://hg.mozilla.org/mozilla-central/rev/2a700a8d9e0f https://hg.mozilla.org/mozilla-central/rev/d2f7c7cea218