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

GeckoView Dynamic Toolbar

Categories

(GeckoView :: General, defect)

Product:
GeckoView
Component:
General
Platform:
Unspecified
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 wontfix, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

References

Details

Attachments

(37 files, 41 obsolete files)

Bug 1335895 - part 1: Update CompositorSession to hold a pointer to UiCompositorControllerChild on Android
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1335895 - part 2: Update remote and in-process compositor sessions to destroy UiCompositorControllerChild on Android
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1335895 - part 3: Update PGPU InitUiCompositorController to include the root layer tree ID
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1335895 - part 4: Update GPUProcessManager so that it creates a new UiCompositorControllerChild for each session
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1335895 - part 5: update PUiCompositorController to contain methods needed to support compositor based dynamic toolbar
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1335895 - part 6: Update UiCompositorControllerChild to support added methods and to be instanced per session instead of a singleton
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1335895 - part 7: Update UiCompositorControllerParent to support added methods and to be instanced per session instead of a single instance
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1335895 - part 8: Add UiCompositorControllerMessageTypes.h
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
Bug 1335895 - part 9: Update CompositorBridgeParent::LayerTreeState to have pointer to UiCompositorControllerParent
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1335895 - part 10: Remove singleton shutdown of UiCompositorControllerChild from gfxPlatform::ShutdownLayersIPC now that UiCompositorControllerChild is per session
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1335895 - part 11: Add Translate function to MultiTouchData
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
Bug 1335895 - part 13: Update APZCTreeManager to create AndroidDynamicToolbarAnimator
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
Bug 1335895 - part 14: Update LayerManagerComposite render static toolbar snapshot and handle robocop screen captures
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1335895 - part 15: Update AsyncPanZoomController to notify toolbar animator when FrameMetrics have changed and when root content is scrolled
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
Bug 1335895 - part 16: Update AsyncCompositionManager to support toolbar animator
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
Bug 1335895 - part 17: Remove SetScrollingRootContent from PAPZ, GeckoContentController, RemoteContentController, and AndroidContentController
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
Bug 1335895 - part 18: Update gfxPrefs to support browser.ui.scroll-toolbar-threshold
59 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
Bug 1335895 - part 19: Update WebRenderBridgeParent.cpp to fix the way the first paint event is sent to the Android UI thread
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1335895 - part 20: Update nsIWidget and nsBaseWidget with four Android only virtual functions used by UiCompositorControllerChild
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1335895 - part 21: Update nsWindow to support update UiCompositorControllerChild
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 22: Update AndroidCompositorWidget to remove frame metrics sync
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 23: Added ArrayRefBase::CopyTo()
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1335895 - part 24: Update LayerView.java support new DynamicToolbar
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 25: Update GeckoLayerClient.java to support new dynamic toolbar
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 26: Update NativePanZoomController.java to use LayerView.isGeckoReady and remove setScrollingRootContent
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 27: Update DynamicToolbarAnimator to work as a proxy to the AndroidDynamicToolbarAnimator
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 29: Update BrowserApp.java to work with new dynamic toolbar
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 30: Update GeckoApp and Tabs to remove code calling deleted function LayerView.setPaintState()
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 31: Update FloatingToolbarTextSelection, FormAssistPopup, OverscrollEdgeEffect, and GeckoInputConnection to use LayerView.getCurrentToolbarHeight()
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 32: Fix rendering of toolbar "curve" so that it works with software rendering
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1335895 - part 33: Update FennecNativeDriver.java so robocop tests work with new dynamic toolbar
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 34: Remove PanZoomTarget.java, ZoomedView.java, and LayerRenderer.java
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 35: Remove ZoomedView.java specific code from ThumbnailHelper.h and AndroidContentController
59 bytes, text/x-review-board-request
kats
: review+
jchen
: review+
Details
Bug 1335895 - part 28: Remove unused resources, prefs, and layout used by deleted ZoomedView.java
59 bytes, text/x-review-board-request
jchen
: review+
kats
: review+
Details
Bug 1335895 - part 36: update JNI bindings
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1335895 - part 37: Remove PaintSyncId
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
Alias: dynamic-toolbar-3
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().
Comment on attachment 8856042 [details] Bug 1335895 - part 24: Update LayerView.java support new DynamicToolbar https://reviewboard.mozilla.org/r/127976/#review132036 LGTM with following fixes ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:96 (Diff revision 3) > + /* package */ final static int REQUEST_HIDE_TOOLBAR_IMMEDIATELY = 8; > + /* package */ final static int REQUEST_HIDE_TOOLBAR_ANIMATED = 9; > + /* package */ final static int LAYER_UPDATED = 10; // Sent from compositor when a layer has been updated > + /* package */ final static int COMPOSITOR_CONTROLLER_OPEN = 20; // Special message sent from UiCompositorControllerChild once it is open > + > + private void postCompositorMessage (final int message) { No space before `(` ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:146 (Diff revision 3) > + > + @WrapForJNI(calledFrom = "ui", dispatchTo = "current") > + /* package */ native void sendToolbarAnimatorMessage(int aMessage); > + > + @WrapForJNI(calledFrom = "ui") > + /* package */ void recvToolbarAnimatorMessage(int aMessage) { I would move the implementation to another method in LayerView itself, and call that from here. ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:151 (Diff revision 3) > + /* package */ void recvToolbarAnimatorMessage(int aMessage) { > + switch(aMessage) { > + case STATIC_TOOLBAR_NEEDS_UPDATE: > + // Send updated toolbar image to compositor. > + Bitmap bm = mToolbarAnimator.getBitmapOfToolbarChrome(); > + if (bm != null) { if (bm == null) { break; } ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:158 (Diff revision 3) > + final int height = bm.getHeight(); > + int[] pixels = new int[bm.getByteCount() / 4]; > + try { > + bm.getPixels(pixels, /* offset */ 0, /* stride */ width, /* x */ 0, /* y */ 0, width, height); > + sendToolbarPixelsToCompositor(width, height, pixels); > + } catch (IllegalArgumentException e) { Catch all three in one block with a generic message, and log the error like this, `Log.e(LOGTAG, 'foo bar', e);` ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:202 (Diff revision 3) > + > + @WrapForJNI(calledFrom = "ui", dispatchTo = "current") > + /* package */ native void requestScreenPixels(); > + > + @WrapForJNI(calledFrom = "ui") > + /* package */ void recvScreenPixels(int aWidth, int aHeight, int aPixels[]) { No `a` prefix here and elsewhere, and should be `int[]` ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:203 (Diff revision 3) > + @WrapForJNI(calledFrom = "ui", dispatchTo = "current") > + /* package */ native void requestScreenPixels(); > + > + @WrapForJNI(calledFrom = "ui") > + /* package */ void recvScreenPixels(int aWidth, int aHeight, int aPixels[]) { > + if (mGetPixelsResult != null) { Make `mGetPixelsResult` package-private ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:219 (Diff revision 3) > + mCompositor.setDefaultClearColor(mDefaultClearColor); > + mCompositor.enableLayerUpdateNotifications(!mDrawListeners.isEmpty()); Is this tested? These are supposed to be called from UI thread but they're called from Gecko thread here. ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:232 (Diff revision 3) > LayerView.this.mCompositorCreated = false; > + LayerView.this.mToolbarAnimator.notifyCompositorDestroyed(); > > LayerView.this.mLayerClient.setGeckoReady(false); > > + LayerView.this.mDrawListeners.clear(); Used on Gecko thread ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:488 (Diff revision 3) > - > - /** Used by robocop for testing purposes. Not for production use! */ > @RobocopTarget > - public IntBuffer getPixels() { > - return mRenderer.getPixels(); > + public void getPixels(final GetPixelsResult aGetPixelsResult) { > + if (!ThreadUtils.isOnUiThread()) { > + mGotPixelsResultFromCompositor = false; `mGotPixelsResultFromCompositor` is never read from? ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:498 (Diff revision 3) > - /* paintState must be a PAINT_xxx constant. */ > + return; > - public void setPaintState(int paintState) { > - mPaintState = paintState; > - } > + } > > - public int getPaintState() { > + if (aGetPixelsResult != null) { Why would `aGetPixelsResult` ever be null? ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java (Diff revision 3) > } > > // This method is called on the Gecko main thread. > @WrapForJNI(calledFrom = "gecko") > private static void updateZoomedView(ByteBuffer data) { > - LayerView layerView = GeckoAppShell.getLayerView(); Add a comment here ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:747 (Diff revision 3) > + } > + > + boolean wasEmpty = mDrawListeners.isEmpty(); > + mDrawListeners.add(listener); > + if (mCompositorCreated && wasEmpty) { > + mCompositor.enableLayerUpdateNotifications(true); Indent ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:766 (Diff revision 3) > + } > + > + boolean notEmpty = mDrawListeners.isEmpty(); > + mDrawListeners.remove(listener); > + if (mCompositorCreated && notEmpty && mDrawListeners.isEmpty()) { > + mCompositor.enableLayerUpdateNotifications(false); Indent ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java (Diff revision 3) > public void requestZoomedViewRender(); > public void updateView(ByteBuffer data); > } > > public void addZoomedViewListener(ZoomedViewListener listener) { > - mRenderer.addZoomedViewListener(listener); Add a comment ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java (Diff revision 3) > public void addZoomedViewListener(ZoomedViewListener listener) { > - mRenderer.addZoomedViewListener(listener); > } > > public void removeZoomedViewListener(ZoomedViewListener listener) { > - mRenderer.removeZoomedViewListener(listener); Add a comment
Attachment #8856042 - Flags: review?(nchen) → review+
Comment on attachment 8856043 [details] Bug 1335895 - part 25: Update GeckoLayerClient.java to support new dynamic toolbar https://reviewboard.mozilla.org/r/127978/#review132154
Attachment #8856043 - Flags: review?(nchen) → review+
Comment on attachment 8856044 [details] Bug 1335895 - part 26: Update NativePanZoomController.java to use LayerView.isGeckoReady and remove setScrollingRootContent https://reviewboard.mozilla.org/r/127980/#review132156
Attachment #8856044 - Flags: review?(nchen) → review+
Comment on attachment 8856045 [details] Bug 1335895 - part 27: Update DynamicToolbarAnimator to work as a proxy to the AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127982/#review132158 LGTM with following fixes ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java:36 (Diff revision 3) > - private final GeckoLayerClient mTarget; > - private final List<LayerView.DynamicToolbarListener> mListeners; > > - /* The translation to be applied to the toolbar UI view. This is the > - * distance from the default/initial location (at the top of the screen, > - * visible to the user) to where we want it to be. This variable should > + private final int mValue; > + PinReason() { > + mValue = mMaxEnumValue; Manually pass a value into `PinReason()` instead of doing this. ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java:41 (Diff revision 3) > - * (toolbar fully hidden), inclusive. > + } > - */ > - private float mToolbarTranslation; > > - /* The translation to be applied to the LayerView. This is the distance from > - * the default/initial location (just below the toolbar, with the bottom > + public int getValue() { > + return mValue; Just make `value` a public field instead of using `getValue()` ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java:69 (Diff revision 3) > - } > - }; > - PrefsHelper.addObserver(new String[] { PREF_SCROLL_TOOLBAR_THRESHOLD }, mPrefObserver); > } > > public void destroy() { Remove? ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java:84 (Diff revision 3) > - for (LayerView.DynamicToolbarListener listener : mListeners) { > + mToolbarChromeProxy = aToolbarChromeProxy; > - listener.onTranslationChanged(mToolbarTranslation, mLayerViewTranslation); > - } > } > > - void onPanZoomStopped() { > + void onToggleChrome(boolean aShow) { `/* package-private */` here and elsewhere ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java:105 (Diff revision 3) > - mMaxTranslation = maxTranslation; > } > } > > - public float getMaxTranslation() { > - return mMaxTranslation; > + public int getCurrentToolbarHeight() { > + if (mToolbarChromeProxy != null) { `if (mToolbarChromeProxy != null && mToolbarChromeProxy.isToolbarChromeVisible()) {` ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java:174 (Diff revision 3) > - private final boolean mImmediate; > - private final boolean mShiftLayerView; > - private boolean mContinueAnimation; > > - public DynamicToolbarAnimationTask(float aTranslation, boolean aImmediate, boolean aShiftLayerView) { > - super(false); > + void notifyCompositorCreated(LayerView.Compositor aCompositor) { > + mCompositor = aCompositor; `mCompositor` changed from Gecko thread but accessed from UI thread elsewhere.
Attachment #8856045 - Flags: review?(nchen) → review+
Comment on attachment 8856046 [details] Bug 1335895 - part 29: Update BrowserApp.java to work with new dynamic toolbar https://reviewboard.mozilla.org/r/127984/#review132164
Attachment #8856046 - Flags: review?(nchen) → review+
Comment on attachment 8856047 [details] Bug 1335895 - part 30: Update GeckoApp and Tabs to remove code calling deleted function LayerView.setPaintState() https://reviewboard.mozilla.org/r/127986/#review132166 ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1649 (Diff revision 3) > } > > + // ToolbarChromeProxy inteface > @Override > - public void onMetricsChanged(ImmutableViewportMetrics aMetrics) { > - if (isHomePagerVisible() || mBrowserChrome == null) { > + public Bitmap getBitmapOfToolbarChrome() { > + if (mBrowserChrome != null) { if (mBrowserChrome == null) { return null; }
Attachment #8856047 - Flags: review?(nchen) → review+
Comment on attachment 8856048 [details] Bug 1335895 - part 31: Update FloatingToolbarTextSelection, FormAssistPopup, OverscrollEdgeEffect, and GeckoInputConnection to use LayerView.getCurrentToolbarHeight() https://reviewboard.mozilla.org/r/127988/#review132172
Attachment #8856048 - Flags: review?(nchen) → review+
Comment on attachment 8856045 [details] Bug 1335895 - part 27: Update DynamicToolbarAnimator to work as a proxy to the AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127982/#review132158 > `mCompositor` changed from Gecko thread but accessed from UI thread elsewhere. I fixed LayerView.java so it only calls notifyCompositorCreated on UI thread. But I'll add ThreadUtils.assertOnUiThread(); to make sure it remains only invoked on UI thread.
Comment on attachment 8856049 [details] Bug 1335895 - part 32: Fix rendering of toolbar "curve" so that it works with software rendering https://reviewboard.mozilla.org/r/127990/#review132234
Attachment #8856049 - Flags: review?(nchen) → review+
Comment on attachment 8856050 [details] Bug 1335895 - part 33: Update FennecNativeDriver.java so robocop tests work with new dynamic toolbar https://reviewboard.mozilla.org/r/127992/#review132236
Attachment #8856050 - Flags: review?(nchen) → review+
Comment on attachment 8856051 [details] Bug 1335895 - part 34: Remove PanZoomTarget.java, ZoomedView.java, and LayerRenderer.java https://reviewboard.mozilla.org/r/127994/#review132238 ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/FennecNativeDriver.java:199 (Diff revision 3) > + private int mPixelsHeight; > + private IntBuffer mPixelsResult; > + > + @Override > + public void onPixelsResult(int aWidth, int aHeight, IntBuffer aPixels) { > + synchronized (this) { Make method `synchronized` instead of `synchronized (this)` block. ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/FennecNativeDriver.java:210 (Diff revision 3) > + } > + } > + > + private static final int COLOR_DEVIATION = 3; > + > + private boolean differentColor(final int c1, final int c2) { static ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/FennecNativeDriver.java:237 (Diff revision 3) > + sb = new StringBuffer(); > + lastStart = agbr; > + } > + > + if ((sb != null) && differentColor(last, agbr)) { > + sb.append("("); Append single character in this case, `'('`, and you can chain append calls, `sb.append(foo).append(bar)` ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/FennecNativeDriver.java:296 (Diff revision 3) > + > + if ((pixelBuffer == null) || (w == 0) || (h == 0)) { > + return null; > + } > + > + // This function will output a human readable log of the returned pixels to the log. I feel you need a more complete description here, because you're only outputting pixels that change from previous pixels (and skipping lines that start with the same pixels).
Attachment #8856051 - Flags: review?(nchen) → review+
Comment on attachment 8856052 [details] Bug 1335895 - part 35: Remove ZoomedView.java specific code from ThumbnailHelper.h and AndroidContentController https://reviewboard.mozilla.org/r/127996/#review132246
Attachment #8856052 - Flags: review?(nchen) → review+
Comment on attachment 8856053 [details] Bug 1335895 - part 35: Update dimens.xml to remove unused resource https://reviewboard.mozilla.org/r/127998/#review132248
Attachment #8856053 - Flags: review?(nchen) → review+
Comment on attachment 8856054 [details] Bug 1335895 - part 28: Remove unused resources, prefs, and layout used by deleted ZoomedView.java https://reviewboard.mozilla.org/r/128000/#review132250
Attachment #8856054 - Flags: review?(nchen) → review+
Comment on attachment 8856051 [details] Bug 1335895 - part 34: Remove PanZoomTarget.java, ZoomedView.java, and LayerRenderer.java https://reviewboard.mozilla.org/r/127994/#review132256 We traditionally set a pref at https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/testing/mochitest/runrobocop.py#224, to avoid dynamic toolbar problems during robocop tests. Is that still relevant?
Attachment #8856051 - Flags: review?(gbrown) → review+
(In reply to Geoff Brown [:gbrown] from comment #233) > Comment on attachment 8856051 [details] > Bug 1335895 - part 33: Update FennecNativeDriver.java so robocop tests work > with new dynamic toolbar > > https://reviewboard.mozilla.org/r/127994/#review132256 > > We traditionally set a pref at > https://dxr.mozilla.org/mozilla-central/rev/ > f40e24f40b4c4556944c762d4764eace261297f5/testing/mochitest/runrobocop.py#224, > to avoid dynamic toolbar problems during robocop tests. Is that still > relevant? I believe all preexisting Dynamic Toolbar flags still work. The pref browser.chrome.dynamictoolbar=false specifically should continue to be set to prevent the Dynamic Toolbar from fouling up tests.
Comment on attachment 8856051 [details] Bug 1335895 - part 34: Remove PanZoomTarget.java, ZoomedView.java, and LayerRenderer.java https://reviewboard.mozilla.org/r/127994/#review132238 > Make method `synchronized` instead of `synchronized (this)` block. Fixed in patch 32 > static Fixed in patch 32 > Append single character in this case, `'('`, and you can chain append calls, `sb.append(foo).append(bar)` Fixed in patch 32 > I feel you need a more complete description here, because you're only outputting pixels that change from previous pixels (and skipping lines that start with the same pixels). Fixed in patch 32
Comment on attachment 8856047 [details] Bug 1335895 - part 30: Update GeckoApp and Tabs to remove code calling deleted function LayerView.setPaintState() https://reviewboard.mozilla.org/r/127986/#review132166 > if (mBrowserChrome == null) { > return null; > } Fixed in patch 28
Comment on attachment 8856026 [details] Bug 1335895 - part 8: Add UiCompositorControllerMessageTypes.h https://reviewboard.mozilla.org/r/127944/#review131564 > nit: newline above #if, and move this into an area that's already public or android-y Fixed in patch 9. > Are you asking me to do this: > > uint64_t GetRootLayerTreeId() { > return mRootLayerTreeID; > } I made this change in patch 9
Comment on attachment 8856019 [details] Bug 1335895 - part 1: Update CompositorSession to hold a pointer to UiCompositorControllerChild on Android https://reviewboard.mozilla.org/r/127928/#review132490 Here are some comments. I ended up reviewing on the squashed diff view in MozReview because it was easier than constantly jumping around in the patchset. I haven't yet finished the first pass but have enough comments so far that I figured you can get started on them. I don't know what will happen if you push updates to MozReview, it might throw away my comments. If you don't mind, make any changes locally for now and don't push to MozReview until I'm done my first pass through the current patchset. I'll let you know when I'm done. ::: gfx/ipc/GPUProcessManager.cpp:561 (Diff revision 4) > - if (session) { > + if (!session) { > - return session; > - } > - > - // We couldn't create a remote compositor, so abort the process. > + // We couldn't create a remote compositor, so abort the process. > - DisableGPUProcess("Failed to create remote compositor"); > + DisableGPUProcess("Failed to create remote compositor"); > - } > + } > - > - return InProcessCompositorSession::Create( > + } else { > + session = InProcessCompositorSession::Create( This code change seems wrong. In the previous code if the CreateRemoteSession call returned null it would fall through, disable the GPU process and execute InProcessCompositorSession::Create and return that. With your change it disables the GPU process but then doesn't create the in-process compositor session and ends up returning null unconditionally. ::: gfx/layers/apz/src/APZCTreeManager.h:533 (Diff revision 4) > +#if defined(MOZ_WIDGET_ANDROID) > + RefPtr<AndroidDynamicToolbarAnimator> mToolbarAnimator; > + > +public: > + void InitializeDynamicToolbarAnimator(const int64_t& aRootLayerTreeId) I'd prefer if you moved the mToolbarAnimator declaration down to below the functions (just before the #endif) and stuck an explicit "private:" above it. This just makes it more explicit that this is a separate section of APZCTreeManager that is android-only with its own public functions and private variables. ::: gfx/layers/apz/src/APZCTreeManager.h:538 (Diff revision 4) > + { > + MOZ_ASSERT(mToolbarAnimator); > + mToolbarAnimator->Initialize(aRootLayerTreeId); > + } > + AndroidDynamicToolbarAnimator* GetAndroidDynamicToolbarAnimator() > + { > + return mToolbarAnimator; Move the bodies of these functions into the .cpp file ::: gfx/layers/apz/src/APZCTreeManager.cpp:725 (Diff revision 4) > + if (isConsumed == nsEventStatus_eConsumeNoDefault) { > + return isConsumed; Just before the return statement here add a APZCTM_LOG("Dynamic toolbar consumed event"); ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:1 (Diff revision 4) > +#ifndef mozilla_layers_AndroidDynamicToolbarAnimator_h_ Missing license header. Same in .cpp file ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:42 (Diff revision 4) > + > +class AndroidDynamicToolbarAnimator { > +public: > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AndroidDynamicToolbarAnimator); > + static already_AddRefed<AndroidDynamicToolbarAnimator> Create(); > + void Initialize(const uint64_t aRootLayerTreeId); Either use "const uint64_t&" or just "uint64_t". Putting a const by itself is useless since it's pass-by-value anyway. Ditto for SetMaxToolbarHeight, SetPinned, and AdoptToolbarPixels ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:45 (Diff revision 4) > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AndroidDynamicToolbarAnimator); > + static already_AddRefed<AndroidDynamicToolbarAnimator> Create(); > + void Initialize(const uint64_t aRootLayerTreeId); > + nsEventStatus ReceiveInputEvent(InputData& aEvent); > + void SetMaxToolbarHeight(const int32_t aHeight); > + void SetPinned(const bool aPinned, const int32_t aReason); Add some docs on this function explaining how the reason is saved and that each pin reason needs to be unpinned before the toolbar as a whole will be unpinned. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:47 (Diff revision 4) > + void Initialize(const uint64_t aRootLayerTreeId); > + nsEventStatus ReceiveInputEvent(InputData& aEvent); > + void SetMaxToolbarHeight(const int32_t aHeight); > + void SetPinned(const bool aPinned, const int32_t aReason); > + int32_t GetMaxToolbarHeight(); > + int32_t GetCurrentToolbarHeight(); Docs needed on this function. My default interpretation is that this returns the height of the visible area of the toolbar, which could be anywhere between 0 and the "max toolbar height". It wouldn't hurt to add a few sentences to the class-level docs about the toolbar height and what particular values correspond to in terms of user-visible behaviour. Docs needed on GetCurrentSurfaceHeight() and GetCompositionHeight() as well. Based on what I've seen so far I don't have a mental model that lets me guess what those mean exactly. Also document at least SetScrollingRootContent(), FirstPaint(), EnableLayerUpdateNotifications() and NotifyLayerUpdated() since those names are all non-obvious. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:58 (Diff revision 4) > + void EnableLayerUpdateNotifications(bool aEnable); > + void NotifyLayerUpdated(); s/Layer/Layers/ in these function names. NotifyLayerUpdated begs the question - "which layer?". It's not really one layer, it's the whole layer tree. Hence "layers". ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:81 (Diff revision 4) > + AnimationStopPending > + }; > + > + AndroidDynamicToolbarAnimator(); > + ~AndroidDynamicToolbarAnimator(){} > + nsEventStatus ProcessTouchDelta(const StaticToolbarState aCurrentToolbarState, int32_t aDelta, uint32_t aTimeStamp); More things that should either be a const-ref or not const: in ProcessTouchDelta, in UpdateFrameMetrics, in CanHideToolbar, in ShowToolbarIfNotVisible ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:84 (Diff revision 4) > + AndroidDynamicToolbarAnimator(); > + ~AndroidDynamicToolbarAnimator(){} > + nsEventStatus ProcessTouchDelta(const StaticToolbarState aCurrentToolbarState, int32_t aDelta, uint32_t aTimeStamp); > + // Called when a touch ends > + void HandleTouchReset(const StaticToolbarState aCurrentToolbarState, int32_t aCurrentTouch); > + // Sends a message to the UI thread. Maybe called from any thread s/Maybe/May be/ ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:103 (Diff revision 4) > + const CSSToParentLayerScale aScale, > + const CSSRect aCssPageRect); > + bool CanHideToolbar(const int32_t delta); > + void ShowToolbarIfNotVisible(const StaticToolbarState aCurrentToolbarState); > + > + // Read only Compositor and Controller threads after Initialize() Nice, thanks for labeling all these variables with their relevant threads! ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:135 (Diff revision 4) > + > + // Returns true if any of the values have changed. > + bool Update(const ParentLayerPoint& aScrollOffset, > + const CSSToParentLayerScale& aScale, > + const CSSRect& aCssPageRect); > + } mControllerFrameMetrics; I'd prefer making the variable declaration a separate line from the struct definition. i.e. struct FrameMetricsState { .. }; // <insert thread-safety comment here> FrameMetricsState mControllerFrameMetrics; ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:7 (Diff revision 4) > +#include "gfxPrefs.h" > +#include "mozilla/EventForwards.h" > +#include "mozilla/FloatingPoint.h" > +#include "mozilla/MathAlgorithms.h" > +#include "mozilla/layers/APZThreadUtils.h" > +#include "mozilla/layers/AndroidDynamicToolbarAnimator.h" The .h file for this .cpp should be the first include, followed by a blank line, followed by all the other includes (thanks for sorting them!) ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:22 (Diff revision 4) > +static const int32_t STATIC_TOOLBAR_NEEDS_UPDATE = 0; // Sent from compositor when the static toolbar wants to hide. > +static const int32_t STATIC_TOOLBAR_READY = 1; // Sent from compositor when the static toolbar image has been updated and is ready to animate. Is there a reason you didn't put these in an enum? I feel like it would be cleaner than having a bunch of "untyped" int32_t things. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:62 (Diff revision 4) > + MOZ_ASSERT(uiController); > + uiController->RegisterAndroidDynamicToolbarAnimator(this); > +} > + > +static bool > +GetTouchValue(MultiTouchInput& multiTouch, int32_t* value) I'd prefer calling this GetTouchY since it only returns the y value ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:79 (Diff revision 4) > +AndroidDynamicToolbarAnimator::ReceiveInputEvent(InputData& aEvent) > +{ > + MOZ_ASSERT(APZThreadUtils::IsControllerThread()); > + nsEventStatus status = nsEventStatus_eIgnore; > + > + if (aEvent.mInputType == MULTITOUCH_INPUT) { Reduce nesting level a bit, by inverting the condition and returning early ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:80 (Diff revision 4) > + MultiTouchInput& multiTouch = aEvent.AsMultiTouchInput(); > + int32_t currentTouch = 0; > + if (!mPinnedFlags && GetTouchValue(multiTouch, &currentTouch)) { You can reduce another nesting level by doing an early-return here. You'd still need to do the event translation near the bottom of the function but you could extract that into a helper function that you call just before the early return as well as on the non-early-return codepath. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:89 (Diff revision 4) > + if (multiTouch.mType == MultiTouchInput::MULTITOUCH_START) { > + mControllerCancelTouchTracking = false; > + mControllerStartTouch = mControllerPreviousTouch = currentTouch; > + if (currentToolbarState == ToolbarAnimating) { > + StopCompositorAnimation(); > + status = nsEventStatus_eIgnore; nit: single space after '=' ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:105 (Diff revision 4) > + mControllerTotalDistance += delta; > + if (delta != 0) { > + mControllerLastDragDirection = (delta > 0 ? MOVE_TOOLBAR_DOWN : MOVE_TOOLBAR_UP); > + } > + if (CanHideToolbar(delta)) { > + const uint32_t dragThreshold = Abs((int32_t)((gfxPrefs::ToolbarScrollThreshold() * mControllerCompositionHeight) + 0.5f)); Can you just use Round instead of the (int32_t)(.. + 0.5f)? ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:113 (Diff revision 4) > + status = ProcessTouchDelta(currentToolbarState, delta, multiTouch.mTime); > + } > + } > + mControllerLastEventTimeStamp = multiTouch.mTime; > + } > + } else { // MultiTouchInput::MULTITOUCH_END This is not necessarily MULTITOUCH_END. It could be a MULTITOUCH_CANCEL. It would be better turn this if/elseif/else into a switch statement and just list all the cases explicitly. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:120 (Diff revision 4) > + } > + } > + > + // Only adjust touch events. Wheel events (aka scroll events) are adjust in the NativePanZoomController > + if (mControllerToolbarHeight > 0) { > + aEvent.Translate(ScreenPoint(0.0f, -(float)mControllerToolbarHeight)); call Translate on multiTouch instead of aEvent, see my comment in InputData.h ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:210 (Diff revision 4) > +AndroidDynamicToolbarAnimator::ToolbarAnimatorMessageFromUI(int32_t aMessage) > +{ > + MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()); > + switch(aMessage) { > + case STATIC_TOOLBAR_NEEDS_UPDATE: > + break; Indent the break statements. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:250 (Diff revision 4) > + break; > + } > +} > + > +bool > +AndroidDynamicToolbarAnimator::UpdateAnimation(TimeStamp& aCurrentFrame) Why does this take a non-const reference? The function doesn't seem to modify it. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:427 (Diff revision 4) > + mCompositorAnimationImmediate(VISIBLE_ANIMATE), > + mCompositorMaxToolbarHeight(0), Missing mCompositorLayerUpdateEnabled ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:451 (Diff revision 4) > + const bool tryingToHideToolbar = aDelta < 0; > + > + if (tryingToHideToolbar && !mControllerScrollingRootContent) { > + // This prevent the toolbar from hiding if a subframe is being scrolled up. > + // The toolbar will always become visible regardless what is being scrolled down. > + return nsEventStatus_eIgnore; return status; ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:458 (Diff revision 4) > + } > + } else if (aCurrentToolbarState == ToolbarUnlocked) { return status; } if (aCurrentToolbarState != ToolbarUnlocked) { return status; } if ((mControllerState != UnlockPending) && (mControllerState != NothingPending)) { return status; } ... (i.e. reduce nesting level) ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:579 (Diff revision 4) > + APZThreadUtils::RunOnControllerThread(NewRunnableMethod<int32_t, int32_t>(this, &AndroidDynamicToolbarAnimator::UpdateControllerToolbarHeight, aHeight, aMaxHeight)); > + return; > + } > + > + mControllerToolbarHeight = aHeight; > + if (aMaxHeight >= 0) { mControllerMaxToolbarHeight = aMaxHeight; } newlines plz ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:628 (Diff revision 4) > + // NOTE: StartCompositorAnimation will set mControllerState to AnimationStartPending > + StartCompositorAnimation(aDirection, aImmediate, mControllerToolbarHeight); You could do a MOZ_ASSERT(mControllerState == AnimationStartPending); after the call to enforce this note. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:703 (Diff revision 4) > + if (!APZThreadUtils::IsControllerThread()) { > + APZThreadUtils::RunOnControllerThread(NewRunnableMethod<int32_t>(this, &AndroidDynamicToolbarAnimator::NotifyControllerAnimationStopped, aHeight)); > + return; > + } > + > + mControllerState = NothingPending; Do you want to assert that mControllerState == AnimationStopPending before this assignment? I'm kind of worried about how much stuff is getting bounced around between threads. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:760 (Diff revision 4) > + aCssPageRect.x, aCssPageRect.y, > + aCssPageRect.XMost(), aCssPageRect.YMost())); > + } > +} > + > +static const float SHRINK_FACTOR = 0.95f; Move this up to be with other const things at the top of the file ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:767 (Diff revision 4) > + if (((float)mCompositorSurfaceHeight >= (mControllerFrameMetrics.mPageRect.YMost() * SHRINK_FACTOR)) || > + ((float)mCompositorSurfaceHeight >= ((mControllerFrameMetrics.mPageRect.YMost() - mControllerFrameMetrics.mScrollOffset.y) * SHRINK_FACTOR))) { This assumes mCompositorSurfaceHeight is in ParentLayer coordinates. Therefore it should probably be a ParentLayerCoord or ParentLayerIntCoord rather than an untyped int32_t. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:792 (Diff revision 4) > + if (!FuzzyEqualsMultiplicative(aScrollOffset.x, mScrollOffset.x) || > + !FuzzyEqualsMultiplicative(aScrollOffset.y, mScrollOffset.y) || > + !FuzzyEqualsMultiplicative(aScale.scale, mScale.scale) || Is there a noticeable perf hit/overhead from doing == comparisons here (in that more messages get sent unnecessarily)? If not it might be safer to do that. In my experience with the previous dynamic toolbar there were some edge cases where values ended up being very close to, but not quite, zero. I can't recall exactly but you might end up with similar issues. ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3814 (Diff revision 4) > + if ((aNewState == NOTHING) && mFrameMetrics.IsRootContent()) { > + if (APZCTreeManager* manager = GetApzcTreeManager()) { This warrants a comment. It's not clear to me why the metrics need updating specifically when we go into state NOTHING and not at other times. ::: gfx/layers/composite/AsyncCompositionManager.h:240 (Diff revision 4) > +public: > + void SetFixedLayerMargins(); Again I'd prefer laying this out like so: #ifdef MOZ_WIDGET_ANDROID public: void SetFixedLayerMargins(); private: ... #endif }; ::: gfx/layers/composite/AsyncCompositionManager.cpp:965 (Diff revision 4) > - mFixedLayerMargins = fixedLayerMargins; > mLayersUpdated = false; > - mPaintSyncId = 0; > } > mIsFirstPaint = false; > + fixedLayerMargins = mFixedLayerMargins; It doesn't look like |fixedLayerMargins| is used at all, I think you can remove it. |mFixedLayerMargins| should have the margins and that is used wherever needed. ::: gfx/layers/composite/LayerManagerComposite.h:221 (Diff revision 4) > // change its rendering at this time. In order not to miss it, we composite > // on every vsync until this time occurs (this is the latest such time). > TimeStamp mCompositeUntilTime; > +#if defined(MOZ_WIDGET_ANDROID) > +public: > + // Used by UiCompositorControllerParent to set it self as the target for the s/it self/itself/ ::: gfx/layers/composite/LayerManagerComposite.h:498 (Diff revision 4) > + UiCompositorControllerParent* mScreenPixelsTarget; > +public: > + virtual void RequestScreenPixels(UiCompositorControllerParent* aController) Move mScreenPixelsTarget to a private block below the function ::: gfx/layers/composite/LayerManagerComposite.cpp:952 (Diff revision 4) > clipRect = ParentLayerIntRect(rect.x, rect.y, rect.width, rect.height); > } > +#if defined(MOZ_WIDGET_ANDROID) > + int32_t toolbarHeight = RenderToolbar(); > + // This doesn't affect the projection matrix after BeginFrame has been called. > + ScopedCompositorRenderOffset scopedOffset(mCompositor->AsCompositorOGL(), ScreenPoint(0, toolbarHeight)); neat! ::: gfx/layers/composite/LayerManagerComposite.cpp:1184 (Diff revision 4) > +int32_t > +LayerManagerComposite::RenderToolbar() This needs wrapping in a MOZ_WIDGET_ANDROID #ifdef, but I don't see one enclosing it. I must be missing something because this shouldn't even compile ::: gfx/layers/composite/LayerManagerComposite.cpp:1190 (Diff revision 4) > + if (mCompositor->GetTargetContext() == nullptr) { > + if (CompositorBridgeParent* bridge = mCompositor->GetCompositorBridgeParent()) { Do some early returns here ::: gfx/layers/composite/LayerManagerComposite.cpp:1216 (Diff revision 4) > + } > + > + return toolbarHeight; > +} > + > +// Used by robocop tests to get a snap shot of the frame buffer. s/snap shot/snapshot/ ::: gfx/layers/ipc/CompositorBridgeParent.h:129 (Diff revision 4) > +#if defined(MOZ_WIDGET_ANDROID) > + virtual uint64_t GetRootLayerTreeId() { return 0; } > +#endif // defined(MOZ_WIDGET_ANDROID) See my comment in WebRenderBridgeParent, I would rather leave this out for now. ::: gfx/layers/ipc/UiCompositorControllerParent.cpp:200 (Diff revision 4) > + > +void > +UiCompositorControllerParent::RegisterAndroidDynamicToolbarAnimator(AndroidDynamicToolbarAnimator* aAnimator) > +{ > +#if defined(MOZ_WIDGET_ANDROID) > + mAnimator = aAnimator; Is it possible to register a new toolbar on top of the old one? If not, add a MOZ_ASSERT(!mAnimator) before this assignment. ::: gfx/layers/ipc/UiCompositorControllerParent.cpp:227 (Diff revision 4) > + : mRootLayerTreeId(aRootLayerTreeId), > + mMaxToolbarHeight(0) nit: line up the comma under the colon ::: gfx/layers/wr/WebRenderBridgeParent.cpp:146 (Diff revision 4) > - widget::AndroidCompositorWidget* widget = mWidget->AsAndroid(); > - if (widget) { > - widget->SetFirstPaintViewport(LayerIntPoint(0, 0), CSSToLayerScale(), CSSRect(0, 0, aSize.width, aSize.height)); > + RefPtr<UiCompositorControllerParent> uiController = UiCompositorControllerParent::GetFromRootLayerTreeId(mCompositorBridge->GetRootLayerTreeId()); > + if (uiController) { > + uiController->ToolbarAnimatorMessageFromCompositor(/*FIRST_PAINT*/ 5); Do you actually care about webrender support right now? I would rather just comment this out for now, because then we get rid of the GetRootLayerTreeId() functions you had to add as well. ::: gfx/thebes/gfxPrefs.h:346 (Diff revision 4) > DECL_GFX_PREF(Live, "apz.zoom_animation_duration_ms", APZZoomAnimationDuration, int32_t, 250); > DECL_GFX_PREF(Live, "apz.scale_repaint_delay_ms", APZScaleRepaintDelay, int32_t, 500); > > DECL_GFX_PREF(Live, "browser.ui.zoom.force-user-scalable", ForceUserScalable, bool, false); > DECL_GFX_PREF(Live, "browser.viewport.desktopWidth", DesktopViewportWidth, int32_t, 980); > + DECL_GFX_PREF(Live, "browser.ui.scroll-toolbar-threshold", ToolbarScrollThreshold, float, 0.10f); Move this up a couple lines so it's sorted by pref name ::: widget/InputData.h:100 (Diff revision 4) > + virtual void Translate(const ScreenPoint& aTranslation) = 0; > + This is overkill - you only ever call this function on touch inputs, so putting it on InputData and implementing it on all the different input types seems unnecessary. You can just have the function on MultiTouchInput and use it there.
Comment on attachment 8856019 [details] Bug 1335895 - part 1: Update CompositorSession to hold a pointer to UiCompositorControllerChild on Android https://reviewboard.mozilla.org/r/127928/#review132546 OK, I finished my first pass through it. I kind of skimmed a good chunk of the details but I'll try and dig into it on the second pass. Feel free to push updated patches now if you want. I won't do the second pass until Monday at the earliest. ::: gfx/layers/ipc/PUiCompositorController.ipdl:24 (Diff revision 4) > - sync ResumeAndResize(uint64_t layersId, int32_t height, int32_t width); > - async InvalidateAndRender(uint64_t layersId); > + sync ResumeAndResize(int32_t aWidth, int32_t aHeight); > + async InvalidateAndRender(); Add a blank line after the ResumeAndResize so that it's more obvious the comment above Pause() doesn't apply to everything. ::: gfx/layers/ipc/UiCompositorControllerChild.h:66 (Diff revision 4) > + bool mResizeCached; > + int32_t mResizeWidth; > + int32_t mResizeHeight; You can replace these with a Maybe<IntSize>. It would make the code a lot cleaner. ::: gfx/layers/ipc/UiCompositorControllerChild.h:69 (Diff revision 4) > + bool mMaxToolbarHeightCached; > + int32_t mMaxToolbarHeight; Ditto here, Maybe<int32_t>. Same for the other "bool ...Cached;" variables. ::: gfx/layers/ipc/UiCompositorControllerChild.cpp:33 (Diff revision 4) > + return nullptr; > +} > +#endif // defined(MOZ_WIDGET_ANDROID) > > namespace { > - > +static const int32_t COMPOSITOR_CONTROLLER_OPEN = 20; I'm not terribly happy with this const floating out here by itself. Instead of piggybacking this notification with the other messages that come over IPC, would it make sense to have a dedicated method for it? e.g. widget->CompositorControllerOpen() or whatever ::: gfx/layers/ipc/UiCompositorControllerChild.cpp:42 (Diff revision 4) > - mSurfaceWidth = value.mSurfaceWidth; > + return NS_GetCurrentThread() == GetUiThread(); > - mSurfaceHeight = value.mSurfaceHeight; > - return *this; > - } > +} > > - SurfaceResizeCache() : > +using namespace mozilla::gfx; This is pre-existing, but I'd like to avoid "using namespace" as it is another vector for unified compilation failures when things shift around. Please remove it if you can - using "using" statements for individual classes is fine. ::: gfx/layers/ipc/UiCompositorControllerParent.h:19 (Diff revision 4) > +#include "mozilla/RefPtr.h" > > namespace mozilla { > namespace layers { > > +class AndroidDynamicToolbarAnimator; I'm finding your use of ifdef kind of inconsistent. For example in this file you have an #include that's wrapped in an ifdef, you have this forward-declare that's not wrapped in an ifdef, you have the function RegisterAndroidDynamicToolbarAnimator not wrapped in an ifdef, and you have the RefPtr which is wrapped in an ifdef. I get that if you're leaving RegisterAndroidDynamicToolbarAnimator outside the ifdef, then you need this forward-declare to make the compiler happy. But you don't need the include in that case, because RefPtr<AndroidDynamicToolbarAnimator> should compile fine with the forward-declare. But I guess why did you leave the Register function outside the ifdef? It's only ever called from the AndroidDynamicToolbarAnimator code which is android-only. Personally I'd rather remove the usage of #ifdef as much as possible, so I would also un-ifdef things like the mUiControllerParent field in LayerTreeState which allows you to un-ifdef a bunch of other stuff. It might bloat a few data structures here and there but on the whole I find it results in less time wasted with compilation failures, cognitive load, etc. Whichever route you want to take, though, I'd like it to be consistent - either ifdef as much of the android-specific stuff or as little as possible. ::: gfx/layers/ipc/UiCompositorControllerParent.cpp:173 (Diff revision 4) > + if (mAnimator) { > + mAnimator->AdoptToolbarPixels(aWidth, aHeight, Move(aMem)); > + } else { > + DeallocShmem(aMem); Add a comment here to the effect that the animator becomes responsible for dealloc'ing the shmem if the call to AdoptToolbarPixels happens. ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java:31 (Diff revision 4) > - ACTION_MODE, > - FULL_SCREEN, > - CARET_DRAG, > - PAGE_LOADING > - } > - > + RELAYOUT(1), > + ACTION_MODE(2), > + FULL_SCREEN(3), > + CARET_DRAG(4), > + PAGE_LOADING(5), > + MAX_PIN_REASON(6); I don't think you need MAX_PIN_REASON for anything ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:55 (Diff revision 4) > > private GeckoLayerClient mLayerClient; > private PanZoomController mPanZoomController; > private DynamicToolbarAnimator mToolbarAnimator; > - private LayerRenderer mRenderer; > /* Must be a PAINT_xxx constant */ Drop this comment ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:221 (Diff revision 4) > + bm.getPixels(pixels, /* offset */ 0, /* stride */ width, /* x */ 0, /* y */ 0, width, height); > + mCompositor.sendToolbarPixelsToCompositor(width, height, pixels); > + } catch (Exception e) { > + Log.e(LOGTAG, "Caught exception while getting toolbar pixels from Bitmap: " + e.toString()); > + } > + break; Indent all the break statements ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:246 (Diff revision 4) > + break; > + case COMPOSITOR_CONTROLLER_OPEN: > + mToolbarAnimator.notifyCompositorControllerOpen(); > + break; > + default: > + Log.e(LOGTAG,"Unhandled Toolbar Animator Message: " + message); break; here as well ::: widget/android/nsWindow.cpp:1133 (Diff revision 4) > + > + void RequestScreenPixels() > + { > MOZ_ASSERT(AndroidBridge::IsJavaUiThread()); > - child->SendInvalidateAndRender(id); > + if (RefPtr<UiCompositorControllerChild> child = GetUiCompositorControllerChild()) { > + child->RequestScreenPixels(); nit: indentation. Ditto for similar functions below. ::: widget/nsBaseWidget.h:410 (Diff revision 4) > + void UpdateRootFrameMetrics(float aScrollX, float aScrollY, float aZoom, > + float aCssPageLeft, float aCssPageTop, > + float aCssPageRight, float aCssPageBottom) override {}; nit: line up the overhanging lines with the '(' ::: widget/nsGUIEventIPC.h:1103 (Diff revision 4) > { > typedef mozilla::MultiTouchInput paramType; > > static void Write(Message* aMsg, const paramType& aParam) > { > - WriteParam(aMsg, static_cast<mozilla::InputData>(aParam)); > + WriteParam(aMsg, static_cast<const mozilla::InputData&>(aParam)); I'm assuming this change will not be needed if you get rid of the Translate method from InputData ::: widget/nsIWidget.h:2033 (Diff revision 4) > + * @param aScrollX page X scroll value in pixels. > + * @param aScrollY page Y scroll value in pixels. which pixels? ::: widget/nsIWidget.h:2042 (Diff revision 4) > + virtual void UpdateRootFrameMetrics(float aScrollX, float aScrollY, float aZoom, > + float aCssPageLeft, float aCssPageTop, > + float aCssPageRight, float aCssPageBottom) = 0; I'd prefer if the aCssXXX variables were of type CSSCoord. Those types even have IPC serialization/deserialization so you could put them in the ipdl.
Comment on attachment 8857702 [details] Bug 1335895 - part 36: update JNI bindings https://reviewboard.mozilla.org/r/129636/#review132746
Attachment #8857702 - Flags: review?(nchen) → review+
Attachment #8856053 - Attachment is obsolete: true
Attachment #8856053 - Flags: review?(bugmail)
Comment on attachment 8856019 [details] Bug 1335895 - part 1: Update CompositorSession to hold a pointer to UiCompositorControllerChild on Android https://reviewboard.mozilla.org/r/127928/#review133432 Review of the interdiff between what I last reviewed and the latest version (i.e. review of the changes you made since my last review). Generally the patches are much better - particularly with respect to documentation. Thanks! ::: gfx/ipc/GPUProcessManager.cpp:568 (Diff revisions 4 - 5) > DisableGPUProcess("Failed to create remote compositor"); > } > - } else { > + } > + > + if (!session) { > session = InProcessCompositorSession::Create( nit: one space after = ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:54 (Diff revisions 4 - 5) > static already_AddRefed<AndroidDynamicToolbarAnimator> Create(); > - void Initialize(const uint64_t aRootLayerTreeId); > + void Initialize(uint64_t aRootLayerTreeId); > + // Used to intercept events to determine if the event affects the toolbar. > nsEventStatus ReceiveInputEvent(InputData& aEvent); > - void SetMaxToolbarHeight(const int32_t aHeight); > - void SetPinned(const bool aPinned, const int32_t aReason); > + void SetMaxToolbarHeight(int32_t aHeight); > + // When an pinned reason is set to true, the animator will prevent s/an pinned/a pinned/ ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:69 (Diff revisions 4 - 5) > int32_t GetCurrentToolbarHeight(); > + // returns the height in device pixels of the current Android surface used to display content and the toolbar. > + // This will only change when the surface provided by the system actually changes size such as when > + // the device is rotated or the virtual keyboard is made visible. > int32_t GetCurrentSurfaceHeight(); > + // This is the height in device pixels of the root documents content. While the toolbar is being hidden or s/documents/document's/ ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:70 (Diff revisions 4 - 5) > + // returns the height in device pixels of the current Android surface used to display content and the toolbar. > + // This will only change when the surface provided by the system actually changes size such as when > + // the device is rotated or the virtual keyboard is made visible. > int32_t GetCurrentSurfaceHeight(); > + // This is the height in device pixels of the root documents content. While the toolbar is being hidden or > + // shown, the content may extend beyond the bottom of the surface until to toolbar is completely s/until to/until the/ ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:85 (Diff revisions 4 - 5) > void ToolbarAnimatorMessageFromUI(int32_t aMessage); > // Returns true if the animation if the animation will continue and false if it has completed. > - bool UpdateAnimation(TimeStamp& aCurrentFrame); > + bool UpdateAnimation(const TimeStamp& aCurrentFrame); > + // Called to signify the first paint has occurred. > void FirstPaint(); > + // Called when ever the root documents FrameMetrics have reached a steady state. s/when ever/whenever/ s/documents/document's/ ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:90 (Diff revisions 4 - 5) > + // Called when ever the root documents FrameMetrics have reached a steady state. > void UpdateRootFrameMetrics(const FrameMetrics& aMetrics); > - void EnableLayerUpdateNotifications(bool aEnable); > - void NotifyLayerUpdated(); > - void AdoptToolbarPixels(const int32_t aWidth, const int32_t aHeight, mozilla::ipc::Shmem&& aMem); > + // When aEnable is set to true, it informs the animator that the UI thread expects to > + // be notified when a layer has been updated. Enabled currently by robocop tests. > + void EnableLayersUpdateNotifications(bool aEnable); > + // Called when a layer has been updated so the UI thread may be notified if necessary. s/a layer/the layer tree/ ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:725 (Diff revisions 4 - 5) > + } else if (mControllerState == NothingPending) { > +// Nothing to do. > + } else { > + Empty else clause, seems like you can remove it? ::: gfx/layers/ipc/UiCompositorControllerChild.cpp:342 (Diff revisions 4 - 5) > > void > UiCompositorControllerChild::SendCachedValues() > { > MOZ_ASSERT(mIsOpen); > - if (mResizeCached) { > + if (mResize.isSome()) { You can just use `if (mResize)` since Maybe implements automatic conversion to bool. Same for the other checks in this function. ::: mobile/android/chrome/content/browser.js (Diff revision 5) > }; > > var BrowserEventHandler = { > init: function init() { > - this._clickInZoomedView = false; > - Services.obs.addObserver(this, "Gesture:SingleTap", false); Oh nice, if we remove Gesture:SingleTap we can remove some or all of the code in AndroidContentController::DispatchSingleTapToObservers as well.
Comment on attachment 8856026 [details] Bug 1335895 - part 8: Add UiCompositorControllerMessageTypes.h https://reviewboard.mozilla.org/r/127944/#review133472 ::: gfx/layers/ipc/UiCompositorControllerMessageTypes.h:21 (Diff revision 5) > +// > + > +enum UiCompositorControllerMessageTypes { > + STATIC_TOOLBAR_NEEDS_UPDATE = 0, // Sent from compositor when the static toolbar wants to hide. > + STATIC_TOOLBAR_READY = 1, // Sent from compositor when the static toolbar image has been updated and is ready to animate. > + TOOLBAR_HIDDEN = 2, // Sent to compositor when the real toolbar has been hidden. nit: alignment
Attachment #8856026 - Flags: review?(botond) → review+
Comment on attachment 8856030 [details] Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127952/#review131056 I actually wrote these review comments last Monday, and thought I had published them, but it appears I did not. Apologies if some of them are out of date by now or overlapping with kats' comments. I'm not done reviewing this patch, will continue now. ::: commit-message-0d0de:1 (Diff revision 1) > +Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator r=kats,botond The commit message could probably use a brief (one or two sentence) description of what AndroidDynamicToolbarAnimator is. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:42 (Diff revision 1) > + > +class AndroidDynamicToolbarAnimator { > +public: > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AndroidDynamicToolbarAnimator); > + static already_AddRefed<AndroidDynamicToolbarAnimator> Create(); > + void Initialize(const uint64_t aRootLayerTreeId); Throughout this file, please remove toplevel const from function parameters. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:43 (Diff revision 1) > +class AndroidDynamicToolbarAnimator { > +public: > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AndroidDynamicToolbarAnimator); > + static already_AddRefed<AndroidDynamicToolbarAnimator> Create(); > + void Initialize(const uint64_t aRootLayerTreeId); > + nsEventStatus ReceiveInputEvent(InputData& aEvent); // May modify the event by applying a translation to it ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:43 (Diff revision 1) > +class AndroidDynamicToolbarAnimator { > +public: > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AndroidDynamicToolbarAnimator); > + static already_AddRefed<AndroidDynamicToolbarAnimator> Create(); > + void Initialize(const uint64_t aRootLayerTreeId); > + nsEventStatus ReceiveInputEvent(InputData& aEvent); It might be worth grouping these methods into categories based on which thread need to be called from (AsyncPanZoomController does something similar). ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:45 (Diff revision 1) > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AndroidDynamicToolbarAnimator); > + static already_AddRefed<AndroidDynamicToolbarAnimator> Create(); > + void Initialize(const uint64_t aRootLayerTreeId); > + nsEventStatus ReceiveInputEvent(InputData& aEvent); > + void SetMaxToolbarHeight(const int32_t aHeight); > + void SetPinned(const bool aPinned, const int32_t aReason); It looks like the reasons originate in Java-land as the enum DynamicToolbarAnimator.PinReason. Could we declare a corresponding C++ enum, and cast the number to be that type? Could probably do it as early as in nsWindow::LayerViewSupport::SetPinned(). (Ideally, the JNI wrapper generator for generate this enum for us, but for now I'd rather do it manually than not at all.) ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:46 (Diff revision 1) > + static already_AddRefed<AndroidDynamicToolbarAnimator> Create(); > + void Initialize(const uint64_t aRootLayerTreeId); > + nsEventStatus ReceiveInputEvent(InputData& aEvent); > + void SetMaxToolbarHeight(const int32_t aHeight); > + void SetPinned(const bool aPinned, const int32_t aReason); > + int32_t GetMaxToolbarHeight(); This and other getters can be const. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:53 (Diff revision 1) > + int32_t GetCurrentSurfaceHeight(); > + int32_t GetCompositionHeight(); > + // Returns true if the composition size has changed from the last time it was set. > + bool SetCompositionSize(int32_t aWidth, int32_t aHeight); > + void SetScrollingRootContent(); > + void ToolbarAnimatorMessageFromUI(int32_t aMessage); As with the pin reason, consider making the message an enum. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:55 (Diff revision 1) > + // Returns true if the composition size has changed from the last time it was set. > + bool SetCompositionSize(int32_t aWidth, int32_t aHeight); > + void SetScrollingRootContent(); > + void ToolbarAnimatorMessageFromUI(int32_t aMessage); > + // Returns true if the animation if the animation will continue and false if it has completed. > + bool UpdateAnimation(TimeStamp& aCurrentFrame); This can take the timestamp by const reference. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:113 (Diff revision 1) > + > + // Controller thread only > + bool mControllerScrollingRootContent; // Set to true when the root content is being scrolled > + bool mControllerDragThresholdReached; // Set to true when the drag threshold has been passed in a single touch > + bool mControllerCancelTouchTracking; // Set to true when the UI thread requests the toolbar be made visible > + int32_t mControllerStartTouch; // The Y position where to touch started It would be nice to use typed units (e.g. LayerCoord, ScreenCoord, or whatever coordinate system they are in) for these (and in the corresponding getters/setters). If that's too inconvenient for some reason, we should at least mention the coordinate system in the comment accompanying the variable, e.g. "// The Y position where the touch started, in \_\_\_\_ pixels" ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:113 (Diff revision 1) > + > + // Controller thread only > + bool mControllerScrollingRootContent; // Set to true when the root content is being scrolled > + bool mControllerDragThresholdReached; // Set to true when the drag threshold has been passed in a single touch > + bool mControllerCancelTouchTracking; // Set to true when the UI thread requests the toolbar be made visible > + int32_t mControllerStartTouch; // The Y position where to touch started to -> the ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:37 (Diff revision 1) > + > +// Internal flags and constants > +static const float ANIMATION_DURATION = 0.15f; // How many seconds the complete animation should span > +static const int32_t MOVE_TOOLBAR_DOWN = 1; // Multiplier to move the toolbar down > +static const int32_t MOVE_TOOLBAR_UP = -1; // Multiplier to move the toolbar up > +static const bool VISIBLE_IMMEDIATE = true; // Flag indicating the animation should be immediate I would prefer making these an enum, e.g. enum AnimationStyle { eImmediate, eAnimate } and then have the second parameter of NotifyControllerPendingAnimation() have type AnimationStyle. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:61 (Diff revision 1) > + MOZ_ASSERT(uiController); > + uiController->RegisterAndroidDynamicToolbarAnimator(this); > +} > + > +static bool > +GetTouchValue(MultiTouchInput& multiTouch, int32_t* value) Please change the type of |value| to |ScreenCoord*|. (Based on the usage of GetTouchValue(), that suggests mControllerStartTouch, mControllerPreviousTouch, mControllerTotalDistance, and mControllerToolbarHeight are all in screen pixels as well.)
Comment on attachment 8856030 [details] Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127952/#review133480 This was a relatively surface-level review - I don't really have enough background with the dynamic toolbar to do an in-depth review of this patch, particularly the aspects related to the messages and state transitions. Hopefully Kats' review is sufficient for those aspects. With that proviso, r+ with comments addressed. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:52 (Diff revision 5) > +public: > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AndroidDynamicToolbarAnimator); > + static already_AddRefed<AndroidDynamicToolbarAnimator> Create(); > + void Initialize(uint64_t aRootLayerTreeId); > + // Used to intercept events to determine if the event affects the toolbar. > + nsEventStatus ReceiveInputEvent(InputData& aEvent); Please document the interpretation of the various nsEventStatus return values. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:54 (Diff revision 5) > + static already_AddRefed<AndroidDynamicToolbarAnimator> Create(); > + void Initialize(uint64_t aRootLayerTreeId); > + // Used to intercept events to determine if the event affects the toolbar. > + nsEventStatus ReceiveInputEvent(InputData& aEvent); > + void SetMaxToolbarHeight(int32_t aHeight); > + // When an pinned reason is set to true, the animator will prevent an -> a ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:74 (Diff revision 5) > + // This is the height in device pixels of the root documents content. While the toolbar is being hidden or > + // shown, the content may extend beyond the bottom of the surface until to toolbar is completely > + // visible or hidden. > + int32_t GetCompositionHeight(); > + // Returns true if the composition size has changed from the last time it was set. > + bool SetCompositionSize(int32_t aWidth, int32_t aHeight); Take a Size, preferably a typed one like ScreenIntSize? ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:81 (Diff revision 5) > + // affecting the toolbar when being scrolled up. The idea is a scrolling down will always > + // show the toolbar while scrolling up will only hide the toolbar if it is the root content > + // being scrolled. > + void SetScrollingRootContent(); > + void ToolbarAnimatorMessageFromUI(int32_t aMessage); > + // Returns true if the animation if the animation will continue and false if it has completed. extra "if the animation" ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:189 (Diff revision 5) > + int32_t mCompositorSurfaceHeight; // Current height of the render surface > + int32_t mCompositorCompositionWidth; // Current width of the visible page > + int32_t mCompositorCompositionHeight; // Current height of the visible page > + int32_t mCompositorAnimationDirection; // Direction the snapshot should be animated > + int32_t mCompositorAnimationStartHeight; // The height of the snapshot at the start of an animation > + bool mCompositorToolbarPixelsUpdated; // Set to true when the snapshot needs to be updated with new pixels mCompositorToolbarPixels and mCompositorToolbarPixelsUpdated may be another good place to use mozilla::Maybe. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:74 (Diff revision 5) > +nsEventStatus > +AndroidDynamicToolbarAnimator::ReceiveInputEvent(InputData& aEvent) > +{ > + MOZ_ASSERT(APZThreadUtils::IsControllerThread()); > + > + // Only process and adjust touch events. Wheel events (aka scroll events) are adjust in the NativePanZoomController adjust -> adjusted ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:318 (Diff revision 5) > +} > + > +void > +AndroidDynamicToolbarAnimator::UpdateRootFrameMetrics(const FrameMetrics& aMetrics) > +{ > + nit: stray blank line ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:354 (Diff revision 5) > + mCompositorToolbarPixelsHeight = aHeight; > + mCompositorToolbarPixelsUpdated = true; > +} > + > +Effect* > +AndroidDynamicToolbarAnimator::GetToolbarEffect(CompositorOGL* gl) I would prefer that this function be reviewed by :nical or :mattwoodrow. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:384 (Diff revision 5) > + RefPtr<UiCompositorControllerParent> uiController = UiCompositorControllerParent::GetFromRootLayerTreeId(mRootLayerTreeId); > + uiController->DeallocShmem(mCompositorToolbarPixels); > + mCompositorToolbarPixels = mozilla::ipc::Shmem(); > + mCompositorToolbarPixelsUpdated = false; > + if (mCompositorToolbarTexture) { > + CompositorThreadHolder::Loop()->PostTask(NewRunnableMethod(this, &AndroidDynamicToolbarAnimator::PostToolbarReady)); If we're already on the compositor thread, why are we posting a task to the compositor thread? Is it so that it's run after the rest of the processing of the current task? If so, that needs a comment. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:388 (Diff revision 5) > + if (mCompositorToolbarTexture) { > + CompositorThreadHolder::Loop()->PostTask(NewRunnableMethod(this, &AndroidDynamicToolbarAnimator::PostToolbarReady)); > + } > + } > + > + if (mCompositorToolbarTexture) { I notice we only clear mCompositorToolbarTexture on shutdown, or if the texture upload failed. Is there not anything else that invalidates the texture (like the contents it's supposed to show changing)? ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:458 (Diff revision 5) > + MOZ_ASSERT(APZThreadUtils::IsControllerThread()); > + nsEventStatus status = nsEventStatus_eIgnore; > + > + const bool tryingToHideToolbar = aDelta < 0; > + > + if (tryingToHideToolbar && !mControllerScrollingRootContent) { This is called by ReceiveInputEvent(), but mControllerScrollingRootContent is set by AsyncPanZoomController::AttemptScroll(), which happens later. So, for the first touch-move event that scrolls the root content, mControllerScrollingRootContent will be false here even though we are in fact scrolling the root content. Is that OK? ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:494 (Diff revision 5) > + mControllerState = ShowPending; > + } > + > + UpdateCompositorToolbarHeight(mControllerToolbarHeight); > + RequestComposite(); > + status = nsEventStatus_eConsumeNoDefault; Do we necessarily want to consume the entire delta? In case where the delta is larger than necessary to finish hiding or showing the toolbar, might we not instead want to modify the delta to store the remainder, and have the processed by the rest of the pipeline? ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:510 (Diff revision 5) > + > + return status; > +} > + > +void > +AndroidDynamicToolbarAnimator::HandleTouchReset(StaticToolbarState aCurrentToolbarState, int32_t aCurrentTouch) I'd call this HandleTouchEndOrCancel(), or just HandleTouchEnd(). ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:513 (Diff revision 5) > + > +void > +AndroidDynamicToolbarAnimator::HandleTouchReset(StaticToolbarState aCurrentToolbarState, int32_t aCurrentTouch) > +{ > + MOZ_ASSERT(APZThreadUtils::IsControllerThread()); > + int32_t direction = mControllerLastDragDirection;; nit: stray semicolon ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:541 (Diff revision 5) > + return; > + } > + > + // Don't animate up if not scrolling root content. > + if (!isRoot && > + ((direction == MOVE_TOOLBAR_UP) && (mControllerToolbarHeight == mControllerMaxToolbarHeight))) { (mControllerToolbarHeight == mControllerMaxToolbarHeight) is already being checked by ShowToolbarIfNotVisible(). ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:655 (Diff revision 5) > +} > + > +void > +AndroidDynamicToolbarAnimator::StartCompositorAnimation(int32_t aDirection, bool aImmediate, int32_t aHeight) > +{ > + stray blank line ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:656 (Diff revision 5) > + > +void > +AndroidDynamicToolbarAnimator::StartCompositorAnimation(int32_t aDirection, bool aImmediate, int32_t aHeight) > +{ > + > + if (!CompositorThreadHolder::IsInCompositorThread()) { MOZ_ASSERT(aDirection == MOVE_TOOLBAR_UP || aDirection == MOVE_TOOLBAR_DOWN) ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:746 (Diff revision 5) > + > + CompositorBridgeParent* parent = CompositorBridgeParent::GetCompositorBridgeParentFromLayersId(mRootLayerTreeId); > + if (parent) { > + AsyncCompositionManager* manager = parent->GetCompositionManager(nullptr); > + if (manager) { > + manager->SetFixedLayerMargins(); Since it's the ToolbarAnimator calling AsyncCompositionManager::SetFixedLayerMargins(), is it necessary for that method to get a hold of the ToolbarAnimator and query it for things like GetCurrentToolbarHeight()? Can we not just pass that in? ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:786 (Diff revision 5) > + aScrollOffset.x, aScrollOffset.y, aScale.scale, aCssPageRect)); > + } > +} > + > +bool > +AndroidDynamicToolbarAnimator::CanHideToolbar(int32_t delta) I don't understand the semantics of this function. Is it supposed to be "can a touch with this delta hide the toolbar"? If so, why does it return true when delta > 0? Or is it supposed to be "can the toolbar be hidden at all on this page"? That's what the comparisons to the page rect suggest, but then why is it dependent on delta at all? ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:790 (Diff revision 5) > +bool > +AndroidDynamicToolbarAnimator::CanHideToolbar(int32_t delta) > +{ > + MOZ_ASSERT(APZThreadUtils::IsControllerThread()); > + if (delta < 0) { > + if (((float)mCompositorSurfaceHeight >= (mControllerFrameMetrics.mPageRect.YMost() * SHRINK_FACTOR)) || It might be helpful to annotate these conditions with a couple of comments: // The page is so short that there's no point hiding the toolbar // We're so close to the end of the page that there's no point hiding the toolbar ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:791 (Diff revision 5) > +AndroidDynamicToolbarAnimator::CanHideToolbar(int32_t delta) > +{ > + MOZ_ASSERT(APZThreadUtils::IsControllerThread()); > + if (delta < 0) { > + if (((float)mCompositorSurfaceHeight >= (mControllerFrameMetrics.mPageRect.YMost() * SHRINK_FACTOR)) || > + ((float)mCompositorSurfaceHeight >= ((mControllerFrameMetrics.mPageRect.YMost() - mControllerFrameMetrics.mScrollOffset.y) * SHRINK_FACTOR))) { mCompositorSurfaceHeight is marked as being a compositor-thread-only variable, but it's being accessed on the controller thread here. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:815 (Diff revision 5) > +bool > +AndroidDynamicToolbarAnimator::FrameMetricsState::Update(const ParentLayerPoint& aScrollOffset, > + const CSSToParentLayerScale& aScale, > + const CSSRect& aCssPageRect) > +{ > + if (!FuzzyEqualsMultiplicative(aScrollOffset.x, mScrollOffset.x) || Optional: consider introducing FuzzyEqualsMultiplicative overloads for points and rects.
Attachment #8856030 - Flags: review?(botond) → review+
Comment on attachment 8856026 [details] Bug 1335895 - part 8: Add UiCompositorControllerMessageTypes.h https://reviewboard.mozilla.org/r/127944/#review134040
Attachment #8856026 - Flags: review?(bugmail) → review+
Comment on attachment 8856029 [details] Bug 1335895 - part 11: Add Translate function to MultiTouchData https://reviewboard.mozilla.org/r/127950/#review134052 r+ assuming the nsGUIEventIPC.h changes can be removed. ::: commit-message-4bca2:3 (Diff revision 6) > +Compile fix: widget/nsGUIEventIPC.h needed to have all > +static_cast<InputData> changed to static_cast<const InputData&> Are the changes to nsGUIEventIPC.h still needed, now that Translate isn't a virtual function on InputData?
Attachment #8856029 - Flags: review?(bugmail) → review+
Comment on attachment 8856052 [details] Bug 1335895 - part 35: Remove ZoomedView.java specific code from ThumbnailHelper.h and AndroidContentController https://reviewboard.mozilla.org/r/127996/#review134054
Attachment #8856052 - Flags: review?(bugmail) → review+
Comment on attachment 8856051 [details] Bug 1335895 - part 34: Remove PanZoomTarget.java, ZoomedView.java, and LayerRenderer.java https://reviewboard.mozilla.org/r/127994/#review134056
Attachment #8856051 - Flags: review?(bugmail) → review+
Comment on attachment 8856050 [details] Bug 1335895 - part 33: Update FennecNativeDriver.java so robocop tests work with new dynamic toolbar https://reviewboard.mozilla.org/r/127992/#review134064 ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/FennecNativeDriver.java:223 (Diff revision 6) > + int lastStart = 0xFF000000; > + int last = 0xFF000000; No idea what these are, so I can't say what this output would look like. I guess it's for debugging so whatever. ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/FennecNativeDriver.java:235 (Diff revision 6) > + sb = new StringBuffer(); > + lastStart = agbr; > + } > + > + if ((sb != null) && differentColor(last, agbr)) { > + sb.append('('); This entire if-body block can be replaced by: sb.append(String.format("(%3d,%3d,%3d) ", (agbr & 0xFF), (agbr & 0xFF00) >> 8, (agbr & 0xFF0000) >> 8));
Attachment #8856050 - Flags: review?(bugmail) → review+
Comment on attachment 8856049 [details] Bug 1335895 - part 32: Fix rendering of toolbar "curve" so that it works with software rendering https://reviewboard.mozilla.org/r/127990/#review134068 I'm not really qualified to review this patch, dropping r? since you already got r=jchen
Attachment #8856049 - Flags: review?(bugmail)
Comment on attachment 8856048 [details] Bug 1335895 - part 31: Update FloatingToolbarTextSelection, FormAssistPopup, OverscrollEdgeEffect, and GeckoInputConnection to use LayerView.getCurrentToolbarHeight() https://reviewboard.mozilla.org/r/127988/#review134072
Attachment #8856048 - Flags: review?(bugmail) → review+
Comment on attachment 8856047 [details] Bug 1335895 - part 30: Update GeckoApp and Tabs to remove code calling deleted function LayerView.setPaintState() https://reviewboard.mozilla.org/r/127986/#review134074
Attachment #8856047 - Flags: review?(bugmail) → review+
Comment on attachment 8856046 [details] Bug 1335895 - part 29: Update BrowserApp.java to work with new dynamic toolbar https://reviewboard.mozilla.org/r/127984/#review134078
Attachment #8856046 - Flags: review?(bugmail) → review+
Comment on attachment 8856054 [details] Bug 1335895 - part 28: Remove unused resources, prefs, and layout used by deleted ZoomedView.java https://reviewboard.mozilla.org/r/128000/#review134082
Attachment #8856054 - Flags: review?(bugmail) → review+
Comment on attachment 8856050 [details] Bug 1335895 - part 33: Update FennecNativeDriver.java so robocop tests work with new dynamic toolbar https://reviewboard.mozilla.org/r/127992/#review134064 > No idea what these are, so I can't say what this output would look like. I guess it's for debugging so whatever. They are both transparent black. The color isn't found in the test page so it was a good one to start with. > This entire if-body block can be replaced by: > sb.append(String.format("(%3d,%3d,%3d) ", (agbr & 0xFF), (agbr & 0xFF00) >> 8, (agbr & 0xFF0000) >> 8)); Yeah, that is better :).
Comment on attachment 8856030 [details] Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127952/#review133480 > I notice we only clear mCompositorToolbarTexture on shutdown, or if the texture upload failed. Is there not anything else that invalidates the texture (like the contents it's supposed to show changing)? The texture is only shown while the toolbar is being hidden or shown. As soon as the texture is completely visible, the real toolbar is show. The real toolbar then can not be hidden until the texture has been updated. So the animator doesn't care if the content has changed because it always assume the texture is stale and requests an update before asking the real toolbar to hide and starting animations.
Comment on attachment 8856030 [details] Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127952/#review133480 > This is called by ReceiveInputEvent(), but mControllerScrollingRootContent is set by AsyncPanZoomController::AttemptScroll(), which happens later. > > So, for the first touch-move event that scrolls the root content, mControllerScrollingRootContent will be false here even though we are in fact scrolling the root content. Is that OK? This is fine. The page must be scrolled a certain amount before the animator attempts to hide or show the toolbar. In that time, the flag gets set. This is how it worked in the previous dynamic toolbar with possibly more lag since it had to jump one or two thread to get back to the Java UI thread.
(In reply to Randall Barker [:rbarker] from comment #364) > > No idea what these are, so I can't say what this output would look like. I guess it's for debugging so whatever. > > They are both transparent black. The color isn't found in the test page so > it was a good one to start with. > I meant more so I have no idea what the logging output will produce. I assume it shows the pixels that are different in some useful format but I don't understand how exactly, or why you are tracking old pixels and special-casing x==0 and things like that. If you feel like explaining do it by adding comments in the code, not in a bugzilla comment.
Comment on attachment 8856019 [details] Bug 1335895 - part 1: Update CompositorSession to hold a pointer to UiCompositorControllerChild on Android https://reviewboard.mozilla.org/r/127928/#review134316 ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:146 (Diff revisions 6 - 7) > void RequestComposite(); > void PostToolbarReady(); > void UpdateFrameMetrics(ScreenPoint aScrollOffset, > CSSToScreenScale aScale, > CSSRect aCssPageRect); > - bool CanHideToolbar(int32_t delta); > + bool IsEnoughtPageToHideToolbar(ScreenIntCoord delta); s/Enought/Enough/ ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:149 (Diff revisions 6 - 7) > CSSToScreenScale aScale, > CSSRect aCssPageRect); > - bool CanHideToolbar(int32_t delta); > + bool IsEnoughtPageToHideToolbar(ScreenIntCoord delta); > void ShowToolbarIfNotVisible(StaticToolbarState aCurrentToolbarState); > - void TranslateTouchEvent(MultiTouchInput& aTouchEvent); > + void TranslateTouchEvent(MultiTouchInput& aTouchEvent, ScreenIntCoord* aDeltaConsumed = nullptr); > + ScreenIntCoord GetFixeLayerMarginsBottom(); s/FixeLayer/FixedLayer/ ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:443 (Diff revisions 6 - 7) > - , mCompositorToolbarPixelsWidth(0) > - , mCompositorToolbarPixelsHeight(0) > {} > > nsEventStatus > -AndroidDynamicToolbarAnimator::ProcessTouchDelta(StaticToolbarState aCurrentToolbarState, int32_t aDelta, uint32_t aTimeStamp) > +AndroidDynamicToolbarAnimator::ProcessTouchDelta(StaticToolbarState aCurrentToolbarState, ScreenIntCoord aDelta, uint32_t aTimeStamp, ScreenIntCoord& aDeltaConsumed) nit: one space in "ScreenIntCoord aDelta" ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:487 (Diff revisions 6 - 7) > mControllerToolbarHeight = mControllerMaxToolbarHeight; > PostMessage(TOOLBAR_SHOW); > mControllerState = eShowPending; > } > > + aDeltaConsumed = aDelta - deltaRemainder; maybe MOZ_ASSERT(aDelta >= deltaRemainder) here, to make sure the numbers come out sane. ::: gfx/layers/ipc/UiCompositorControllerParent.cpp:173 (Diff revisions 6 - 7) > UiCompositorControllerParent::RecvToolbarPixelsToCompositor(const int32_t& aWidth, const int32_t& aHeight, Shmem&& aMem) > { > #if defined(MOZ_WIDGET_ANDROID) > if (mAnimator) { > // By adopting the Shmem, the animator is responsible for deallocating. > - mAnimator->AdoptToolbarPixels(aWidth, aHeight, Move(aMem)); > + mAnimator->AdoptToolbarPixels(ScreenIntSize(aWidth, aHeight), Move(aMem)); You could propagate the ScreenIntSize over IPDL too instead of passing raw int32_t's.
Comment on attachment 8856031 [details] Bug 1335895 - part 13: Update APZCTreeManager to create AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127954/#review134318
Attachment #8856031 - Flags: review?(bugmail) → review+
Comment on attachment 8856033 [details] Bug 1335895 - part 15: Update AsyncPanZoomController to notify toolbar animator when FrameMetrics have changed and when root content is scrolled https://reviewboard.mozilla.org/r/127958/#review134326
Attachment #8856033 - Flags: review?(bugmail) → review+
Comment on attachment 8856034 [details] Bug 1335895 - part 16: Update AsyncCompositionManager to support toolbar animator https://reviewboard.mozilla.org/r/127960/#review134336 ::: gfx/layers/composite/AsyncCompositionManager.cpp:949 (Diff revision 7) > if (mIsFirstPaint) { > - LayerIntPoint scrollOffsetLayerPixels = RoundedToInt(metrics.GetScrollOffset() * geckoZoom); > - mContentRect = metrics.GetScrollableRect(); > + if (CompositorBridgeParent* bridge = compositor->GetCompositorBridgeParent()) { > + AndroidDynamicToolbarAnimator* animator = bridge->GetAPZCTreeManager()->GetAndroidDynamicToolbarAnimator(); So I *think* that this code (both the mIsFirstPaint and the mLayersUpdated blocks) can be moved into AsyncPanZoomController::NotifyLayersUpdated. Basically what's happening here is that this code runs on every composite but only does stuff if the mIsFirstPaint or mLayersUpdated flag is set. Whereas if we put it in NotifyLayersUpdated it would run every time the layers update came in, and there is a flag to check for first-paint there too. There is another codepath which sets the mIsFirstPaint flag here in AsyncCompositionManager, coming from the android widget code via http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/widget/android/nsWindow.cpp#939 - however with the calls to SyncFrameMetrics etc removed I think that codepath is no longer needed and can be removed. However I'd rather do this change in a follow-up bug since I'm a little uncertain about the ramifications and I don't want to destabilize your patch queue in case I missed something (quite possible, I don't remember this code that well). It might also not be worth doing unless we can remove all of this "aOutFoundRoot" code from this function and get a significant clean-up.
Attachment #8856034 - Flags: review?(bugmail) → review+
Comment on attachment 8856035 [details] Bug 1335895 - part 17: Remove SetScrollingRootContent from PAPZ, GeckoContentController, RemoteContentController, and AndroidContentController https://reviewboard.mozilla.org/r/127962/#review134342 Note that in general I haven't been reviewing commit messages since I'm assuming this will all land squashed together. There's not much point landing the patches separately since they don't build independently and would break bisection. (I mention this only because there's a typo in this commit message which caught my eye)
Attachment #8856035 - Flags: review?(bugmail) → review+
Comment on attachment 8856036 [details] Bug 1335895 - part 18: Update gfxPrefs to support browser.ui.scroll-toolbar-threshold https://reviewboard.mozilla.org/r/127964/#review134344 mobile.js contains this pref as an int value with a value 10. That needs to be updated to be a string pref with value "0.10" as well. I'm not sure what happens if you declare a gfxPref as a float and then there's an int value in the preferences - probably undefined behaviour and best not to rely on it being sane.
Attachment #8856036 - Flags: review?(bugmail) → review-
Comment on attachment 8856037 [details] Bug 1335895 - part 19: Update WebRenderBridgeParent.cpp to fix the way the first paint event is sent to the Android UI thread https://reviewboard.mozilla.org/r/127966/#review134346
Attachment #8856037 - Flags: review?(bugmail) → 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/#review134348 Not too happy with polluting nsIWidget with this stuff but I can't think of anything better off the top of my head. Since jchen already gave it r+ that's fine, I'll drop my review flag on this part. ::: widget/nsIWidget.h:2030 (Diff revision 7) > + */ > + > + virtual void UpdateRootFrameMetrics(float aScrollX, float aScrollY, float aZoom, const CSSRect& aPage) = 0; nit: move the blank line from between the comment and decl to below the decl.
Attachment #8856038 - Flags: review?(bugmail)
Comment on attachment 8856039 [details] Bug 1335895 - part 21: Update nsWindow to support update UiCompositorControllerChild https://reviewboard.mozilla.org/r/127970/#review134350 ::: widget/android/nsWindow.cpp:973 (Diff revision 7) > - int64_t id = 0; > + RefPtr<UiCompositorControllerChild> child; > if (LockedWindowPtr window{mWindow}) { > - id = window->GetRootLayerId(); > + child = window->GetUiCompositorControllerChild(); > } Can you use the GetUiCompositorControllerChild function on |this| instead of duplicating the LockedWindowPtr stuff here? Ditto in SyncResumeCompositor and SyncResumeResizeCompositor. ::: widget/android/nsWindow.cpp:1012 (Diff revision 7) > - if (id == 0) { > - return; > + if (child) { > + child->ResumeAndResize(aWidth, aHeight); > - } > - > - RefPtr<UiCompositorControllerChild> child = UiCompositorControllerChild::Get(); > - > - if (!child) { > - // When starting, sometimes the UiCompositorControllerChild is still initializing > - // so cache the resized surface dimensions until it has initialized. Based on the comment in the old code I'm wondering if you need to cache the surface size if |child| is null here. Can that happen, or is this guaranteed to always run after the child has been created? Probably worth a MOZ_ASSERT(child) to make sure. ::: widget/android/nsWindow.cpp:1069 (Diff revision 7) > + void SetMaxToolbarHeight(int32_t aHeight) > + { > + MOZ_ASSERT(AndroidBridge::IsJavaUiThread()); > + > + if (RefPtr<UiCompositorControllerChild> child = GetUiCompositorControllerChild()) { > + mCompositorPaused = false; Why is mCompositorPaused set back to false here? This warrants a comment if the code is correct, because it's not obvious. The other places that set this back to false are all obviously resuming the compositor so those make sense. ::: widget/android/nsWindow.cpp:2390 (Diff revision 7) > + lvs->RecvToolbarAnimatorMessage(aMessage); > + } > +} > + > +void > +nsWindow::UpdateRootFrameMetrics(float aScrollX, float aScrollY, float aZoom, const CSSRect& aPage) { brace placement and indentation in this function is inconsistent with the prevailing style in this file. Same (indentation) problem in RecvScreenPixels
Attachment #8856039 - Flags: review?(bugmail) → review+
Comment on attachment 8856040 [details] Bug 1335895 - part 22: Update AndroidCompositorWidget to remove frame metrics sync https://reviewboard.mozilla.org/r/127972/#review134352
Attachment #8856040 - Flags: review?(bugmail) → 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/#review134348 Originally I only wanted these functions in android/nsWindow but there isn't mechanism to go from nsBaseWidget to nsWindow (other than a gross static cast) so I put the functions in nsBaseWidget. But in an earlier review I was asked to place pure virtual version in nsIWidget.
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/#review134348 Here is the review I was referring to: https://reviewboard.mozilla.org/r/127968/#review131016
Comment on attachment 8856039 [details] Bug 1335895 - part 21: Update nsWindow to support update UiCompositorControllerChild https://reviewboard.mozilla.org/r/127970/#review134350 > Based on the comment in the old code I'm wondering if you need to cache the surface size if |child| is null here. Can that happen, or is this guaranteed to always run after the child has been created? Probably worth a MOZ_ASSERT(child) to make sure. I'll add an assert. In the previous code when UiCompositorControllerChild was a singleton, UiCompositorControllerChild::Get() would return nullptr until UiCompositorControllerChild had been initialized. However, now that it is per CompositorSession, the UiCompositorControllerChild pointer is returned even if it hasn't initialized yet and the values are cached internally until IPC is open.
Comment on attachment 8856031 [details] Bug 1335895 - part 13: Update APZCTreeManager to create AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127954/#review134448
Attachment #8856031 - Flags: review?(botond) → review+
Comment on attachment 8856032 [details] Bug 1335895 - part 14: Update LayerManagerComposite render static toolbar snapshot and handle robocop screen captures https://reviewboard.mozilla.org/r/127956/#review134454 This looks fine to me, but I'm not qualified to r+ it. As with AndroidDynamicToolbarAnimator::GetToolbarEffect(), please ask :nical or :mattwoodrow for a review. ::: gfx/layers/composite/LayerManagerComposite.h:67 (Diff revision 7) > class TextRenderer; > class CompositingRenderTarget; > struct FPSState; > class PaintCounter; > +#if defined(MOZ_WIDGET_ANDROID) > +class UiCompositorControllerParent; It should be harmless to forward-declare things unconditionally. ::: gfx/layers/composite/LayerManagerComposite.h:442 (Diff revision 7) > * Render the current layer tree to the active target. > */ > void Render(const nsIntRegion& aInvalidRegion, const nsIntRegion& aOpaqueRegion); > #if defined(MOZ_WIDGET_ANDROID) > void RenderToPresentationSurface(); > + int32_t RenderToolbar(); Please document what the return value means. ::: gfx/layers/composite/LayerManagerComposite.h:443 (Diff revision 7) > */ > void Render(const nsIntRegion& aInvalidRegion, const nsIntRegion& aOpaqueRegion); > #if defined(MOZ_WIDGET_ANDROID) > void RenderToPresentationSurface(); > + int32_t RenderToolbar(); > + void HandlePixelsTarget(); Please document what this function does. (I see the definition has a comment, putting that here is sufficient.)
Attachment #8856032 - Flags: review?(botond)
Comment on attachment 8856033 [details] Bug 1335895 - part 15: Update AsyncPanZoomController to notify toolbar animator when FrameMetrics have changed and when root content is scrolled https://reviewboard.mozilla.org/r/127958/#review134496 ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3460 (Diff revision 7) > +#if defined(MOZ_WIDGET_ANDROID) > + if (aLayerMetrics.IsRootContent()) { > + if (APZCTreeManager* manager = GetApzcTreeManager()) { > + AndroidDynamicToolbarAnimator* animator = manager->GetAndroidDynamicToolbarAnimator(); > + MOZ_ASSERT(animator); > + ScreenIntSize size = ScreenIntSize::Round(aLayerMetrics.GetRootCompositionSize() * aLayerMetrics.DisplayportPixelsPerCSSPixel()); While I don't think it will ever be numerically wrong given that this is the root layer, it's conceptually wrong to use DisplayportPixelsPerCSSPixel(). Instead, please use: ViewTargetAs<ScreenPixel>(aLayerMetrics.GetZoom(), PixelCastJustification::ScreenIsParentLayerForRoot) ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3831 (Diff revision 7) > } > - > +#if defined(MOZ_WIDGET_ANDROID) > + // The Android UI thread only shows overlay UI elements when the content is not being > + // panned or zoomed and it is in a steady state. So the FrameMetrics only need to be > + // updated when the APZC returns to the NOTHING state. > + if ((aNewState == NOTHING) && mFrameMetrics.IsRootContent()) { The test we use for "steady state" for other purposes is "!IsTransformingState(state)". I would put this code inside the existing "IsTransformingState(aOldState) && !IsTransformingState(aNewState)" block below.
Attachment #8856033 - Flags: review?(botond) → review+
Comment on attachment 8856030 [details] Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127952/#review134470 ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:28 (Diff revision 7) > +struct FrameMetrics; > +class CompositorOGL; > + > +/* > + * The AndroidDynamicToolbarAnimator is responsible for calculating the position > + * and drawing the static snap shot of the toolbar. The animator lives in both s/snap shot/snapshot/ ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:41 (Diff revision 7) > + * that it may begin moving and or animating the snapshot. The animator is > + * responsible for rendering the snapshot until it receives a message to show the > + * toolbar or touch events cause the snapshot to be completely visible. When the > + * snapshot is made completely visible the animator sends a message to the UI > + * thread to show the real toolbar and the whole process may start again. > + * The toolbar height is in device pixels. The toolbar height will be at max height s/device pixels/screen pixels/ ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:108 (Diff revision 7) > + eToolbarUpdated, > + eToolbarUnlocked, eToolbarUpdated and eToolbarUnlocked could use comments since it's not obvious what they mean. In particular there are a number of comments and variables that refer to the toolbar being "unlocked" but there's no explanation anywhere of what it means to be "unlocked". Adding a sentence to that effect in the class documentation would be helpful. If you added something there then presumably the eToolbarUnlocked state would be self-explanatory and wouldn't need a comment. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:160 (Diff revision 7) > + Atomic<StaticToolbarState> mToolbarState; // Current toolbar state. > + Atomic<uint32_t> mPinnedFlags; // The toolbar should not be moved or animated unless no flags are set > + > + // Controller thread only > + bool mControllerScrollingRootContent; // Set to true when the root content is being scrolled > + bool mControllerDragThresholdReached; // Set to true when the drag threshold has been passed in a single touch s/in a single touch/within a single drag/ ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:191 (Diff revision 7) > + // Controller thread only > + FrameMetricsState mControllerFrameMetrics; > + > + // Compositor thread only > + bool mCompositorShutdown; > + bool mCompositorAnimationDeferred; // An animation has been deferred until the controller thread has been notified This comment is confusing, it would be better if it said "... until the toolbar has been unlocked" as that seems to be what the code is doing in practice. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:40 (Diff revision 7) > +AndroidDynamicToolbarAnimator::Create() > +{ > + RefPtr<AndroidDynamicToolbarAnimator> value = new AndroidDynamicToolbarAnimator(); > + return value.forget(); Any particular reason you used a Create() method instead of just new'ing at the call site? I don't mind, just curious. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:223 (Diff revision 7) > + if (mToolbarState != eToolbarAnimating) { > + mToolbarState = eToolbarUnlocked; Technically this is not threadsafe, mToolbarState could change in between the comparison and the assignment. If it were an == comparison I'd ask you to use Atomic::compareExchange but as it stands you'll have to do something more convoluted (good ol' Mutex) if you want to guarantee thread-safety here. I see similar unsafe use of mToolbarState all over the place. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:261 (Diff revision 7) > + > +bool > +AndroidDynamicToolbarAnimator::UpdateAnimation(const TimeStamp& aCurrentFrame) > +{ > + MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()); > + if (mToolbarState == eToolbarAnimating) { Invert condition and early-exit ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:513 (Diff revision 7) > + > +void > +AndroidDynamicToolbarAnimator::HandleTouchEnd(StaticToolbarState aCurrentToolbarState, ScreenIntCoord aCurrentTouch) > +{ > + MOZ_ASSERT(APZThreadUtils::IsControllerThread()); > + int32_t direction = mControllerLastDragDirection; If I'm reading this correctly, you're using the drag direction from the last touchmove to decide whether to fully show or fully hide the toolbar once the finger is lifted. I'm not sure if that's such a great idea, a lot of flings can result in a small jump in the other direction right at the end as the finger is lifted. But if it feels fine in practice it's fine by me. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:532 (Diff revision 7) > + mControllerPreviousTouch = 0; > + mControllerTotalDistance = 0; > + mControllerDragThresholdReached = false; > + mControllerLastEventTimeStamp = 0; > + > + // Received a UI thread request to show or hide the snap shot during a touch. nit: snapshot ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:741 (Diff revision 7) > + } else if (mControllerState == eNothingPending) { > +// Nothing to do. Don't need this else-if clause if you're going to fall through. Or did you mean to have a return statement here? Also, indent the comment if it's staying. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:775 (Diff revision 7) > + // The compositor is already animating the toolbar so no need to defer. > + mCompositorAnimationDeferred = false; Do you need something like: if (mCompositorAnimationDeferred) { StartCompositorAnimation(...); } here? Or is it guaranteed that you'll get a TOOLBAR_HIDDEN notification after this to kick off the animation? If so a comment here to that effect would be good. ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:808 (Diff revision 7) > + // The toolbar will only hide if dragging up so ignore positive deltas from dragging down. > + if (delta >= 0) { > + return true; > + } > + > + // if the page is 1) too small or 2) too close to the page end then the toolbar can not be hidden did you mean "too close to the bottom"?
Comment on attachment 8856032 [details] Bug 1335895 - part 14: Update LayerManagerComposite render static toolbar snapshot and handle robocop screen captures https://reviewboard.mozilla.org/r/127956/#review134502 ::: gfx/layers/composite/LayerManagerComposite.cpp:1224 (Diff revision 7) > + > +// Used by robocop tests to get a snapshot of the frame buffer. > +void > +LayerManagerComposite::HandlePixelsTarget() > +{ > + if (mScreenPixelsTarget) { invert and early return
Attachment #8856032 - Flags: review?(bugmail) → review+
Comment on attachment 8856030 [details] Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127952/#review134470 > Technically this is not threadsafe, mToolbarState could change in between the comparison and the assignment. If it were an == comparison I'd ask you to use Atomic::compareExchange but as it stands you'll have to do something more convoluted (good ol' Mutex) if you want to guarantee thread-safety here. I see similar unsafe use of mToolbarState all over the place. Sorry, as you pointed out on IRC mToolbarState is only written to on the compositor thread so this is fine.
Comment on attachment 8856030 [details] Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127952/#review134512 ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:149 (Diff revision 7) > + CSSToScreenScale aScale, > + CSSRect aCssPageRect); > + bool IsEnoughtPageToHideToolbar(ScreenIntCoord delta); > + void ShowToolbarIfNotVisible(StaticToolbarState aCurrentToolbarState); > + void TranslateTouchEvent(MultiTouchInput& aTouchEvent, ScreenIntCoord* aDeltaConsumed = nullptr); > + ScreenIntCoord GetFixeLayerMarginsBottom(); Fixe -> Fixed
Comment on attachment 8856042 [details] Bug 1335895 - part 24: Update LayerView.java support new DynamicToolbar https://reviewboard.mozilla.org/r/127976/#review134516 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:91 (Diff revision 7) > + /* package */ final static int FIRST_PAINT = 5; // Sent from compositor after first paint > + /* package */ final static int REQUEST_SHOW_TOOLBAR_IMMEDIATELY = 6; // Sent to compositor requesting toolbar be shown immediately > + /* package */ final static int REQUEST_SHOW_TOOLBAR_ANIMATED = 7; // Sent to compositor requesting toolbar be shown animated > + /* package */ final static int REQUEST_HIDE_TOOLBAR_IMMEDIATELY = 8; // Sent to compositor requesting toolbar be hidden immediately > + /* package */ final static int REQUEST_HIDE_TOOLBAR_ANIMATED = 9; // Sent to compositor requesting toolbar be hidden animated > + /* package */ final static int LAYER_UPDATED = 10; // Sent from compositor when a layer has been updated LAYERS, plural
Attachment #8856042 - Flags: review?(bugmail) → review+
Comment on attachment 8856030 [details] Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127952/#review134524
Attachment #8856030 - Flags: review?(bugmail) → review+
Comment on attachment 8856033 [details] Bug 1335895 - part 15: Update AsyncPanZoomController to notify toolbar animator when FrameMetrics have changed and when root content is scrolled https://reviewboard.mozilla.org/r/127958/#review134496 > The test we use for "steady state" for other purposes is "!IsTransformingState(state)". > > I would put this code inside the existing "IsTransformingState(aOldState) && !IsTransformingState(aNewState)" block below. I did this originally, but it only seems to works for scrolling. It didn't seem to work for when flings end. Did I do something wrong?
Comment on attachment 8856043 [details] Bug 1335895 - part 25: Update GeckoLayerClient.java to support new dynamic toolbar https://reviewboard.mozilla.org/r/127978/#review134534 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/GeckoLayerClient.java (Diff revision 7) > - data.putDouble("y", scrollChange.y / mViewportMetrics.zoomFactor); > - data.putInt("id", id); > - } else { > - data = null; > - } > - EventDispatcher.getInstance().dispatch("Window:Resize", data); This means you can remove the Window:Resize listener in browser.js and code that it uses (including DOMWindowUtils.setNextPaintSyncId, ClientLayerManager::SetNextPaintSyncId, etc all the way to AsyncCompositionManager::mPaintSyncId.
Attachment #8856043 - Flags: review?(bugmail) → review+
Comment on attachment 8856044 [details] Bug 1335895 - part 26: Update NativePanZoomController.java to use LayerView.isGeckoReady and remove setScrollingRootContent https://reviewboard.mozilla.org/r/127980/#review134544 ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/NativePanZoomController.java:114 (Diff revision 7) > > final MotionEvent.PointerCoords coords = new MotionEvent.PointerCoords(); > event.getPointerCoords(0, coords); > final float x = coords.x; > - final float y = coords.y; > + // Scroll events are not adjusted by the AndroidDyanmicToolbarAnimator so adjust the offset here. > + final float y = coords.y - mView.getCurrentToolbarHeight(); Do we not need to do this for mouse events too? (in NativePanZoomController.onMotionEvent for example).
Comment on attachment 8856045 [details] Bug 1335895 - part 27: Update DynamicToolbarAnimator to work as a proxy to the AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127982/#review134546
Attachment #8856045 - Flags: review?(bugmail) → review+
Comment on attachment 8856034 [details] Bug 1335895 - part 16: Update AsyncCompositionManager to support toolbar animator https://reviewboard.mozilla.org/r/127960/#review134550
Attachment #8856034 - Flags: review?(botond) → review+
Comment on attachment 8856035 [details] Bug 1335895 - part 17: Remove SetScrollingRootContent from PAPZ, GeckoContentController, RemoteContentController, and AndroidContentController https://reviewboard.mozilla.org/r/127962/#review134552
Attachment #8856035 - Flags: review?(botond) → review+
Comment on attachment 8856036 [details] Bug 1335895 - part 18: Update gfxPrefs to support browser.ui.scroll-toolbar-threshold https://reviewboard.mozilla.org/r/127964/#review134554 r+ with the corresponding change to mobile.js that kats mentioned
Attachment #8856036 - Flags: review?(botond) → review+
Comment on attachment 8856030 [details] Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127952/#review134470 > Any particular reason you used a Create() method instead of just new'ing at the call site? I don't mind, just curious. When I started, it was just called DynamicToolbarAnimator as I was hoping to not make it completely android specific. So I had the create to allow different sub classes. I was obviously delusional but never went back and removed the Create function. I can do that now.
Comment on attachment 8856030 [details] Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127952/#review134470 > If I'm reading this correctly, you're using the drag direction from the last touchmove to decide whether to fully show or fully hide the toolbar once the finger is lifted. I'm not sure if that's such a great idea, a lot of flings can result in a small jump in the other direction right at the end as the finger is lifted. But if it feels fine in practice it's fine by me. Yeah, I had it like the current toolbar and but it always bothered me that the bar didn't always complete the hide/show in the direction of the drag. So I tried using the drag direction. It seemed to work for *me* but a sample of one is useless. I tried to get other people to try by posting builds on #mobile and asking specifically about the hide/show behavior. I got zero feedback. I can easily be persuaded to change it back. It is still something I'm concerned about.
Comment on attachment 8856030 [details] Bug 1335895 - part 12: Add AndroidDynamicToolbarAnimator https://reviewboard.mozilla.org/r/127952/#review134470 > Do you need something like: > if (mCompositorAnimationDeferred) { > StartCompositorAnimation(...); > } > > here? Or is it guaranteed that you'll get a TOOLBAR_HIDDEN notification after this to kick off the animation? If so a comment here to that effect would be good. The TOOLBAR_HIDDEN will always be sent in response to STATIC_TOOLBAR_READY. If the animation were kicked off before receiving TOOLBAR_HIDDEN, when the real toolbar was hidden, it could reveal the static snapshot midway into an animation. I'll add a comment.
Comment on attachment 8856044 [details] Bug 1335895 - part 26: Update NativePanZoomController.java to use LayerView.isGeckoReady and remove setScrollingRootContent https://reviewboard.mozilla.org/r/127980/#review134544 > Do we not need to do this for mouse events too? (in NativePanZoomController.onMotionEvent for example). Since Bug 1294707, all motion events are touch events again so there aren't any native mouse events sent. If we ever start sending mouse events again, then yes, they will need to be treated like the wheel events.
Comment on attachment 8856036 [details] Bug 1335895 - part 18: Update gfxPrefs to support browser.ui.scroll-toolbar-threshold https://reviewboard.mozilla.org/r/127964/#review134344 So I actually changed gfPref to use int32_t instead of changing mobile.js so it doesn't break anyone who has this pref set.
Comment on attachment 8856033 [details] Bug 1335895 - part 15: Update AsyncPanZoomController to notify toolbar animator when FrameMetrics have changed and when root content is scrolled https://reviewboard.mozilla.org/r/127958/#review134496 > I did this originally, but it only seems to works for scrolling. It didn't seem to work for when flings end. Did I do something wrong? Seems to work now so I switched it back. Must have been a bug in my code that has since been fixed.
Comment on attachment 8856039 [details] Bug 1335895 - part 21: Update nsWindow to support update UiCompositorControllerChild https://reviewboard.mozilla.org/r/127970/#review134350 > Why is mCompositorPaused set back to false here? This warrants a comment if the code is correct, because it's not obvious. The other places that set this back to false are all obviously resuming the compositor so those make sense. Copy/paste error.
Comment on attachment 8856019 [details] Bug 1335895 - part 1: Update CompositorSession to hold a pointer to UiCompositorControllerChild on Android https://reviewboard.mozilla.org/r/127928/#review134846 ::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:804 (Diff revisions 7 - 8) > // The toolbar will only hide if dragging up so ignore positive deltas from dragging down. > if (delta >= 0) { > return true; > } > > - // if the page is 1) too small or 2) too close to the page end then the toolbar can not be hidden > + // if the page is 1) too small or 2) too close to the bottom, then the toolbar can not be hidden Doh. Last time I read "page end" as "page and" which is why it didn't make sense to me...
Comment on attachment 8856036 [details] Bug 1335895 - part 18: Update gfxPrefs to support browser.ui.scroll-toolbar-threshold https://reviewboard.mozilla.org/r/127964/#review134848
Attachment #8856036 - Flags: review?(bugmail) → review+
Comment on attachment 8856044 [details] Bug 1335895 - part 26: Update NativePanZoomController.java to use LayerView.isGeckoReady and remove setScrollingRootContent https://reviewboard.mozilla.org/r/127980/#review134850
Attachment #8856044 - Flags: review?(bugmail) → review+
Comment on attachment 8859826 [details] Bug 1335895 - part 37: Remove PaintSyncId https://reviewboard.mozilla.org/r/131812/#review134852 Great, thanks for getting this done!
Attachment #8859826 - Flags: review?(bugmail) → review+
Pushed by rbarker@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/81de9d1439b0 Android GeckoView Dynamic Toolbar Version 3 r=botond,dvander,jchen,kats
https://hg.mozilla.org/mozilla-central/rev/81de9d1439b0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Blocks: 1358554
No longer blocks: 1358554
Depends on: 1358554
Depends on: 1358592
Depends on: 1358763
Depends on: 1358781
(In reply to Botond Ballo [:botond] from comment #353) > > +Effect* > > +AndroidDynamicToolbarAnimator::GetToolbarEffect(CompositorOGL* gl) > > I would prefer that this function be reviewed by :nical or :mattwoodrow. (In reply to Botond Ballo [:botond] from comment #418) > As with > AndroidDynamicToolbarAnimator::GetToolbarEffect(), please ask :nical or > :mattwoodrow for a review. Randall, did you end up getting these reviews?
Depends on: 1359618
Depends on: 1359659
Depends on: 1360639
(In reply to Botond Ballo [:botond] from comment #483) > (In reply to Botond Ballo [:botond] from comment #353) > > > +Effect* > > > +AndroidDynamicToolbarAnimator::GetToolbarEffect(CompositorOGL* gl) > > > > I would prefer that this function be reviewed by :nical or :mattwoodrow. > > (In reply to Botond Ballo [:botond] from comment #418) > > As with > > AndroidDynamicToolbarAnimator::GetToolbarEffect(), please ask :nical or > > :mattwoodrow for a review. > > Randall, did you end up getting these reviews? Reminder. Please ask for a post-landing review of these compositor changes.
Flags: needinfo?(rbarker)
(In reply to Botond Ballo [:botond] from comment #484) > (In reply to Botond Ballo [:botond] from comment #483) > > (In reply to Botond Ballo [:botond] from comment #353) > > > > +Effect* > > > > +AndroidDynamicToolbarAnimator::GetToolbarEffect(CompositorOGL* gl) > > > > > > I would prefer that this function be reviewed by :nical or :mattwoodrow. > > > > (In reply to Botond Ballo [:botond] from comment #418) > > > As with > > > AndroidDynamicToolbarAnimator::GetToolbarEffect(), please ask :nical or > > > :mattwoodrow for a review. > > > > Randall, did you end up getting these reviews? > > Reminder. Please ask for a post-landing review of these compositor changes. As discussed on IRC, Bug 1358775 will require non-trivial changes to the code in question so I will insure that one of them is included on the review for that bug.
Flags: needinfo?(rbarker)
Depends on: 1361880
Blocks: 1361881
Depends on: 1364194
Depends on: 1365161
Mark 54 won't fix as this is late for Beta54.
status-firefox54: affected → wontfix
Depends on: 1377224
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 55 → mozilla55

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

Component: General → Toolbar
Component: Toolbar → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: