1306083 - Support separate layer clients for each GeckoView

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Since we still use GekcoLayerClient to some degree, we should support using a separate layer client for each GeckoView.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 1. Call GeckoLayerClient directly in gfx code (v1)

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.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 2. Pass in window object for nsIAndroidBridge calls (v1)

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.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 3. Remove obsolete code from AndroidBridge (v1)

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.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 1. Use AndroidCompositorWidget to access GeckoLayerClient (v2)

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.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/da5048d1f778
https://hg.mozilla.org/mozilla-central/rev/c8bc3190f954
https://hg.mozilla.org/mozilla-central/rev/db8b6698e822