Closed Bug 1306083 Opened 3 years ago Closed 3 years ago

Support separate layer clients for each GeckoView

Categories

(GeckoView :: General, defect)

All
Android
defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(3 files, 1 obsolete file)

Since we still use GekcoLayerClient to some degree, we should support using a separate layer client for each GeckoView.
Get GeckoLayerClient from the widget and call its methods directly in
AsyncCompositionManager, instead of going through AndroidBridge. This
declutters AndroidBridge and enables support for multiple GeckoView.
Attachment #8795909 - Flags: review?(snorp)
Make contentDocumentChanged and isContentDocumentDisplayed calls require
the caller to pass in a window object, so that we can get the widget and
GeckoLayerClient from the window object. This way these calls no longer
depend on having a global layer client in AndroidBridge.
Attachment #8795910 - Flags: review?(snorp)
Remove the global layer client object from AndroidBridge, now that it's
no longer used. Also remove other associated calls that are no longer
used.
Attachment #8795911 - Flags: review?(snorp)
Comment on attachment 8795909 [details] [diff] [review]
1. Call GeckoLayerClient directly in gfx code (v1)

Review of attachment 8795909 [details] [diff] [review]:
-----------------------------------------------------------------

You should probably have kats review the ACM bit
Attachment #8795909 - Flags: review?(snorp)
Attachment #8795909 - Flags: review?(bugmail)
Attachment #8795909 - Flags: review+
Comment on attachment 8795909 [details] [diff] [review]
1. Call GeckoLayerClient directly in gfx code (v1)

Review of attachment 8795909 [details] [diff] [review]:
-----------------------------------------------------------------

See comment below. I think making the changes I suggest below would be good, but if you don't want to for whatever reason I won't object to landing as-is.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +1595,5 @@
> +static jni::DependentRef<java::GeckoLayerClient>
> +GetJavaLayerClient(LayerManagerComposite* aLayerManager)
> +{
> +  nsIWidget* const widget =
> +      aLayerManager->GetCompositor()->GetWidget()->RealWidget();

As-is this is not going to be compatible with GPU process separation. I don't know how high that is on the list of priorities for Fennec relative to this geckoview work but it's something to consider. If you want to make it work properly you'll have to add these functions to the CompositorWidget interface, and implement them in InProcessCompositorWidget to delegate to the android widget. Then when you want to support GPU process separation you'll need to add the corresponding remote implementation.

That being said, even if you don't want to do that right now, I think it makes a lot of sense to move some of this to be on android's nsWindow instead. That is, instead of having a GetJavaLayerClient(LayerManagerComposite*) here, make this a GetAndroidWidget(LayerManagerComposite*), and then in e.g. AsyncCompositionManager::SetFirstPaintViewport(...) call GetAndroidWidget(mLayerManager)->SetFirstPaintViewport(...).

All the wrapper stuff that you moved from AndroidBridge to this class would then live in widget/android/nsWindow.cpp which IMO makes more sense (rather than polluting this gfx code with JNI goop, or leaving it in AndroidBridge which is not per-geckoview). And it makes the transition to GPU process smoother.
Attachment #8795909 - Flags: review?(bugmail) → review+
Attachment #8795910 - Flags: review?(snorp) → review+
Attachment #8795911 - Flags: review?(snorp) → review+
Here's what I have now. I chose to add AndroidCompositorWidget, which is still
an InProcessCompositorWidget at the moment. When we implement OOP compositing,
we can switch it to a remote version.
Attachment #8796585 - Flags: review?(bugmail)
Attachment #8795909 - Attachment is obsolete: true
Comment on attachment 8796585 [details] [diff] [review]
1. Use AndroidCompositorWidget to access GeckoLayerClient (v2)

Review of attachment 8796585 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, thanks. Passing f? to dvander as well just so he's aware of this.

::: widget/android/AndroidCompositorWidget.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_widget_InProcessAndroidCompositorWidget_h
> +#define mozilla_widget_InProcessAndroidCompositorWidget_h

Update #ifdef to mozilla_widget_AndroidCompositorWidget_h
Attachment #8796585 - Flags: review?(bugmail)
Attachment #8796585 - Flags: review+
Attachment #8796585 - Flags: feedback?(dvander)
Comment on attachment 8796585 [details] [diff] [review]
1. Use AndroidCompositorWidget to access GeckoLayerClient (v2)

Review of attachment 8796585 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, though would have to revisit to get OOP-compositor support on Android.
Attachment #8796585 - Flags: feedback?(dvander) → feedback+
Comment on attachment 8796585 [details] [diff] [review]
1. Use AndroidCompositorWidget to access GeckoLayerClient (v2)

Review of attachment 8796585 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/CompositorWidget.h
@@ +250,5 @@
>    virtual X11CompositorWidget* AsX11() {
>      return nullptr;
>    }
> +  virtual AndroidCompositorWidget* AsAndroid() {
> +    return nullptr;

Oh, i just realized this probably needs to be wrapped in an #ifdef for android, since AndroidCompositorWidget won't be defined otherwise.
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da5048d1f778
1. Use AndroidCompositorWidget to access GeckoLayerClient; r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8bc3190f954
2. Pass in window object for nsIAndroidBridge calls; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/db8b6698e822
3. Remove obsolete code from AndroidBridge; r=snorp
https://hg.mozilla.org/mozilla-central/rev/da5048d1f778
https://hg.mozilla.org/mozilla-central/rev/c8bc3190f954
https://hg.mozilla.org/mozilla-central/rev/db8b6698e822
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 52 → mozilla52
You need to log in before you can comment on or make changes to this bug.