1281998 - Implement IPDL version of CompositorWidgetProxy

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

The goal of this bug is to introduce a new actor, PCompositorWidget, that can be sent to PCompositorBridgeParent. On that end, it will construct a CompositorWidgetProxy, which the UI process can interact with via IPC.

To do this I'd like to do another small refactoring, since the original CompositorWidgetProxy design doesn't 100% fit. It contains both Compositor-only methods and Widget-only methods, and we need to split those out to make IPDL work.

CompositorWidgetProxy will be renamed "CompositorWidget", and CompositorSession will be responsible for creating it. The widget will get a new class representing its side of the actor - probably called "CompositorWidgetProxy", and this will be platform-specific. We'll still have a generic wrapper version for in-process widgets.

Comment 1

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

Attachment: part 1, don't require nsWindow

Don't require nsWindow when creating a WinCompositorWidgetProxy.

Comment 2

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

Attachment: part 2, rename CompositorWidgetProxy

Sorry for all the churn here, Jim... this renames CompositorWidgetProxy to CompositorWidget. My justification for dropping the "Proxy" is that whether or not it's a proxy is sort of an implementation detail, and the names get confusing later on with IPDL.

With all these patches in place, the widget-specific callbacks in CompositorWidget, like "UpdateTransparency", get factored out into a CompositorWigetDelegate. This makes the IPDL implementation super easy - the widget only accesses the delegate, which may be a CompositorWidget in-process or a PCompositorWidgetChild out-of-process. Meanwhile, the PCompositorWidgetParent can simply forward these methods into the real CompositorWiget. (See part 7.)

Comment 3

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

Attachment: part 3, move InProcessCompositorWidget

This puts InProcessCompositorWidget into its own file. Not strictly necessary but makes things a little nicer.

Comment 4

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

Attachment: part 4, CompositorSession should own CompositorWidget

CompositorWidget only contains methods for the compositor. nsIWidget shouldn't own it. This patch moves ownership to CompositorSession, and the next few patches will remove it from nsIWidget completely.

Comment 5

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

Attachment: part 5, move CompositorWidget construction

This patch moves CompositorWidget construction out of nsIWidget - necessary since CompositorBridgeParent won't have access to nsIWidget in the GPU process. Instead, we introduce a new "CompositorWidgetInitData" type and a factory method on CompositorWidget.

On Windows, CompositorWidgetInitData contains the HWND, plugin key, and transparency mode. No other platforms have cross-process widgets yet, so they get a stub structure.

Comment 6

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

Attachment: part 6, factor nsIWidget callbacks out of CompositorWidget

This patch factors nsIWidget callbacks out of CompositorWidget into a new class, CompositorWidgetDelegate.

Since these callbacks are very much platform-specific, the widget is responsible for declaring a CompositorWidgetDelegate class if it needs one. The default is just a forward declaration.

This allows us to remove nsIWidget's direct access of CompositorWidget, and to move forward with an IPDL implementation.

Now, Windows is a weird case: the drawing code is also shared with BasicLayers, but it exposes methods we don't necessarily want in CompositorWidgetDelegate. Instead, nsWindow now owns a separate, in-process CompositorWidget specifically for BasicLayers. It's only allocated if we don't use OMTC.

Comment 7

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

Attachment: part 7, IPDL implementation

This patch implements an almost-complete IPDL version of CompositorWidget, for Windows, as a subprotocol of PCompositorBridge. The only method not supported yet is GetCompositorVsyncDispatcher. I'll handle that in a separate bug.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8765317 [details] [diff] [review]
part 2, rename CompositorWidgetProxy

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

::: gfx/ipc/CompositorSession.cpp
@@ +14,5 @@
>  class InProcessCompositorSession final : public CompositorSession
>  {
>  public:
>    InProcessCompositorSession(
> +    widget::CompositorWidget* aWidget,

I prefer this name actually, so lgtm.

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8765320 [details] [diff] [review]
part 5, move CompositorWidget construction

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

::: gfx/ipc/CompositorSession.cpp
@@ +74,5 @@
>                                                         bool aUseExternalSurfaceSize,
>                                                         const gfx::IntSize& aSurfaceSize)
>  {
> +  CompositorWidgetInitData initData;
> +  aWidget->GetCompositorWidgetInitData(&initData);

MOZ_ASSERT on aWidget

::: widget/CompositorWidget.h
@@ +40,5 @@
>    NS_INLINE_DECL_REFCOUNTING(mozilla::widget::CompositorWidget)
>  
>    /**
> +   * Create an in-process compositor widget. aWidget may be ignored if the
> +   * platform ddoes not require it.

nit - ddoes

::: widget/InProcessCompositorWidget.cpp
@@ +13,5 @@
> +#if !defined(XP_WIN)
> +/* static */ RefPtr<CompositorWidget>
> +CompositorWidget::CreateLocal(const CompositorWidgetInitData& aInitData, nsIWidget* aWidget)
> +{
> +  return new InProcessCompositorWidget(static_cast<nsBaseWidget*>(aWidget));

or maybe add the MOS_ASSERT in CreateLocal. The comment on CreateLocal seems to imply a null might be ok in some cases.

::: widget/windows/nsWindow.cpp
@@ +3626,5 @@
>      MOZ_ASSERT(!mCompositorWidget);
>  
>      // Ensure we have a widget proxy even if we're not using the compositor,
>      // since all our transparent window handling lives there.
> +    CompositorWidgetInitData initData(

This is a nice addition.

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8765323 [details] [diff] [review]
part 6, factor nsIWidget callbacks out of CompositorWidget

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

nice improvement. I like 'delegate' a lot more than the old 'proxy'.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8765324 [details] [diff] [review]
part 7, IPDL implementation

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

::: gfx/layers/ipc/CompositorBridgeChild.cpp
@@ +964,5 @@
> +bool
> +CompositorBridgeChild::DeallocPCompositorWidgetChild(PCompositorWidgetChild* aActor)
> +{
> +#ifdef MOZ_WIDGET_SUPPORTS_OOP_COMPOSITING
> +  return true;

You already pointed out that this needs to delete the object.

::: widget/windows/CompositorWidgetChild.cpp
@@ +19,5 @@
> +
> +void
> +CompositorWidgetChild::EnterPresentLock()
> +{
> +  Unused << SendEnterPresentLock();

You need to avoid sending messages like this after ActorDestroy has been called. It sounds like you'll do that by somehow removing the widget's access to this object. In any case, it would be nice to have an assertion that these Send functions never get called. One way to do that is to implement ProcessingError on CompositorBridgeChild and have it do some sort of assertion. I think we'll get a MsgDropped error there.

::: widget/windows/CompositorWidgetChild.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 _widget_windows_CompositorWidgetChild_h__
> +#define _widget_windows_CompositorWidgetChild_h__

Shouldn't have the initial or final underscores here. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices.

::: widget/windows/PCompositorWidget.ipdl
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

c-basic-offset should be 2 for this file.

::: widget/windows/WinCompositorWidget.h
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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 _widget_windows_CompositorWidgetParent_h__

Would be nice to fix this too.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5a9585754b1
Allow creating WinCompositorWidgetProxy without an nsWindow. (bug 1281998 part 1, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/99d1da1293b7
Rename CompositorWidgetProxy files to CompositorWidget. (bug 1281998 part 2, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/54a0e73f6906
Move InProcessCompositorWidget to its own file. (bug 1281998 part 3, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/74198f88fa37
Move CompositorWidget ownership from nsWindow to CompositorSession. (bug 1281998 part 4, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a72929c0c3ec
Move CompositorWidget construction out of nsIWidget. (bug 1281998 part 5, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d4fedfd7eb
Extract a delegate interface out of WinCompositorWidget. (bug 1281998 part 6, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d806fac2c856
Implement remote CompositorWidgets on Windows. (bug 1281998 part 7, r=billm)

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3d503858ee3347ad5361af51de16e8bf049de4de for Windows Marionette crashes, but thanks to bug 1276011 you don't actually get to know where you are crashing.

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48bd14a01b55
Back out 7 changesets for Windows Marionette crashes

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/456f9b45d6d6
Allow creating WinCompositorWidgetProxy without an nsWindow. (bug 1281998 part 1, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cbb330c02c7
Rename CompositorWidgetProxy files to CompositorWidget. (bug 1281998 part 2, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/887adb3d9482
Move InProcessCompositorWidget to its own file. (bug 1281998 part 3, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1009ef8e1ed6
Move CompositorWidget ownership from nsWindow to CompositorSession. (bug 1281998 part 4, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/67eaf2f074ba
Move CompositorWidget construction out of nsIWidget. (bug 1281998 part 5, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/91f81099937f
Extract a delegate interface out of WinCompositorWidget. (bug 1281998 part 6, r=jimm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdd62c877b66
Implement remote CompositorWidgets on Windows. (bug 1281998 part 7, r=billm)

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/456f9b45d6d6
https://hg.mozilla.org/mozilla-central/rev/0cbb330c02c7
https://hg.mozilla.org/mozilla-central/rev/887adb3d9482
https://hg.mozilla.org/mozilla-central/rev/1009ef8e1ed6
https://hg.mozilla.org/mozilla-central/rev/67eaf2f074ba
https://hg.mozilla.org/mozilla-central/rev/91f81099937f
https://hg.mozilla.org/mozilla-central/rev/bdd62c877b66