Closed Bug 1358805 Opened 7 years ago Closed 7 years ago

Dynamic toolbar snapshot shown in chrome custom tabs

Categories

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

Product:
Firefox for Android Graveyard
Component:
Toolbar
Version:
55 Branch
Platform:
All
Android
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: snorp, Assigned: rbarker)

References

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

Bug 1358805 - part 1: Add new UiCompositorController message types TOOLBAR_SNAPSHOT_FAILED and IS_COMPOSITOR_CONTROLLER_OPEN
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1358805 - part 2: Allow DynamicToolbarAnimator to query if the UiCompositorController is open in the case it missed the open message
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1358805 - part 3: Keep the toolbar state between UI thread and compositor thread consistent once IPC is open
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1358805 - part 4: Allow custom tabs to pin the dynamic toolbar
59 bytes, text/x-review-board-request
droeh
: review+
Details
Bug 1358805 - part 5: Update the dynamic toolbar animator to gracefully handle toolbar snapshot generation failure
59 bytes, text/x-review-board-request
kats
: review+
Details 
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.
Blocks: dynamic-toolbar-3
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
Keywords: regression
OS: Unspecified → Android
Priority: -- → P3
Hardware: Unspecified → All
Version: unspecified → 55 Branch
(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.
Attachment #8861163 - Flags: feedback?(rbarker)
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.
Attachment #8861163 - Flags: feedback?(rbarker)
Attachment #8861163 - Attachment is obsolete: true
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)
Attachment #8861493 - Flags: review?(bugmail) → review-
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.
Attachment #8861497 - Flags: review?(bugmail) → review-
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.
Attachment #8861498 - Attachment is obsolete: true
Attachment #8861498 - Flags: review?(droeh)
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.
Attachment #8861496 - Flags: review?(droeh) → review+
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
}
Attachment #8861495 - Flags: review?(bugmail) → review+
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!
Attachment #8861497 - Flags: review?(bugmail) → review+
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
Attachment #8861494 - Flags: review?(bugmail) → review+
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
Attachment #8861493 - Flags: review?(bugmail) → review+
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.
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

Creator:
Created:
Updated:
Size: