Closed Bug 1335895 (dynamic-toolbar-3) Opened 3 years ago Closed 3 years ago

GeckoView Dynamic Toolbar

Categories

(GeckoView :: General, defect)

Unspecified
Android
defect
Not set

Tracking

(firefox54 wontfix, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

References

(Blocks 2 open bugs)

Details

Attachments

(37 files, 41 obsolete files)

59 bytes, text/x-review-board-request
dvander
: review+
Details
59 bytes, text/x-review-board-request
dvander
: review+
Details
59 bytes, text/x-review-board-request
dvander
: review+
Details
59 bytes, text/x-review-board-request
dvander
: review+
Details
59 bytes, text/x-review-board-request
dvander
: review+
Details
59 bytes, text/x-review-board-request
dvander
: review+
Details
59 bytes, text/x-review-board-request
dvander
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
59 bytes, text/x-review-board-request
dvander
: review+
Details
59 bytes, text/x-review-board-request
dvander
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
kats
: review+
Details
59 bytes, text/x-review-board-request
jchen
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
Problem: The dynamic toolbar and compositor operate in different threads. Each thread is responsible for drawing its content synchronized with the other. As the toolbar and surface are moved in the UI Java thread, the compositor must adjust the content rendered to the surface so that the entire exposed surface is filled. In order for the two threads to remain synchronized, the frame metrics from the compositor must be shared with the UI thread every frame and the UI thread provides the fixed frame margins. This is accomplished by calling nsWidget::SyncFrameMetrics() from the compositor thread. This works as long as both threads reside in the same process. With the addition of the GPU process on Android, they no longer exist in the same process. A new solution needs to be found to enable the dynamic toolbar to function smoothly across the two processes.

Proposed Solution: Currently the toolbar animation is handled by the Java class DynamicToolbarAnimator. All UI element placement is handled by an instance of this class. Moving the animation of the toolbar to the compositor thread would eliminate the need to synchronize the UI thread with the compositor thread.

How: A new C++ version of the DynamicToolbarAnimator class would be created. It would be instantiated and live in the compositor thread. The live toolbar UI elements would still live in the UI thread. However, instead of animating the actual UI elements, the UI thread would only manage the binary state of visible and not visible. Instead of animating the toolbar UI elements, the C++ DynamicToolbarAnimator would animate a static snapshot of the toolbar that is shared from the UI thread to the compositor thread via IPC. This is possible because the toolbar should not visually change while it is being animated. An additional change would be in the placement of the surface view. Currently the surface is created with the same dimensions as the screen and then shifted down to accommodate the toolbar when it is visible. The fixed frame margin ensures that fixed position elements and scrollbars appear in the proper location. By moving the toolbar animator to the compositor thread, it would no longer be necessary to shift the surface. Instead, the Toolbar UI elements would be rendered on top of the surface and the compositor would render to the portion of surface that is not obstructed by the toolbar. 

Hiding the toolbar would work as follows:

1) Content is scrolled so the UI thread passes a snapshot of the toolbar to the compositor.

2) The compositor starts rendering the snapshot underneath the UI toolbar.

3) The compositor thread notifies the UI thread that the static snapshot is being rendered and the UI thread hides the overlaid toolbar so that the static snapshot is visible.

4) The UI thread notifies the compositor thread that the UI toolbar has been hidden. This lets the C++ DynamicToolbarAnimator know it is allowed to animate static snapshot.

5) The C++ DynamicToolbarAnimator intercepts input events in APZCTreeManager::ReceiveInputEvent and moves the static snapshot offscreen while at the same time expanding the fixed frame margin to completely cover the remainder of the surface.

6) Once the static toolbar is hidden, The viewport is resized and events are processed as usual.

Revealing the toolbar would work as follows:

1) The UI toolbar elements start hidden and the static snapshot scrolled off the screen.

2) The user swipes or drags down.

3) The C++ DynamicToolbarAnimator intercepts input events in APZCTreeManager::ReceiveInputEvent and moves the static snapshot onscreen while at the same time resizing the fixed frame margin so that all fixed elements will remain visible.

4) Once the static snapshot is fully visible, the static snapshot is pinned and the UI thread is notified to show the UI toolbar elements. The viewport is resized at this time.

Event translation: Since the content will not always be rendered up at the top edge of the surface widget, events will need to be translated to accommodate for the content displacement. The C++ DynamicToolbarAnimator would apply this translation when it intercepts the event in APZCTreeManager::ReceiveInputEvent.

Where the static snapshot is rendered: The C++ DynamicToolbarAnimator would have the additional task of adding a translation prior to rendering the content in LayerManagerComposite::Render if the static snapshot is visible. The static snapshot would also be blitted at this point.

UI thread takes back control: The UI thread will be able to at any point request that the static toolbar be pinned visible. Once the UI thread has been informed that the static snapshot is visible, the UI toolbar elements may be shown again. The C++ DynamicToolbarAnimator must get permission from the UI thread before it may hide and/or animate the static snapshot again.

Toolbar display flow control: When a page is loaded, the UI toolbar elements are displayed and the C++ DynamicToolbarAnimator is offsetting content so it is rendered starting below the bottom of the toolbar. The C++ DynamicToolbarAnimator is in a state that prevents it from animating the toolbar at this time. The C++ DynamicToolbarAnimator is intercepting events in APZCTreeManager::ReceiveInputEvent so they may be translated. Once content begins to scroll, the compositor thread must notify the UI thread that it wishes to animate the toolbar. The UI thread responds by sending a static snapshot of the UI toolbar to the compositor thread. This may be accomplished by either sending a Surface over the binder or copying the content of a Surface into shared memory and then shared via IPDL. Once the compositor thread has received the static snapshot and render it once, the compositor thread notifies the UI thread that it is now rendering the static snapshot underneath the UI toolbar elements. At this point the compositor thread is still in a state the prevents it from begin to animate the static snapshot even if the user is scrolling. It must wait for the UI thread to notify it back that the UI toolbar has been hidden. The UI thread may not immediately notify the compositor thread that it may begin to animate. The UI thread may want the toolbar to remain visible until certain conditions are met such as if the page load progress bar is still visible. Once the UI thread determines that the compositor thread may start animating the static snapshot, it hides the toolbar and notifies the compositor thread that it may start animating. The compositor thread may not immediately start animating the static snapshot if at the time it receives the notification there are no user events that would cause the static snapshot to animate.

Fullscreen: When a page goes full screen, some coordination may be required to ensure a blank space is not visible when the UI toolbar is hidden.

Copy/Cut/Paste on devices with a CCP Toolbar: On some Android devices (older versions of android, CyanogenMod, etc), the copy/cut/paste menu is a toolbar across the top of the page instead of a floating toolbar. Additional coordination might be need to ensure it displays properly. It is important that the content does not shift under the user’s finger when text is being selected.

Sharing the FrameMetric with the UI thread: The UI thread will still need a copy of the current FrameMetric values. However, it will not need to be updated in real time. All UI elements that are positioned based on the current FrameMetric values are hidden during a scroll. Thus, the UI thread only needs the FrameMetric values when they are in a steady state. This means the compositor thread only needs to update when a scroll or fling completes. The UI thread would probably need to keep track of when the FrameMetric becomes stale and not display floating UI elements until the FrameMetric has been updated from the compositor thread.

Partially visible static snapshot: If the user removes their finger before a drag has completely hidden or revealed the static snapshot, the C++ DynamicToolbarAnimator will need to finish either hiding or revealing the static snapshot. The heuristic will mirror what is currently done in the Java DynamicToolbarAnimator. It was suggested that the C++ DynamicToolbarAnimator would be able to use the Vsync notification to advance the animation once there are no longer user events to drive the translation of the static snapshot.

GeckoView API: GeckoView consumers will be presented with a very simple API for dynamic toolbar support. The consumer registers a toolbar with GeckoView. GeckoView will be responsible for managing visibility of the toolbar. The consumer will be able to request the toolbar be made visible at any time and choose if it is visible instantly or animated. A callback maybe provide to notify the consumer when the toolbar is made visible or invisible.

Scroll to end-of-page special case: When the user scrolls to the end of a page, the UI toolbar should be made visible again.

1) C++ DynamicToolbarAnimator is notified the end of page has been reached.

2) The static snapshot is animated into view. It is important that the bottom of the page remain at the bottom of the surface so that the content does not appear to shift off the screen.

3) Once the static snapshot is visible, the UI thread is notified to show the UI toolbar elements.
Assignee: nobody → rbarker
Blocks: 1331109
Comment on attachment 8836268 [details] [diff] [review]
0001-Convert-UiCompositorController-from-singleton-to-one-per-CompositorSession-17021015-e245404.patch

For the dynamic toolbar rework, I need to be able to communicate from the compositor thread back to the UI thread. It is easier to do this if each CompositorSession has a UiCompositorControllerChild instance instead of a singleton. Is this the correct approach? Do you see any issues that may have been introduced? Sorry, git diff made a mess of this patch.
Attachment #8836268 - Flags: feedback?(dvander)
Attachment #8836268 - Attachment is obsolete: true
Attachment #8836268 - Flags: feedback?(dvander)
Attachment #8856017 - Attachment is obsolete: true
Attachment #8856018 - Attachment is obsolete: true
Attachment #8855981 - Attachment is obsolete: true
Attachment #8855981 - Flags: review?(dvander)
Attachment #8855982 - Attachment is obsolete: true
Attachment #8855982 - Flags: review?(dvander)
Attachment #8855983 - Attachment is obsolete: true
Attachment #8855983 - Flags: review?(dvander)
Attachment #8855984 - Attachment is obsolete: true
Attachment #8855984 - Flags: review?(dvander)
Attachment #8855985 - Attachment is obsolete: true
Attachment #8855985 - Flags: review?(dvander)
Attachment #8855986 - Attachment is obsolete: true
Attachment #8855986 - Flags: review?(dvander)
Attachment #8855987 - Attachment is obsolete: true
Attachment #8855987 - Flags: review?(dvander)
Attachment #8855988 - Attachment is obsolete: true
Attachment #8855988 - Flags: review?(dvander)
Attachment #8855989 - Attachment is obsolete: true
Attachment #8855989 - Flags: review?(dvander)
Attachment #8855990 - Attachment is obsolete: true
Attachment #8855990 - Flags: review?(dvander)
Attachment #8855991 - Attachment is obsolete: true
Attachment #8855991 - Flags: review?(bugmail)
Attachment #8855991 - Flags: review?(botond)
Attachment #8855992 - Attachment is obsolete: true
Attachment #8855992 - Flags: review?(bugmail)
Attachment #8855992 - Flags: review?(botond)
Attachment #8855993 - Attachment is obsolete: true
Attachment #8855993 - Flags: review?(bugmail)
Attachment #8855993 - Flags: review?(botond)
Attachment #8855994 - Attachment is obsolete: true
Attachment #8855994 - Flags: review?(bugmail)
Attachment #8855994 - Flags: review?(botond)
Attachment #8855995 - Attachment is obsolete: true
Attachment #8855995 - Flags: review?(bugmail)
Attachment #8855995 - Flags: review?(botond)
Attachment #8855996 - Attachment is obsolete: true
Attachment #8855996 - Flags: review?(bugmail)
Attachment #8855996 - Flags: review?(botond)
Attachment #8855997 - Attachment is obsolete: true
Attachment #8855997 - Flags: review?(bugmail)
Attachment #8855997 - Flags: review?(botond)
Attachment #8855998 - Attachment is obsolete: true
Attachment #8855998 - Flags: review?(bugmail)
Attachment #8855998 - Flags: review?(botond)
Attachment #8855999 - Attachment is obsolete: true
Attachment #8855999 - Flags: review?(bugmail)
Attachment #8856000 - Attachment is obsolete: true
Attachment #8856000 - Flags: review?(nchen)
Attachment #8856000 - Flags: review?(bugmail)
Attachment #8856001 - Attachment is obsolete: true
Attachment #8856001 - Flags: review?(nchen)
Attachment #8856001 - Flags: review?(bugmail)
Attachment #8856002 - Attachment is obsolete: true
Attachment #8856002 - Flags: review?(nchen)
Attachment #8856002 - Flags: review?(bugmail)
Attachment #8856003 - Attachment is obsolete: true
Attachment #8856003 - Flags: review?(nchen)
Attachment #8856004 - Attachment is obsolete: true
Attachment #8856004 - Flags: review?(nchen)
Attachment #8856004 - Flags: review?(bugmail)
Attachment #8856005 - Attachment is obsolete: true
Attachment #8856005 - Flags: review?(nchen)
Attachment #8856005 - Flags: review?(bugmail)
Attachment #8856006 - Attachment is obsolete: true
Attachment #8856006 - Flags: review?(nchen)
Attachment #8856006 - Flags: review?(bugmail)
Attachment #8856007 - Attachment is obsolete: true
Attachment #8856007 - Flags: review?(nchen)
Attachment #8856007 - Flags: review?(bugmail)
Attachment #8856008 - Attachment is obsolete: true
Attachment #8856008 - Flags: review?(nchen)
Attachment #8856008 - Flags: review?(bugmail)
Attachment #8856009 - Attachment is obsolete: true
Attachment #8856009 - Flags: review?(nchen)
Attachment #8856009 - Flags: review?(bugmail)
Attachment #8856010 - Attachment is obsolete: true
Attachment #8856010 - Flags: review?(nchen)
Attachment #8856010 - Flags: review?(bugmail)
Attachment #8856011 - Attachment is obsolete: true
Attachment #8856011 - Flags: review?(nchen)
Attachment #8856011 - Flags: review?(bugmail)
Attachment #8856012 - Attachment is obsolete: true
Attachment #8856012 - Flags: review?(nchen)
Attachment #8856012 - Flags: review?(bugmail)
Attachment #8856013 - Attachment is obsolete: true
Attachment #8856013 - Flags: review?(nchen)
Attachment #8856013 - Flags: review?(bugmail)
Attachment #8856014 - Attachment is obsolete: true
Attachment #8856014 - Flags: review?(nchen)
Attachment #8856014 - Flags: review?(bugmail)
Attachment #8856015 - Attachment is obsolete: true
Attachment #8856015 - Flags: review?(nchen)
Attachment #8856015 - Flags: review?(bugmail)
Attachment #8856016 - Attachment is obsolete: true
Attachment #8856016 - Flags: review?(nchen)
Comment on attachment 8856021 [details]
Bug 1335895 - part 3: Update PGPU InitUiCompositorController to include the root layer tree ID

https://reviewboard.mozilla.org/r/127934/#review130986

::: gfx/ipc/GPUParent.cpp:237
(Diff revision 1)
>  }
>  
>  mozilla::ipc::IPCResult
> -GPUParent::RecvInitUiCompositorController(Endpoint<PUiCompositorControllerParent>&& aEndpoint)
> +GPUParent::RecvInitUiCompositorController(const uint64_t& aRootLayerTreeId, Endpoint<PUiCompositorControllerParent>&& aEndpoint)
>  {
> -  UiCompositorControllerParent::Start(Move(aEndpoint));
> +  UiCompositorControllerParent::Start(aRootLayerTreeId, Move(aEndpoint));

This patch isn't going to compile, the Start function hasn't been updated...
Comment on attachment 8856041 [details]
Bug 1335895 - part 23: Added ArrayRefBase::CopyTo()

https://reviewboard.mozilla.org/r/127974/#review131014

::: widget/android/jni/Refs.h:874
(Diff revision 1)
> +                      "Size of native type must match size of JNI type");
> +
> +        const size_t len = size_t(Base::Env()->GetArrayLength(Base::Instance()));
> +        const size_t amountToCopy = (len > size ? size : len);
> +        (Base::Env()->*detail::TypeAdapter<ElementType>::GetArray)(
> +                Base::Instance(), 0, (jsize)amountToCopy,

`jsize(amountToCopy)` or static_cast
Attachment #8856041 - Flags: review?(nchen) → review+
Comment on attachment 8856038 [details]
Bug 1335895 - part 20: Update nsIWidget and nsBaseWidget with four Android only virtual functions used by UiCompositorControllerChild

https://reviewboard.mozilla.org/r/127968/#review131016

I think these should go in nsIWidget.h, because nsBaseWidget is supposed to be a template implementation of nsIWidget. Also, since they're ifdef'd to Android, you can make them pure virtual so they must be implemented somewhere.
Attachment #8856038 - Flags: review?(nchen)
Comment on attachment 8856024 [details]
Bug 1335895 - part 6: Update UiCompositorControllerChild to support added methods and to be instanced per session instead of a singleton

https://reviewboard.mozilla.org/r/127940/#review131022

::: gfx/layers/ipc/UiCompositorControllerChild.h:77
(Diff revision 1)
> +  // Should only be set when compositor is in process.
> +  RefPtr<UiCompositorControllerParent> mParent;

I find this kind of strange, that the child is holding a RefPtr to the parent. If it doesn't need it for the OOP case it shouldn't need it for the in-process case. Having different behaviours/codepaths for the two cases is not ideal.
Comment on attachment 8856024 [details]
Bug 1335895 - part 6: Update UiCompositorControllerChild to support added methods and to be instanced per session instead of a singleton

https://reviewboard.mozilla.org/r/127940/#review131022

> I find this kind of strange, that the child is holding a RefPtr to the parent. If it doesn't need it for the OOP case it shouldn't need it for the in-process case. Having different behaviours/codepaths for the two cases is not ideal.

This how it is done in the CompositorBridgeChild which I "copied":

https://dxr.mozilla.org/mozilla-central/rev/45692c884fdd5136a64fb2f8a61a0c8183b69331/gfx/layers/ipc/CompositorBridgeChild.cpp#255
Comment on attachment 8856039 [details]
Bug 1335895 - part 21: Update nsWindow to support update UiCompositorControllerChild

https://reviewboard.mozilla.org/r/127970/#review131024

::: widget/android/nsWindow.cpp:1064
(Diff revision 1)
> +                                   nsIThread::DISPATCH_NORMAL);
> +            }
> +            return;
> +        }
> +
> +        MOZ_ASSERT(AndroidBridge::IsJavaUiThread());

Redundant assert

::: widget/android/nsWindow.cpp:1085
(Diff revision 1)
> +        RefPtr<UiCompositorControllerChild> child;
> +        if (LockedWindowPtr window{mWindow}) {
> +            child = window->GetUiCompositorControllerChild();
> +        }

Maybe you should move this idiom into a helper function.
Attachment #8856039 - Flags: review?(nchen) → review+
Comment on attachment 8856040 [details]
Bug 1335895 - part 22: Update AndroidCompositorWidget to remove frame metrics sync

https://reviewboard.mozilla.org/r/127972/#review131028
Attachment #8856040 - Flags: review?(nchen) → review+
Comment on attachment 8856038 [details]
Bug 1335895 - part 20: Update nsIWidget and nsBaseWidget with four Android only virtual functions used by UiCompositorControllerChild

https://reviewboard.mozilla.org/r/127968/#review131016

Doing this means I need to add stubs to MockWidget and PuppetWidget. I can do that but seems to spread the MOZ_WIDGET_ANDROID over move files which I was trying to avoid.
Comment on attachment 8856038 [details]
Bug 1335895 - part 20: Update nsIWidget and nsBaseWidget with four Android only virtual functions used by UiCompositorControllerChild

https://reviewboard.mozilla.org/r/127968/#review131016

Also HeadlessWidget.
Comment on attachment 8856038 [details]
Bug 1335895 - part 20: Update nsIWidget and nsBaseWidget with four Android only virtual functions used by UiCompositorControllerChild

https://reviewboard.mozilla.org/r/127968/#review131016

So, I put pure virtual version in nsIWidget.h and left the stubs in nsBaseWidget.h so I didn't have to modify MockWidget, PuppetWidget, and HeadlessWidget.
Comment on attachment 8856029 [details]
Bug 1335895 - part 11: Add Translate function to MultiTouchData

https://reviewboard.mozilla.org/r/127950/#review131050

r+ with PinchGestureInput::Translate() implemented.

::: widget/InputData.cpp:187
(Diff revision 1)
>  }
>  
> +void
> +MultiTouchInput::Translate(const ScreenPoint& aTranslation)
> +{
> +  const int32_t xTranslation = (int32_t)aTranslation.x;

Are we truncating instead of rounding on purpose?

::: widget/InputData.cpp:608
(Diff revision 1)
>  }
>  
> +void
> +PinchGestureInput::Translate(const ScreenPoint& aTranslation)
> +{
> +  // No op?

mFocusPoint should probably be translated
Attachment #8856029 - Flags: review?(botond) → review+
Comment on attachment 8856029 [details]
Bug 1335895 - part 11: Add Translate function to MultiTouchData

https://reviewboard.mozilla.org/r/127950/#review131094

::: widget/InputData.h:100
(Diff revision 1)
>    INPUTDATA_AS_CHILD_TYPE(ScrollWheelInput, SCROLLWHEEL_INPUT)
>  
>    virtual ~InputData();
>    explicit InputData(InputType aInputType);
>  
> +  // Due to static_casts in the code, this can not be pure virtual.

As per IRC discussion, this can be made a pure virtual if the affected static_asserts are changed to target |const InputData&| rather than |InputData|.
Comment on attachment 8856019 [details]
Bug 1335895 - part 1: Update CompositorSession to hold a pointer to UiCompositorControllerChild on Android

https://reviewboard.mozilla.org/r/127930/#review131208
Attachment #8856019 - Flags: review?(dvander) → review+
Comment on attachment 8856020 [details]
Bug 1335895 - part 2: Update remote and in-process compositor sessions to destroy UiCompositorControllerChild on Android

https://reviewboard.mozilla.org/r/127932/#review131210
Attachment #8856020 - Flags: review?(dvander) → review+
Comment on attachment 8856038 [details]
Bug 1335895 - part 20: Update nsIWidget and nsBaseWidget with four Android only virtual functions used by UiCompositorControllerChild

https://reviewboard.mozilla.org/r/127968/#review131206

::: widget/nsBaseWidget.h:739
(Diff revision 2)
>  
>    static bool debug_GetCachedBoolPref(const char* aPrefName);
>  #endif
> +#if defined(MOZ_WIDGET_ANDROID)
> +public:
> +    virtual void RecvToolbarAnimatorMessageFromCompositor(int32_t) override {};

`override` implies `virtual` so you don't need `virtual` here.

::: widget/nsIWidget.h:2041
(Diff revision 2)
>      nsIWidget* MOZ_NON_OWNING_REF mPrevSibling;
>      // When Destroy() is called, the sub class should set this true.
>      bool mOnDestroyCalled;
>      nsWindowType mWindowType;
>      int32_t mZIndex;
> +#if defined(MOZ_WIDGET_ANDROID)

I think this should be put next to the other virtual functions in nsIWidget.
Attachment #8856038 - Flags: review?(nchen) → review+
Comment on attachment 8856019 [details]
Bug 1335895 - part 1: Update CompositorSession to hold a pointer to UiCompositorControllerChild on Android

https://reviewboard.mozilla.org/r/127930/#review131214

::: gfx/ipc/CompositorSession.h:85
(Diff revision 2)
> +  // doesn't get mucked up for other platforms.
> +  virtual void SetUiCompositorControllerChild(RefPtr<UiCompositorControllerChild> aUiController) {
> +    mUiCompositorControllerChild = aUiController;
> +  }
> +
> +  virtual RefPtr<UiCompositorControllerChild> GetUiCompositorControllerChild() {

Nit: It doesn't look like virtual is needed on either of these functions, but I might be missing something later down in the patch queue.
Comment on attachment 8856021 [details]
Bug 1335895 - part 3: Update PGPU InitUiCompositorController to include the root layer tree ID

https://reviewboard.mozilla.org/r/127934/#review131534

I realize it's too late for a patch queue this big, but it's hard to review a big change when it's split into non-functional units. Here, I can't tell how the layer id is being used without reading ahead/back in the queue.
Attachment #8856021 - Flags: review?(dvander) → review+
Comment on attachment 8856022 [details]
Bug 1335895 - part 4: Update GPUProcessManager so that it creates a new UiCompositorControllerChild for each session

https://reviewboard.mozilla.org/r/127936/#review131540

::: gfx/ipc/GPUProcessManager.cpp:578
(Diff revision 2)
> -    aSurfaceSize);
> +      aSurfaceSize);
> -}
> +  }
>  
> +#if defined(MOZ_WIDGET_ANDROID)
> +  if (session) {
> +    session->SetUiCompositorControllerChild(CreateUiCompositorController(aWidget, session->RootLayerTreeId()));

nit: I'd like to see CreateUiCompositorController assign to a temporary RefPtr or variable first, just to make it easier to read/debug/change. Also a super brief comment about not null checking the return value (it looks okay, more to assure the reader.)
Attachment #8856022 - Flags: review?(dvander) → review+
Comment on attachment 8856021 [details]
Bug 1335895 - part 3: Update PGPU InitUiCompositorController to include the root layer tree ID

https://reviewboard.mozilla.org/r/127934/#review131534

Yeah, I tried to keep it broken up functionally initially, but after about 60+ patches, I just couldn't make it work. Fixing older patches with broken code became unmanagable for me.
Comment on attachment 8856038 [details]
Bug 1335895 - part 20: Update nsIWidget and nsBaseWidget with four Android only virtual functions used by UiCompositorControllerChild

https://reviewboard.mozilla.org/r/127968/#review131206

> I think this should be put next to the other virtual functions in nsIWidget.

Sure, I can. I was previously told to always added new functions and member vairiables to the end of the class. This is the sort of thing a platform wide and **enforced** style guide would help with :)
Comment on attachment 8856019 [details]
Bug 1335895 - part 1: Update CompositorSession to hold a pointer to UiCompositorControllerChild on Android

