Implement IPDL version of CompositorWidgetProxy

RESOLVED FIXED in Firefox 50

Status

()

Core
Graphics: Layers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(7 attachments)

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.
Created attachment 8765316 [details] [diff] [review]
part 1, don't require nsWindow

Don't require nsWindow when creating a WinCompositorWidgetProxy.
Attachment #8765316 - Flags: review?(jmathies)
Created attachment 8765317 [details] [diff] [review]
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.)
Attachment #8765317 - Flags: review?(jmathies)
Created attachment 8765318 [details] [diff] [review]
part 3, move InProcessCompositorWidget

This puts InProcessCompositorWidget into its own file. Not strictly necessary but makes things a little nicer.
Attachment #8765318 - Flags: review?(jmathies)
Created attachment 8765319 [details] [diff] [review]
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.
Attachment #8765319 - Flags: review?(jmathies)
Created attachment 8765320 [details] [diff] [review]
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.
Attachment #8765320 - Flags: review?(jmathies)
Created attachment 8765323 [details] [diff] [review]
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.
Attachment #8765323 - Flags: review?(jmathies)
Created attachment 8765324 [details] [diff] [review]
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.
Attachment #8765324 - Flags: review?(wmccloskey)

Updated

2 years ago
Attachment #8765316 - Flags: review?(jmathies) → review+

Comment 8

2 years ago
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.
Attachment #8765317 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8765318 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8765319 - Flags: review?(jmathies) → review+

Comment 9

2 years ago
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.
Attachment #8765320 - Flags: review?(jmathies) → review+

Comment 10

2 years ago
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'.

Updated

2 years ago
Attachment #8765323 - Flags: review?(jmathies) → review+
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.
Attachment #8765324 - Flags: review?(wmccloskey) → review+

Comment 12

2 years ago
Pushed by danderson@mozilla.com:
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)
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.
Depends on: 1276011

Comment 14

2 years ago
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48bd14a01b55
Back out 7 changesets for Windows Marionette crashes

Comment 15

2 years ago
Pushed by danderson@mozilla.com:
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)

Updated

2 years ago
Depends on: 1284092

Updated

2 years ago
No longer depends on: 1284092
No longer depends on: 1276011
You need to log in before you can comment on or make changes to this bug.