Change AudioOutputDeviceClientImpl to use WebLocalFrameBase and move to modules.

Change AudioOutputDeviceClientImpl to use WebLocalFrameBase and move to modules.

AudioOutputDeviceClientImpl is the first class that is using WebLocalFrameBase
to break the dependency on WebLocalFrameImpl, and as a result this CL contains
a lot off additional once only changes to support this.

Changes contained in this cl:
- Create a WebInitializer class that is called when WebKit is being initialized
  for the process. This class can then contain once-per-process initialization
  code.
- Use WebInitialize to register callbacks to create WebLocalFrameImpl from
  WebLocalFrameBase. Without this callback mechanism we would need to put
  WebLocalFrameBase::FromFrame implementation intl WebLocalFrameImpl, but this
  will not link as modules/ cannot depend on web/. The callbacks allow us to
  keep WebLocalFrameBase independent from WebLocalFrameImpl.
- Add a pure virtual Client() method to WebLocalFrameBase that is used by
  AudioOutputDeviceCientImpl.
- Move AudioOutputDeviceClientImpl to modules/audio_output_devices.

I suppose it is possible to merge AudioOutputDeviceClientImpl with 
AudioOutputDeviceClient, but my preference would be to do this in a followup
CL.

BUG=708879

[slangley, 2017-05-05 03:50:32]

Description was changed from

==========
Change AudioOutputDeviceClientImpl to use WebLocalFrameBase and move to modules.


BUG=708879
==========

to

==========
Change AudioOutputDeviceClientImpl to use WebLocalFrameBase and move to modules.

AudioOutputDeviceClientImpl is the first class that is using WebLocalFrameBase
to break the dependency on WebLocalFrameImpl, and as a result this CL contains
a lot off additional once only changes to support this.

Changes contained in this cl:
- Create a WebInitializer class that is called when WebKit is being initialized
  for the process. This class can then contain once-per-process initialization
  code.
- Use WebInitialize to register callbacks to create WebLocalFrameImpl from
  WebLocalFrameBase. Without this callback mechanism we would need to put
  WebLocalFrameBase::FromFrame implementation intl WebLocalFrameImpl, but this
  will not link as modules/ cannot depend on web/. The callbacks allow us to
  keep WebLocalFrameBase independent from WebLocalFrameImpl.
- Add a pure virtual Client() method to WebLocalFrameBase that is used by
  AudioOutputDeviceCientImpl.
- Move AudioOutputDeviceClientImpl to modules/audio_output_devices.

I suppose it is possible to merge AudioOutputDeviceClientImpl with 
AudioOutputDeviceClient, but my preference would be to do this in a followup
CL.

BUG=708879
==========

[slangley, 2017-05-05 03:50:52]

slangley@chromium.org changed reviewers:
+ sashab@chromium.org

[sashab, 2017-05-05 03:54:38]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 03:55:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sashab, 2017-05-05 04:00:17]

lgtm

[sashab, 2017-05-05 04:05:41]

Only thing I'm not sure about is the callback syntax, haven't seen that much in
blink. But I think in this case its fine, its a very simple use case and its
temporary anyway.

[slangley, 2017-05-05 04:07:30]

slangley@chromium.org changed reviewers:
+ haraken@chromium.org

[slangley, 2017-05-05 04:07:30]

haraken@ - ptal

WDYT of the function pointer style? Also, WDYT in general? ;)

Thanks

[dcheng, 2017-05-05 04:54:51]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-05-05 04:54:52]

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/WebLocalFrameBase.cpp (right):

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/WebLocalFrameBase.cpp:21: return
create_local_frame_ref_callback_(frame);
Maybe this can just use the pointer version?

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClientImpl.cpp
(right):

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClientImpl.cpp:7:
#include <memory>
Nit: newline after

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebKit.cpp (right):

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebKit.cpp:83: (WTF::WrapUnique(new
WebInitializer)));
Nit: no need for unique_ptr here: DEFINE_STATIC_LOCAL leaks the object anyway,
so it's just an additional indirection with no cleanup.

[commit-bot: I haz the power, 2017-05-05 05:02:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 05:02:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[slangley, 2017-05-05 06:25:01]

dcheng@ - ptal

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/WebLocalFrameBase.cpp (right):

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/WebLocalFrameBase.cpp:21: return
create_local_frame_ref_callback_(frame);
On 2017/05/05 at 04:54:52, dcheng wrote:
> Maybe this can just use the pointer version?

Done

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClientImpl.cpp
(right):

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/audio_output_devices/AudioOutputDeviceClientImpl.cpp:7:
#include <memory>
On 2017/05/05 at 04:54:52, dcheng wrote:
> Nit: newline after

Done

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebKit.cpp (right):

https://codereview.chromium.org/2863913002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebKit.cpp:83: (WTF::WrapUnique(new
WebInitializer)));
On 2017/05/05 at 04:54:52, dcheng wrote:
> Nit: no need for unique_ptr here: DEFINE_STATIC_LOCAL leaks the object anyway,
so it's just an additional indirection with no cleanup.

Done.

[slangley, 2017-05-05 06:26:23]

The CQ bit was checked by slangley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 06:26:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-05-05 06:29:58]

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/WebLocalFrameBase.h (right):

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebLocalFrameBase.h:33: // so that
WebLocalFrameBase can instantiate the concrete class.
I'm really tempted to say that we should just #include WebLocalFrameImpl.h in
WebLocalFrameBase.cpp and avoid the callback indirection. Are there downsides to
doing that, given that this is temporary code?

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInitializer.cpp (right):

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInitializer.cpp:16: return
static_cast<WebLocalFrameBase*>(
Nit: I think this static cast isn't needed, as WebLocalFrameBase is a subclass.

[commit-bot: I haz the power, 2017-05-05 07:20:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 07:20:20]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[slangley, 2017-05-05 09:25:08]

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/WebLocalFrameBase.h (right):

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebLocalFrameBase.h:33: // so that
WebLocalFrameBase can instantiate the concrete class.
On 2017/05/05 at 06:29:58, dcheng wrote:
> I'm really tempted to say that we should just #include WebLocalFrameImpl.h in
WebLocalFrameBase.cpp and avoid the callback indirection. Are there downsides to
doing that, given that this is temporary code?

There are scenarios that don't link in libblink_web so we can't do this. Also,
this is not the only use case for this kind of layering, so providing this
mechanism is probably the easiest way to deal with other cases as well (i.e. We
should have done this for WebViewBase & WebViewImpl - right now we downcast.)

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInitializer.cpp (right):

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInitializer.cpp:16: return
static_cast<WebLocalFrameBase*>(
On 2017/05/05 at 06:29:58, dcheng wrote:
> Nit: I think this static cast isn't needed, as WebLocalFrameBase is a
subclass.

Doesn't compile without the cast unfortunately - get no viable conversion
errors.

[haraken, 2017-05-05 13:48:51]

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/WebLocalFrameBase.h (right):

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebLocalFrameBase.h:33: // so that
WebLocalFrameBase can instantiate the concrete class.
On 2017/05/05 09:25:08, slangley wrote:
> On 2017/05/05 at 06:29:58, dcheng wrote:
> > I'm really tempted to say that we should just #include WebLocalFrameImpl.h
in
> WebLocalFrameBase.cpp and avoid the callback indirection. Are there downsides
to
> doing that, given that this is temporary code?
> 
> There are scenarios that don't link in libblink_web so we can't do this. Also,
> this is not the only use case for this kind of layering, so providing this
> mechanism is probably the easiest way to deal with other cases as well (i.e.
We
> should have done this for WebViewBase & WebViewImpl - right now we downcast.)

Yeah, I want to avoid using the callback pattern.

Can we probably use ChromeClient? We can define
ChromeClient::createWebLocalFrameImpl as a virtual method and override the
behavior in ChromeClientImpl.

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/audio_output_devices/BUILD.gn (right):

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/audio_output_devices/BUILD.gn:12:
"AudioOutputDeviceClientImpl.h",

Now that both AudioOutputDeviceClient and AudioOutputDeviceClientImpl are in
modules/, can we simply remove the Impl class?

[slangley, 2017-05-07 00:12:10]

haraken@ - your thoughts?

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/WebLocalFrameBase.h (right):

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebLocalFrameBase.h:33: // so that
WebLocalFrameBase can instantiate the concrete class.
On 2017/05/05 at 13:48:51, haraken wrote:
> On 2017/05/05 09:25:08, slangley wrote:
> > On 2017/05/05 at 06:29:58, dcheng wrote:
> > > I'm really tempted to say that we should just #include WebLocalFrameImpl.h
in
> > WebLocalFrameBase.cpp and avoid the callback indirection. Are there
downsides to
> > doing that, given that this is temporary code?
> > 
> > There are scenarios that don't link in libblink_web so we can't do this.
Also,
> > this is not the only use case for this kind of layering, so providing this
> > mechanism is probably the easiest way to deal with other cases as well (i.e.
We
> > should have done this for WebViewBase & WebViewImpl - right now we
downcast.)
> 
> Yeah, I want to avoid using the callback pattern.
> 
> Can we probably use ChromeClient? We can define
ChromeClient::createWebLocalFrameImpl as a virtual method and override the
behavior in ChromeClientImpl.

We don't have a way to get a reference to a ChromeClient object (that I can see)
at this point of the code (at least cleanly).

If you're opposed to callbacks how about I create a Factory class that can
instantiate these *Impl types - instead of trying to wedge it into something
like ChromeClient?

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/audio_output_devices/BUILD.gn (right):

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/audio_output_devices/BUILD.gn:12:
"AudioOutputDeviceClientImpl.h",
On 2017/05/05 at 13:48:51, haraken wrote:
> Now that both AudioOutputDeviceClient and AudioOutputDeviceClientImpl are in
modules/, can we simply remove the Impl class?

Yes - but i plan on doing this in a followup CL so as to keep this CL concise.
Is that OK?

[haraken, 2017-05-08 00:01:58]

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/WebLocalFrameBase.h (right):

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebLocalFrameBase.h:33: // so that
WebLocalFrameBase can instantiate the concrete class.
On 2017/05/07 00:12:10, slangley wrote:
> On 2017/05/05 at 13:48:51, haraken wrote:
> > On 2017/05/05 09:25:08, slangley wrote:
> > > On 2017/05/05 at 06:29:58, dcheng wrote:
> > > > I'm really tempted to say that we should just #include
WebLocalFrameImpl.h
> in
> > > WebLocalFrameBase.cpp and avoid the callback indirection. Are there
> downsides to
> > > doing that, given that this is temporary code?
> > > 
> > > There are scenarios that don't link in libblink_web so we can't do this.
> Also,
> > > this is not the only use case for this kind of layering, so providing this
> > > mechanism is probably the easiest way to deal with other cases as well
(i.e.
> We
> > > should have done this for WebViewBase & WebViewImpl - right now we
> downcast.)
> > 
> > Yeah, I want to avoid using the callback pattern.
> > 
> > Can we probably use ChromeClient? We can define
> ChromeClient::createWebLocalFrameImpl as a virtual method and override the
> behavior in ChromeClientImpl.
> 
> We don't have a way to get a reference to a ChromeClient object (that I can
see)
> at this point of the code (at least cleanly).
> 
> If you're opposed to callbacks how about I create a Factory class that can
> instantiate these *Impl types - instead of trying to wedge it into something
> like ChromeClient?

When you call WebLocalFrameImpl::FromFrame(Frame*), you already have a pointer
to Frame. You can easily get the ChromeClient by frame->page()->chromeClient(),
right?

[slangley, 2017-05-08 01:47:50]

PTAL

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/WebLocalFrameBase.h (right):

https://codereview.chromium.org/2863913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebLocalFrameBase.h:33: // so that
WebLocalFrameBase can instantiate the concrete class.
On 2017/05/08 at 00:01:58, haraken wrote:
> On 2017/05/07 00:12:10, slangley wrote:
> > On 2017/05/05 at 13:48:51, haraken wrote:
> > > On 2017/05/05 09:25:08, slangley wrote:
> > > > On 2017/05/05 at 06:29:58, dcheng wrote:
> > > > > I'm really tempted to say that we should just #include
WebLocalFrameImpl.h
> > in
> > > > WebLocalFrameBase.cpp and avoid the callback indirection. Are there
> > downsides to
> > > > doing that, given that this is temporary code?
> > > > 
> > > > There are scenarios that don't link in libblink_web so we can't do this.
> > Also,
> > > > this is not the only use case for this kind of layering, so providing
this
> > > > mechanism is probably the easiest way to deal with other cases as well
(i.e.
> > We
> > > > should have done this for WebViewBase & WebViewImpl - right now we
> > downcast.)
> > > 
> > > Yeah, I want to avoid using the callback pattern.
> > > 
> > > Can we probably use ChromeClient? We can define
> > ChromeClient::createWebLocalFrameImpl as a virtual method and override the
> > behavior in ChromeClientImpl.
> > 
> > We don't have a way to get a reference to a ChromeClient object (that I can
see)
> > at this point of the code (at least cleanly).
> > 
> > If you're opposed to callbacks how about I create a Factory class that can
> > instantiate these *Impl types - instead of trying to wedge it into something
> > like ChromeClient?
> 
> When you call WebLocalFrameImpl::FromFrame(Frame*), you already have a pointer
to Frame. You can easily get the ChromeClient by frame->page()->chromeClient(),
right?

Seems true - i wasn't able to grok that many indirections ;)

Done.

[slangley, 2017-05-08 01:47:59]

The CQ bit was checked by slangley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 01:48:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 03:01:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 03:01:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2017-05-08 04:25:24]

LGTM

https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/WebLocalFrameBase.h (right):

https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebLocalFrameBase.h:27: CORE_EXPORT static
WebLocalFrameBase* FromFrame(LocalFrame*);

FromFrame => Create ?

https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1250: WebLocalFrameBase*
ChromeClientImpl::CreateWebLocalFrameBase(

Maybe should we call this CreateWebLocalFrameImpl?

[dcheng, 2017-05-08 04:29:44]

https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1250: WebLocalFrameBase*
ChromeClientImpl::CreateWebLocalFrameBase(
On 2017/05/08 04:25:24, haraken wrote:
> 
> Maybe should we call this CreateWebLocalFrameImpl?

This isn't actually creating any objects though, is it?

[haraken, 2017-05-08 04:33:54]

https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/WebLocalFrameBase.h (right):

https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/WebLocalFrameBase.h:26: // Creation
Methods.

Reword this. As dcheng@ said, this is not creating any object.

https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:1250: WebLocalFrameBase*
ChromeClientImpl::CreateWebLocalFrameBase(
On 2017/05/08 04:29:44, dcheng wrote:
> On 2017/05/08 04:25:24, haraken wrote:
> > 
> > Maybe should we call this CreateWebLocalFrameImpl?
> 
> This isn't actually creating any objects though, is it?

Ah, right. FromFrame() sounds nice.

[slangley, 2017-05-08 05:00:17]

On 2017/05/08 at 04:33:54, haraken wrote:
>
https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/WebLocalFrameBase.h (right):
> 
>
https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/WebLocalFrameBase.h:26: // Creation
Methods.
> 
> Reword this. As dcheng@ said, this is not creating any object.
> 
>
https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/2863913002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/ChromeClientImpl.cpp:1250: WebLocalFrameBase*
ChromeClientImpl::CreateWebLocalFrameBase(
> On 2017/05/08 04:29:44, dcheng wrote:
> > On 2017/05/08 04:25:24, haraken wrote:
> > > 
> > > Maybe should we call this CreateWebLocalFrameImpl?
> > 
> > This isn't actually creating any objects though, is it?
> 
> Ah, right. FromFrame() sounds nice.

Turns out there is a linker problem, as we need to access WebFrameClient() from
AudioOutputDeviceClientImpl which in turn needs WebFrame which needs a bunch of
stuff. I will create a new CL with for the parts of WebLocalFrameBase::FromFrame
with a simpler use case and then come back to this CL later.