https://reviewboard.mozilla.org/r/127930/#review131214

> Nit: It doesn't look like virtual is needed on either of these functions, but I might be missing something later down in the patch queue.

They don't need to be virtual, it was just a careless copy and paste error.
Comment on attachment 8856024 [details]
Bug 1335895 - part 6: Update UiCompositorControllerChild to support added methods and to be instanced per session instead of a singleton

https://reviewboard.mozilla.org/r/127940/#review131546

::: gfx/layers/ipc/UiCompositorControllerChild.cpp:328
(Diff revision 2)
> +}
> +
> +UiCompositorControllerChild::~UiCompositorControllerChild()
> +{
> +}
> +void

nit: newline above void
Attachment #8856024 - Flags: review?(dvander) → review+
Comment on attachment 8856023 [details]
Bug 1335895 - part 5: update PUiCompositorController to contain methods needed to support compositor based dynamic toolbar

https://reviewboard.mozilla.org/r/127938/#review131550
Attachment #8856023 - Flags: review?(dvander) → review+
FWIW, git makes rebasing large patch queues - as well as splitting up large patches - almost trivial. I can see how this would be difficult to manage in Mercurial.
(In reply to David Anderson [:dvander] from comment #173)
> FWIW, git makes rebasing large patch queues - as well as splitting up large
> patches - almost trivial. I can see how this would be difficult to manage in
> Mercurial.

I use git and it wasn't the rebasing that was the problem, it was as the code evolved over time I need to "fix" previous patches in the queue, but the conflicts became maddening. You would have been reviewing chunks of code that were later deleted and/or heavily modified. I eventually just gave up.
Comment on attachment 8856025 [details]
Bug 1335895 - part 7: Update UiCompositorControllerParent to support added methods and to be instanced per session instead of a single instance

https://reviewboard.mozilla.org/r/127942/#review131560

::: gfx/layers/ipc/UiCompositorControllerParent.cpp:127
(Diff revision 2)
> +}
> +
> +mozilla::ipc::IPCResult
> +UiCompositorControllerParent::RecvDefaultClearColor(const uint32_t& aColor)
> +{
> +

nit: extra newline

::: gfx/layers/ipc/UiCompositorControllerParent.cpp:237
(Diff revision 2)
> +{
> +  MOZ_COUNT_DTOR(UiCompositorControllerParent);
>  }
>  
>  void
> -UiCompositorControllerParent::ShutdownImpl()
> +UiCompositorControllerParent::Initialize()

If this is used in the in-process case, it would be cleaner to expose two methods, InitRemote and InitSameProcess or something (they could still call into a private shared function).

::: gfx/layers/ipc/UiCompositorControllerParent.cpp:239
(Diff revision 2)
>  }
>  
>  void
> -UiCompositorControllerParent::ShutdownImpl()
> +UiCompositorControllerParent::Initialize()
>  {
> -  Close();
> +  if (!CompositorThreadHolder::IsInCompositorThread()) {

There are a few cases like this where we check if we're in the compositor thread. Could you put a comment on each one, noting why it can occur on a different thread? (Even better if it only occurs on exactly one thread, and that can be asserted.)

::: gfx/layers/ipc/UiCompositorControllerParent.cpp:272
(Diff revision 2)
> -  Release();
> +  if (!CompositorThreadHolder::IsInCompositorThread()) {
> +    if (mSelfShutdownRef) {
> +      // we have already dispatch Shutdown to the compositor thread
> +      return;
> +    }
> +    mSelfShutdownRef = this;

Is this self ref necessary if the message loop holds a ref?
Attachment #8856025 - Flags: review?(dvander) → review+
Would it have made sense to fold them all into a few giant patches at the end, then reset & add -p to break them back into smaller units?
Comment on attachment 8856026 [details]
Bug 1335895 - part 8: Add UiCompositorControllerMessageTypes.h

https://reviewboard.mozilla.org/r/127944/#review131564

::: gfx/layers/ipc/CompositorBridgeParent.h:631
(Diff revision 2)
>    // visible again even if their geometry has not changed.
>    bool mPluginWindowsHidden;
>  #endif
>  
>    DISALLOW_EVIL_CONSTRUCTORS(CompositorBridgeParent);
> +#if defined(MOZ_WIDGET_ANDROID)

nit: newline above #if, and move this into an area that's already public or android-y

::: gfx/layers/ipc/CompositorBridgeParent.h:634
(Diff revision 2)
>  
>    DISALLOW_EVIL_CONSTRUCTORS(CompositorBridgeParent);
> +#if defined(MOZ_WIDGET_ANDROID)
> +public:
> +  gfx::IntSize GetEGLSurfaceSize()
> +  {

nit: { on newline (here and below too)
Attachment #8856026 - Flags: review?(dvander) → review+
Comment on attachment 8856027 [details]
Bug 1335895 - part 9: Update CompositorBridgeParent::LayerTreeState to have pointer to UiCompositorControllerParent

https://reviewboard.mozilla.org/r/127946/#review131570
Attachment #8856027 - Flags: review?(dvander) → review+
Comment on attachment 8856028 [details]
Bug 1335895 - part 10: Remove singleton shutdown of UiCompositorControllerChild from gfxPlatform::ShutdownLayersIPC now that UiCompositorControllerChild is per session

https://reviewboard.mozilla.org/r/127948/#review131572
Attachment #8856028 - Flags: review?(dvander) → review+
Comment on attachment 8856026 [details]
Bug 1335895 - part 8: Add UiCompositorControllerMessageTypes.h

https://reviewboard.mozilla.org/r/127944/#review131564

> nit: { on newline (here and below too)

Are you asking me to do this:

uint64_t GetRootLayerTreeId() {
  return mRootLayerTreeID;
}
Comment on attachment 8856025 [details]
Bug 1335895 - part 7: Update UiCompositorControllerParent to support added methods and to be instanced per session instead of a single instance

https://reviewboard.mozilla.org/r/127942/#review131560

> If this is used in the in-process case, it would be cleaner to expose two methods, InitRemote and InitSameProcess or something (they could still call into a private shared function).

UiCompositorControllerParent::Initialize() is already private. I'm not sure I understand the value of having two private functions that just wrap another private function.
Comment on attachment 8856025 [details]
Bug 1335895 - part 7: Update UiCompositorControllerParent to support added methods and to be instanced per session instead of a single instance

https://reviewboard.mozilla.org/r/127942/#review131560

> UiCompositorControllerParent::Initialize() is already private. I'm not sure I understand the value of having two private functions that just wrap another private function.

Okay, I guess I made UiCompositorControllerChild a friend so it could call UiCompositorControllerParent::Initialize(). I'll have it call UiCompositorControllerParent::InitializeForSameProcess().