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.

Don't require nsWindow when creating a WinCompositorWidgetProxy.

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.)

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

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.

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.

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.

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 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 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 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 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.

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.

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

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)

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