Introduced WebFactory and WebFactoryImpl for constructing web/ classes

Introduced WebFactory and WebFactoryImpl for constructing web/ classes

Introduced WebFactory and WebFactoryImpl for constructing web/ classes
outside of core/.

This is a temporary class that will be removed once web/ is removed.

BUG=712963
Review-Url: https://codereview.chromium.org/2887523003
Cr-Commit-Position: refs/heads/master@{#475451}
Committed: https://chromium.googlesource.com/chromium/src/+/8d376e7d11c6d2efe83f46fa043f036b0f6246fe

[sashab, 2017-05-16 05:04:14]

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

[sashab, 2017-05-16 05:04:45]

sashab@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org, slangley@chromium.org

[sashab, 2017-05-16 05:04:45]

thoughts welcome :)

[commit-bot: I haz the power, 2017-05-16 05:04:57]

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-16 05:59:23]

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

[commit-bot: I haz the power, 2017-05-16 05:59:24]

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

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

Ping haraken/dcheng thoughts.

[dcheng, 2017-05-17 04:35:58]

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/WebFactory.h (right):

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/WebFactory.h:6: #define WebFactory_h
Maybe this file should be in page? What other classes is this expected to vend?

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/WebFactory.h:32: };
Nit: newline for consistency

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/WebFactory.h:33: }
Nit: // namespace blink

(same below)

[slangley, 2017-05-17 04:39:44]

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/WebFactory.h (right):

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/WebFactory.h:6: #define WebFactory_h
On 2017/05/17 at 04:35:58, dcheng (in AEST) wrote:
> Maybe this file should be in page? What other classes is this expected to
vend?

We'll use this class to construct many of the web/*Impl classes
(WebLocalFrameImpl, WebPluginContainerImpl, ChromeClientImpl) etc etc.

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/WebFactory.h:23: CORE_EXPORT static void
SetInstance(WebFactory*);
not sure if you need these additional CORE_EXPORT if the class is already
CORE_EXPORT.

[sashab, 2017-05-22 03:44:28]

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

[commit-bot: I haz the power, 2017-05-22 03:44:38]

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-22 03:45:06]

Ty all; ptal at new code with comments done.

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/WebFactory.h (right):

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/WebFactory.h:6: #define WebFactory_h
On 2017/05/17 at 04:39:44, slangley wrote:
> On 2017/05/17 at 04:35:58, dcheng (in AEST) wrote:
> > Maybe this file should be in page? What other classes is this expected to
vend?
> 
> We'll use this class to construct many of the web/*Impl classes
(WebLocalFrameImpl, WebPluginContainerImpl, ChromeClientImpl) etc etc.

Yeah, where do you think this file belongs? I was thinking core/exported even.

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/WebFactory.h:23: CORE_EXPORT static void
SetInstance(WebFactory*);
On 2017/05/17 at 04:39:44, slangley wrote:
> not sure if you need these additional CORE_EXPORT if the class is already
CORE_EXPORT.

Removed.

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/WebFactory.h:32: };
On 2017/05/17 at 04:35:57, dcheng wrote:
> Nit: newline for consistency

Done for both

https://codereview.chromium.org/2887523003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/WebFactory.h:33: }
On 2017/05/17 at 04:35:57, dcheng wrote:
> Nit: // namespace blink
> 
> (same below)

Done for both

[sashab, 2017-05-22 03:51:35]

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

[commit-bot: I haz the power, 2017-05-22 03:51:50]

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-22 04:48:24]

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

[commit-bot: I haz the power, 2017-05-22 04:48:25]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dcheng, 2017-05-22 09:16:49]

LGTM from my perspective (but I'd like to hear haraken's opinion)

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/WebFactory.cpp (right):

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/WebFactory.cpp:19: DCHECK(factory);
Nit: in Blink, it's more customary to pass never-null mutable pointers as
mutable refs.

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

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebKit.cpp:82:
DEFINE_STATIC_LOCAL(std::unique_ptr<WebInitializer>, initializer,
Nit: Just say WebInitializer directly here; statics defined by this macro are
leaked on destruction anyway.

[slangley, 2017-05-22 10:42:38]

lgtm

[haraken, 2017-05-22 13:40:40]

Sorry for the review delay! This slipped off my head...

How many methods are going to be added to WebFactory? We already have
ChromeClient to bypass the inverted dependency from core/ to web/. I think we
should just use ChromeClient instead of introducing a new class that does a
similar thing.

Can we move the contents of WebInitializer to WebKit::Initialize (this is
essentially web/ layer's initializer)?

[slangley, 2017-05-23 00:00:11]

On 2017/05/22 at 13:40:40, haraken wrote:
> Sorry for the review delay! This slipped off my head...
> 
> How many methods are going to be added to WebFactory? We already have
ChromeClient to bypass the inverted dependency from core/ to web/. I think we
should just use ChromeClient instead of introducing a new class that does a
similar thing.
> 
> Can we move the contents of WebInitializer to WebKit::Initialize (this is
essentially web/ layer's initializer)?

We want to leverage this technique to construct many of the *Impl classes that
are in Source/web/ today. 

This would appear to be the cleanest way to achieve this - I don't think we want
to try and wedge this into ChromeClient for all of the use cases (and it may not
even be possible: e.g: CompositorWorkerProxyClientImpl,
AnimationWorkletProxyClientImpl to name a few).

This approach provides a nice separation of concerns.

[dcheng, 2017-05-23 00:18:47]

On 2017/05/23 00:00:11, slangley wrote:
> On 2017/05/22 at 13:40:40, haraken wrote:
> > Sorry for the review delay! This slipped off my head...
> > 
> > How many methods are going to be added to WebFactory? We already have
> ChromeClient to bypass the inverted dependency from core/ to web/. I think we
> should just use ChromeClient instead of introducing a new class that does a
> similar thing.
> > 
> > Can we move the contents of WebInitializer to WebKit::Initialize (this is
> essentially web/ layer's initializer)?
> 
> We want to leverage this technique to construct many of the *Impl classes that
> are in Source/web/ today. 
> 
> This would appear to be the cleanest way to achieve this - I don't think we
want
> to try and wedge this into ChromeClient for all of the use cases (and it may
not
> even be possible: e.g: CompositorWorkerProxyClientImpl,
> AnimationWorkletProxyClientImpl to name a few).
> 
> This approach provides a nice separation of concerns.

Another alternative would be to define ChromeClient::Create: would that work for
this? The nice part is this would be a natural progression of the code as we
collapse the impl classes into the interfaces.

[slangley, 2017-05-23 01:49:43]

On 2017/05/23 at 00:18:47, dcheng wrote:
> On 2017/05/23 00:00:11, slangley wrote:
> > On 2017/05/22 at 13:40:40, haraken wrote:
> > > Sorry for the review delay! This slipped off my head...
> > > 
> > > How many methods are going to be added to WebFactory? We already have
> > ChromeClient to bypass the inverted dependency from core/ to web/. I think
we
> > should just use ChromeClient instead of introducing a new class that does a
> > similar thing.
> > > 
> > > Can we move the contents of WebInitializer to WebKit::Initialize (this is
> > essentially web/ layer's initializer)?
> > 
> > We want to leverage this technique to construct many of the *Impl classes
that
> > are in Source/web/ today. 
> > 
> > This would appear to be the cleanest way to achieve this - I don't think we
want
> > to try and wedge this into ChromeClient for all of the use cases (and it may
not
> > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > AnimationWorkletProxyClientImpl to name a few).
> > 
> > This approach provides a nice separation of concerns.
> 
> Another alternative would be to define ChromeClient::Create: would that work
for this? The nice part is this would be a natural progression of the code as we
collapse the impl classes into the interfaces.

It's not clear (to me) the benefit of adding something like this to ChromeClient
that already has a large surface area, opposed to a dedicated class for object
construction. It potentially makes it harder for use to move classes out of Web/
as ChromeClientImpl would be stuck there.

[dcheng, 2017-05-23 01:52:36]

On 2017/05/23 01:49:43, slangley wrote:
> On 2017/05/23 at 00:18:47, dcheng wrote:
> > On 2017/05/23 00:00:11, slangley wrote:
> > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > Sorry for the review delay! This slipped off my head...
> > > > 
> > > > How many methods are going to be added to WebFactory? We already have
> > > ChromeClient to bypass the inverted dependency from core/ to web/. I think
> we
> > > should just use ChromeClient instead of introducing a new class that does
a
> > > similar thing.
> > > > 
> > > > Can we move the contents of WebInitializer to WebKit::Initialize (this
is
> > > essentially web/ layer's initializer)?
> > > 
> > > We want to leverage this technique to construct many of the *Impl classes
> that
> > > are in Source/web/ today. 
> > > 
> > > This would appear to be the cleanest way to achieve this - I don't think
we
> want
> > > to try and wedge this into ChromeClient for all of the use cases (and it
may
> not
> > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > AnimationWorkletProxyClientImpl to name a few).
> > > 
> > > This approach provides a nice separation of concerns.
> > 
> > Another alternative would be to define ChromeClient::Create: would that work
> for this? The nice part is this would be a natural progression of the code as
we
> collapse the impl classes into the interfaces.
> 
> It's not clear (to me) the benefit of adding something like this to
ChromeClient
> that already has a large surface area, opposed to a dedicated class for object
> construction. It potentially makes it harder for use to move classes out of
Web/
> as ChromeClientImpl would be stuck there.

Sorry, to be clear, my suggestion is:

ChromeClient::Create() would be defined in ChromeClient.h and be defined in
ChromeClientImpl.cpp.

Similarly, any other classes that need to be constructed would be defined in the
base interface header, but defined in the implementation cpp.

As we collapse the class hierachy down, we won't need to change the Create()
function (unless we want to get rid of it), since ChromeClientImpl folds into
ChromeClient, etc.

[haraken, 2017-05-23 01:56:24]

On 2017/05/23 01:49:43, slangley wrote:
> On 2017/05/23 at 00:18:47, dcheng wrote:
> > On 2017/05/23 00:00:11, slangley wrote:
> > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > Sorry for the review delay! This slipped off my head...
> > > > 
> > > > How many methods are going to be added to WebFactory? We already have
> > > ChromeClient to bypass the inverted dependency from core/ to web/. I think
> we
> > > should just use ChromeClient instead of introducing a new class that does
a
> > > similar thing.
> > > > 
> > > > Can we move the contents of WebInitializer to WebKit::Initialize (this
is
> > > essentially web/ layer's initializer)?
> > > 
> > > We want to leverage this technique to construct many of the *Impl classes
> that
> > > are in Source/web/ today. 
> > > 
> > > This would appear to be the cleanest way to achieve this - I don't think
we
> want
> > > to try and wedge this into ChromeClient for all of the use cases (and it
may
> not
> > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > AnimationWorkletProxyClientImpl to name a few).
> > > 
> > > This approach provides a nice separation of concerns.
> > 
> > Another alternative would be to define ChromeClient::Create: would that work
> for this? The nice part is this would be a natural progression of the code as
we
> collapse the impl classes into the interfaces.
> 
> It's not clear (to me) the benefit of adding something like this to
ChromeClient
> that already has a large surface area, opposed to a dedicated class for object
> construction. It potentially makes it harder for use to move classes out of
Web/
> as ChromeClientImpl would be stuck there.

Makes sense. LGTM % removing WebInitializer (by merging the contents into
WebKit::Initialize).

[haraken, 2017-05-23 01:56:36]

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

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:342:
chrome_client_(WebFactory::GetInstance().CreateChromeClient(this)),

Just to clarify: How will this code be in the end state? Will this be reverted
back to:

  chrome_client_impl_(ChromeClientImpl::Create(this))

?

(In the real end state we should remove ChromeClientImpl though.)

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.h:609: Persistent<blink::ChromeClient>
chrome_client_;

Drop blink::.

[haraken, 2017-05-23 01:58:15]

On 2017/05/23 01:52:36, dcheng wrote:
> On 2017/05/23 01:49:43, slangley wrote:
> > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > On 2017/05/23 00:00:11, slangley wrote:
> > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > Sorry for the review delay! This slipped off my head...
> > > > > 
> > > > > How many methods are going to be added to WebFactory? We already have
> > > > ChromeClient to bypass the inverted dependency from core/ to web/. I
think
> > we
> > > > should just use ChromeClient instead of introducing a new class that
does
> a
> > > > similar thing.
> > > > > 
> > > > > Can we move the contents of WebInitializer to WebKit::Initialize (this
> is
> > > > essentially web/ layer's initializer)?
> > > > 
> > > > We want to leverage this technique to construct many of the *Impl
classes
> > that
> > > > are in Source/web/ today. 
> > > > 
> > > > This would appear to be the cleanest way to achieve this - I don't think
> we
> > want
> > > > to try and wedge this into ChromeClient for all of the use cases (and it
> may
> > not
> > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > AnimationWorkletProxyClientImpl to name a few).
> > > > 
> > > > This approach provides a nice separation of concerns.
> > > 
> > > Another alternative would be to define ChromeClient::Create: would that
work
> > for this? The nice part is this would be a natural progression of the code
as
> we
> > collapse the impl classes into the interfaces.
> > 
> > It's not clear (to me) the benefit of adding something like this to
> ChromeClient
> > that already has a large surface area, opposed to a dedicated class for
object
> > construction. It potentially makes it harder for use to move classes out of
> Web/
> > as ChromeClientImpl would be stuck there.
> 
> Sorry, to be clear, my suggestion is:
> 
> ChromeClient::Create() would be defined in ChromeClient.h and be defined in
> ChromeClientImpl.cpp.
> 
> Similarly, any other classes that need to be constructed would be defined in
the
> base interface header, but defined in the implementation cpp.
> 
> As we collapse the class hierachy down, we won't need to change the Create()
> function (unless we want to get rid of it), since ChromeClientImpl folds into
> ChromeClient, etc.

Yes, if we could do this, that looks nicer.

That's why I asked how many methods are going to be added to WebFactory. If they
are going to be only ::Create methods, Daniel's suggestion looks nicer to me.

[slangley, 2017-05-23 01:59:57]

Ahhh  - so your suggestion is to add per class factory methods instead?

Ideally it would be better to decouple object creation from usage tho,
which that wouldn't achieve.

On Tue, May 23, 2017 at 11:52 AM, <dcheng@chromium.org> wrote:

> On 2017/05/23 01:49:43, slangley wrote:
> > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > On 2017/05/23 00:00:11, slangley wrote:
> > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > Sorry for the review delay! This slipped off my head...
> > > > >
> > > > > How many methods are going to be added to WebFactory? We already
> have
> > > > ChromeClient to bypass the inverted dependency from core/ to web/. I
> think
> > we
> > > > should just use ChromeClient instead of introducing a new class that
> does
> a
> > > > similar thing.
> > > > >
> > > > > Can we move the contents of WebInitializer to WebKit::Initialize
> (this
> is
> > > > essentially web/ layer's initializer)?
> > > >
> > > > We want to leverage this technique to construct many of the *Impl
> classes
> > that
> > > > are in Source/web/ today.
> > > >
> > > > This would appear to be the cleanest way to achieve this - I don't
> think
> we
> > want
> > > > to try and wedge this into ChromeClient for all of the use cases
> (and it
> may
> > not
> > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > AnimationWorkletProxyClientImpl to name a few).
> > > >
> > > > This approach provides a nice separation of concerns.
> > >
> > > Another alternative would be to define ChromeClient::Create: would
> that work
> > for this? The nice part is this would be a natural progression of the
> code as
> we
> > collapse the impl classes into the interfaces.
> >
> > It's not clear (to me) the benefit of adding something like this to
> ChromeClient
> > that already has a large surface area, opposed to a dedicated class for
> object
> > construction. It potentially makes it harder for use to move classes out
> of
> Web/
> > as ChromeClientImpl would be stuck there.
>
> Sorry, to be clear, my suggestion is:
>
> ChromeClient::Create() would be defined in ChromeClient.h and be defined in
> ChromeClientImpl.cpp.
>
> Similarly, any other classes that need to be constructed would be defined
> in the
> base interface header, but defined in the implementation cpp.
>
> As we collapse the class hierachy down, we won't need to change the
> Create()
> function (unless we want to get rid of it), since ChromeClientImpl folds
> into
> ChromeClient, etc.
>
> https://codereview.chromium.org/2887523003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[slangley, 2017-05-23 01:59:58]

Ahhh  - so your suggestion is to add per class factory methods instead?

Ideally it would be better to decouple object creation from usage tho,
which that wouldn't achieve.

On Tue, May 23, 2017 at 11:52 AM, <dcheng@chromium.org> wrote:

> On 2017/05/23 01:49:43, slangley wrote:
> > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > On 2017/05/23 00:00:11, slangley wrote:
> > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > Sorry for the review delay! This slipped off my head...
> > > > >
> > > > > How many methods are going to be added to WebFactory? We already
> have
> > > > ChromeClient to bypass the inverted dependency from core/ to web/. I
> think
> > we
> > > > should just use ChromeClient instead of introducing a new class that
> does
> a
> > > > similar thing.
> > > > >
> > > > > Can we move the contents of WebInitializer to WebKit::Initialize
> (this
> is
> > > > essentially web/ layer's initializer)?
> > > >
> > > > We want to leverage this technique to construct many of the *Impl
> classes
> > that
> > > > are in Source/web/ today.
> > > >
> > > > This would appear to be the cleanest way to achieve this - I don't
> think
> we
> > want
> > > > to try and wedge this into ChromeClient for all of the use cases
> (and it
> may
> > not
> > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > AnimationWorkletProxyClientImpl to name a few).
> > > >
> > > > This approach provides a nice separation of concerns.
> > >
> > > Another alternative would be to define ChromeClient::Create: would
> that work
> > for this? The nice part is this would be a natural progression of the
> code as
> we
> > collapse the impl classes into the interfaces.
> >
> > It's not clear (to me) the benefit of adding something like this to
> ChromeClient
> > that already has a large surface area, opposed to a dedicated class for
> object
> > construction. It potentially makes it harder for use to move classes out
> of
> Web/
> > as ChromeClientImpl would be stuck there.
>
> Sorry, to be clear, my suggestion is:
>
> ChromeClient::Create() would be defined in ChromeClient.h and be defined in
> ChromeClientImpl.cpp.
>
> Similarly, any other classes that need to be constructed would be defined
> in the
> base interface header, but defined in the implementation cpp.
>
> As we collapse the class hierachy down, we won't need to change the
> Create()
> function (unless we want to get rid of it), since ChromeClientImpl folds
> into
> ChromeClient, etc.
>
> https://codereview.chromium.org/2887523003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2017-05-23 02:02:23]

On 2017/05/23 01:59:58, slangley wrote:
> Ahhh  - so your suggestion is to add per class factory methods instead?
> 
> Ideally it would be better to decouple object creation from usage tho,
> which that wouldn't achieve.

Can you help me understand this comment better with an example? It seems to me
like once we no longer have the Impl we'll just refer to ChromeClass, etc
directly anyway.

> 
> On Tue, May 23, 2017 at 11:52 AM, <mailto:dcheng@chromium.org> wrote:
> 
> > On 2017/05/23 01:49:43, slangley wrote:
> > > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > > On 2017/05/23 00:00:11, slangley wrote:
> > > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > > Sorry for the review delay! This slipped off my head...
> > > > > >
> > > > > > How many methods are going to be added to WebFactory? We already
> > have
> > > > > ChromeClient to bypass the inverted dependency from core/ to web/. I
> > think
> > > we
> > > > > should just use ChromeClient instead of introducing a new class that
> > does
> > a
> > > > > similar thing.
> > > > > >
> > > > > > Can we move the contents of WebInitializer to WebKit::Initialize
> > (this
> > is
> > > > > essentially web/ layer's initializer)?
> > > > >
> > > > > We want to leverage this technique to construct many of the *Impl
> > classes
> > > that
> > > > > are in Source/web/ today.
> > > > >
> > > > > This would appear to be the cleanest way to achieve this - I don't
> > think
> > we
> > > want
> > > > > to try and wedge this into ChromeClient for all of the use cases
> > (and it
> > may
> > > not
> > > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > > AnimationWorkletProxyClientImpl to name a few).
> > > > >
> > > > > This approach provides a nice separation of concerns.
> > > >
> > > > Another alternative would be to define ChromeClient::Create: would
> > that work
> > > for this? The nice part is this would be a natural progression of the
> > code as
> > we
> > > collapse the impl classes into the interfaces.
> > >
> > > It's not clear (to me) the benefit of adding something like this to
> > ChromeClient
> > > that already has a large surface area, opposed to a dedicated class for
> > object
> > > construction. It potentially makes it harder for use to move classes out
> > of
> > Web/
> > > as ChromeClientImpl would be stuck there.
> >
> > Sorry, to be clear, my suggestion is:
> >
> > ChromeClient::Create() would be defined in ChromeClient.h and be defined in
> > ChromeClientImpl.cpp.
> >
> > Similarly, any other classes that need to be constructed would be defined
> > in the
> > base interface header, but defined in the implementation cpp.
> >
> > As we collapse the class hierachy down, we won't need to change the
> > Create()
> > function (unless we want to get rid of it), since ChromeClientImpl folds
> > into
> > ChromeClient, etc.
> >
> > https://codereview.chromium.org/2887523003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[slangley, 2017-05-23 02:15:46]

In this case, mocking out ChromeClient would be the most obvious example.

On Tue, May 23, 2017 at 12:02 PM, <dcheng@chromium.org> wrote:

> On 2017/05/23 01:59:58, slangley wrote:
> > Ahhh - so your suggestion is to add per class factory methods instead?
> >
> > Ideally it would be better to decouple object creation from usage tho,
> > which that wouldn't achieve.
>
> Can you help me understand this comment better with an example? It seems
> to me
> like once we no longer have the Impl we'll just refer to ChromeClass, etc
> directly anyway.
>
> >
> > On Tue, May 23, 2017 at 11:52 AM, <mailto:dcheng@chromium.org> wrote:
> >
> > > On 2017/05/23 01:49:43, slangley wrote:
> > > > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > > > On 2017/05/23 00:00:11, slangley wrote:
> > > > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > > > Sorry for the review delay! This slipped off my head...
> > > > > > >
> > > > > > > How many methods are going to be added to WebFactory? We
> already
> > > have
> > > > > > ChromeClient to bypass the inverted dependency from core/ to
> web/. I
> > > think
> > > > we
> > > > > > should just use ChromeClient instead of introducing a new class
> that
> > > does
> > > a
> > > > > > similar thing.
> > > > > > >
> > > > > > > Can we move the contents of WebInitializer to
> WebKit::Initialize
> > > (this
> > > is
> > > > > > essentially web/ layer's initializer)?
> > > > > >
> > > > > > We want to leverage this technique to construct many of the *Impl
> > > classes
> > > > that
> > > > > > are in Source/web/ today.
> > > > > >
> > > > > > This would appear to be the cleanest way to achieve this - I
> don't
> > > think
> > > we
> > > > want
> > > > > > to try and wedge this into ChromeClient for all of the use cases
> > > (and it
> > > may
> > > > not
> > > > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > > > AnimationWorkletProxyClientImpl to name a few).
> > > > > >
> > > > > > This approach provides a nice separation of concerns.
> > > > >
> > > > > Another alternative would be to define ChromeClient::Create: would
> > > that work
> > > > for this? The nice part is this would be a natural progression of the
> > > code as
> > > we
> > > > collapse the impl classes into the interfaces.
> > > >
> > > > It's not clear (to me) the benefit of adding something like this to
> > > ChromeClient
> > > > that already has a large surface area, opposed to a dedicated class
> for
> > > object
> > > > construction. It potentially makes it harder for use to move classes
> out
> > > of
> > > Web/
> > > > as ChromeClientImpl would be stuck there.
> > >
> > > Sorry, to be clear, my suggestion is:
> > >
> > > ChromeClient::Create() would be defined in ChromeClient.h and be
> defined in
> > > ChromeClientImpl.cpp.
> > >
> > > Similarly, any other classes that need to be constructed would be
> defined
> > > in the
> > > base interface header, but defined in the implementation cpp.
> > >
> > > As we collapse the class hierachy down, we won't need to change the
> > > Create()
> > > function (unless we want to get rid of it), since ChromeClientImpl
> folds
> > > into
> > > ChromeClient, etc.
> > >
> > > https://codereview.chromium.org/2887523003/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
> https://codereview.chromium.org/2887523003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[slangley, 2017-05-23 02:15:47]

In this case, mocking out ChromeClient would be the most obvious example.

On Tue, May 23, 2017 at 12:02 PM, <dcheng@chromium.org> wrote:

> On 2017/05/23 01:59:58, slangley wrote:
> > Ahhh - so your suggestion is to add per class factory methods instead?
> >
> > Ideally it would be better to decouple object creation from usage tho,
> > which that wouldn't achieve.
>
> Can you help me understand this comment better with an example? It seems
> to me
> like once we no longer have the Impl we'll just refer to ChromeClass, etc
> directly anyway.
>
> >
> > On Tue, May 23, 2017 at 11:52 AM, <mailto:dcheng@chromium.org> wrote:
> >
> > > On 2017/05/23 01:49:43, slangley wrote:
> > > > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > > > On 2017/05/23 00:00:11, slangley wrote:
> > > > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > > > Sorry for the review delay! This slipped off my head...
> > > > > > >
> > > > > > > How many methods are going to be added to WebFactory? We
> already
> > > have
> > > > > > ChromeClient to bypass the inverted dependency from core/ to
> web/. I
> > > think
> > > > we
> > > > > > should just use ChromeClient instead of introducing a new class
> that
> > > does
> > > a
> > > > > > similar thing.
> > > > > > >
> > > > > > > Can we move the contents of WebInitializer to
> WebKit::Initialize
> > > (this
> > > is
> > > > > > essentially web/ layer's initializer)?
> > > > > >
> > > > > > We want to leverage this technique to construct many of the *Impl
> > > classes
> > > > that
> > > > > > are in Source/web/ today.
> > > > > >
> > > > > > This would appear to be the cleanest way to achieve this - I
> don't
> > > think
> > > we
> > > > want
> > > > > > to try and wedge this into ChromeClient for all of the use cases
> > > (and it
> > > may
> > > > not
> > > > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > > > AnimationWorkletProxyClientImpl to name a few).
> > > > > >
> > > > > > This approach provides a nice separation of concerns.
> > > > >
> > > > > Another alternative would be to define ChromeClient::Create: would
> > > that work
> > > > for this? The nice part is this would be a natural progression of the
> > > code as
> > > we
> > > > collapse the impl classes into the interfaces.
> > > >
> > > > It's not clear (to me) the benefit of adding something like this to
> > > ChromeClient
> > > > that already has a large surface area, opposed to a dedicated class
> for
> > > object
> > > > construction. It potentially makes it harder for use to move classes
> out
> > > of
> > > Web/
> > > > as ChromeClientImpl would be stuck there.
> > >
> > > Sorry, to be clear, my suggestion is:
> > >
> > > ChromeClient::Create() would be defined in ChromeClient.h and be
> defined in
> > > ChromeClientImpl.cpp.
> > >
> > > Similarly, any other classes that need to be constructed would be
> defined
> > > in the
> > > base interface header, but defined in the implementation cpp.
> > >
> > > As we collapse the class hierachy down, we won't need to change the
> > > Create()
> > > function (unless we want to get rid of it), since ChromeClientImpl
> folds
> > > into
> > > ChromeClient, etc.
> > >
> > > https://codereview.chromium.org/2887523003/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
> https://codereview.chromium.org/2887523003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2017-05-23 03:01:53]

Ah, that's a good point. However, I think things like ChromeClient are probably
things we don't want to be mocking out in the future anyway: it was historically
just a way to call into the web directory. I'm guessing (I may be overly
optimistic here) that we will eventually only need to mock out the public API
layer for testing: we already do this today for tests in Source/web/tests, and
once web and core are merged, there's no reason core tests can't do that as
well.

On 2017/05/23 02:15:47, slangley wrote:
> In this case, mocking out ChromeClient would be the most obvious example.
> 
> On Tue, May 23, 2017 at 12:02 PM, <mailto:dcheng@chromium.org> wrote:
> 
> > On 2017/05/23 01:59:58, slangley wrote:
> > > Ahhh - so your suggestion is to add per class factory methods instead?
> > >
> > > Ideally it would be better to decouple object creation from usage tho,
> > > which that wouldn't achieve.
> >
> > Can you help me understand this comment better with an example? It seems
> > to me
> > like once we no longer have the Impl we'll just refer to ChromeClass, etc
> > directly anyway.
> >
> > >
> > > On Tue, May 23, 2017 at 11:52 AM, <mailto:dcheng@chromium.org> wrote:
> > >
> > > > On 2017/05/23 01:49:43, slangley wrote:
> > > > > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > > > > On 2017/05/23 00:00:11, slangley wrote:
> > > > > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > > > > Sorry for the review delay! This slipped off my head...
> > > > > > > >
> > > > > > > > How many methods are going to be added to WebFactory? We
> > already
> > > > have
> > > > > > > ChromeClient to bypass the inverted dependency from core/ to
> > web/. I
> > > > think
> > > > > we
> > > > > > > should just use ChromeClient instead of introducing a new class
> > that
> > > > does
> > > > a
> > > > > > > similar thing.
> > > > > > > >
> > > > > > > > Can we move the contents of WebInitializer to
> > WebKit::Initialize
> > > > (this
> > > > is
> > > > > > > essentially web/ layer's initializer)?
> > > > > > >
> > > > > > > We want to leverage this technique to construct many of the *Impl
> > > > classes
> > > > > that
> > > > > > > are in Source/web/ today.
> > > > > > >
> > > > > > > This would appear to be the cleanest way to achieve this - I
> > don't
> > > > think
> > > > we
> > > > > want
> > > > > > > to try and wedge this into ChromeClient for all of the use cases
> > > > (and it
> > > > may
> > > > > not
> > > > > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > > > > AnimationWorkletProxyClientImpl to name a few).
> > > > > > >
> > > > > > > This approach provides a nice separation of concerns.
> > > > > >
> > > > > > Another alternative would be to define ChromeClient::Create: would
> > > > that work
> > > > > for this? The nice part is this would be a natural progression of the
> > > > code as
> > > > we
> > > > > collapse the impl classes into the interfaces.
> > > > >
> > > > > It's not clear (to me) the benefit of adding something like this to
> > > > ChromeClient
> > > > > that already has a large surface area, opposed to a dedicated class
> > for
> > > > object
> > > > > construction. It potentially makes it harder for use to move classes
> > out
> > > > of
> > > > Web/
> > > > > as ChromeClientImpl would be stuck there.
> > > >
> > > > Sorry, to be clear, my suggestion is:
> > > >
> > > > ChromeClient::Create() would be defined in ChromeClient.h and be
> > defined in
> > > > ChromeClientImpl.cpp.
> > > >
> > > > Similarly, any other classes that need to be constructed would be
> > defined
> > > > in the
> > > > base interface header, but defined in the implementation cpp.
> > > >
> > > > As we collapse the class hierachy down, we won't need to change the
> > > > Create()
> > > > function (unless we want to get rid of it), since ChromeClientImpl
> > folds
> > > > into
> > > > ChromeClient, etc.
> > > >
> > > > https://codereview.chromium.org/2887523003/
> > > >
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> > https://codereview.chromium.org/2887523003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[slangley, 2017-05-23 03:26:34]

Well we kinda mock it today with EmptyChromeClient in places, although it
not solely used for tests. We cheat a bit by making the members public
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page...>
so
you can poke them.

The other use case for having an abstract factory rather than static Create
methods is where the Impl class directly implements a class defined in
public/web - where we cannot change the public API.

examples:
- WebFileChooserCompletionImpl
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/ChromeClie...>
- WebTextCheckingCompletionImpl
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/TextChecke...>

The abstract factory would also be useful for places where ::Create is
being called on the impl (e.g. here
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebSharedW...>).
In short it's the easiest way to tease apart all of these different
dependency patterns. It's also the easiest for us to revert - just delete
the factory and fix all of the call sites.



On Tue, May 23, 2017 at 1:01 PM, <dcheng@chromium.org> wrote:

> Ah, that's a good point. However, I think things like ChromeClient are
> probably
> things we don't want to be mocking out in the future anyway: it was
> historically
> just a way to call into the web directory. I'm guessing (I may be overly
> optimistic here) that we will eventually only need to mock out the public
> API
> layer for testing: we already do this today for tests in Source/web/tests,
> and
> once web and core are merged, there's no reason core tests can't do that as
> well.
>
> On 2017/05/23 02:15:47, slangley wrote:
> > In this case, mocking out ChromeClient would be the most obvious example.
> >
> > On Tue, May 23, 2017 at 12:02 PM, <mailto:dcheng@chromium.org> wrote:
> >
> > > On 2017/05/23 01:59:58, slangley wrote:
> > > > Ahhh - so your suggestion is to add per class factory methods
> instead?
> > > >
> > > > Ideally it would be better to decouple object creation from usage
> tho,
> > > > which that wouldn't achieve.
> > >
> > > Can you help me understand this comment better with an example? It
> seems
> > > to me
> > > like once we no longer have the Impl we'll just refer to ChromeClass,
> etc
> > > directly anyway.
> > >
> > > >
> > > > On Tue, May 23, 2017 at 11:52 AM, <mailto:dcheng@chromium.org>
> wrote:
> > > >
> > > > > On 2017/05/23 01:49:43, slangley wrote:
> > > > > > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > > > > > On 2017/05/23 00:00:11, slangley wrote:
> > > > > > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > > > > > Sorry for the review delay! This slipped off my head...
> > > > > > > > >
> > > > > > > > > How many methods are going to be added to WebFactory? We
> > > already
> > > > > have
> > > > > > > > ChromeClient to bypass the inverted dependency from core/ to
> > > web/. I
> > > > > think
> > > > > > we
> > > > > > > > should just use ChromeClient instead of introducing a new
> class
> > > that
> > > > > does
> > > > > a
> > > > > > > > similar thing.
> > > > > > > > >
> > > > > > > > > Can we move the contents of WebInitializer to
> > > WebKit::Initialize
> > > > > (this
> > > > > is
> > > > > > > > essentially web/ layer's initializer)?
> > > > > > > >
> > > > > > > > We want to leverage this technique to construct many of the
> *Impl
> > > > > classes
> > > > > > that
> > > > > > > > are in Source/web/ today.
> > > > > > > >
> > > > > > > > This would appear to be the cleanest way to achieve this - I
> > > don't
> > > > > think
> > > > > we
> > > > > > want
> > > > > > > > to try and wedge this into ChromeClient for all of the use
> cases
> > > > > (and it
> > > > > may
> > > > > > not
> > > > > > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > > > > > AnimationWorkletProxyClientImpl to name a few).
> > > > > > > >
> > > > > > > > This approach provides a nice separation of concerns.
> > > > > > >
> > > > > > > Another alternative would be to define ChromeClient::Create:
> would
> > > > > that work
> > > > > > for this? The nice part is this would be a natural progression
> of the
> > > > > code as
> > > > > we
> > > > > > collapse the impl classes into the interfaces.
> > > > > >
> > > > > > It's not clear (to me) the benefit of adding something like this
> to
> > > > > ChromeClient
> > > > > > that already has a large surface area, opposed to a dedicated
> class
> > > for
> > > > > object
> > > > > > construction. It potentially makes it harder for use to move
> classes
> > > out
> > > > > of
> > > > > Web/
> > > > > > as ChromeClientImpl would be stuck there.
> > > > >
> > > > > Sorry, to be clear, my suggestion is:
> > > > >
> > > > > ChromeClient::Create() would be defined in ChromeClient.h and be
> > > defined in
> > > > > ChromeClientImpl.cpp.
> > > > >
> > > > > Similarly, any other classes that need to be constructed would be
> > > defined
> > > > > in the
> > > > > base interface header, but defined in the implementation cpp.
> > > > >
> > > > > As we collapse the class hierachy down, we won't need to change the
> > > > > Create()
> > > > > function (unless we want to get rid of it), since ChromeClientImpl
> > > folds
> > > > > into
> > > > > ChromeClient, etc.
> > > > >
> > > > > https://codereview.chromium.org/2887523003/
> > > > >
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google
> Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it,
> send an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > > https://codereview.chromium.org/2887523003/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
> https://codereview.chromium.org/2887523003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[slangley, 2017-05-23 03:26:35]

Well we kinda mock it today with EmptyChromeClient in places, although it
not solely used for tests. We cheat a bit by making the members public
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page...>
so
you can poke them.

The other use case for having an abstract factory rather than static Create
methods is where the Impl class directly implements a class defined in
public/web - where we cannot change the public API.

examples:
- WebFileChooserCompletionImpl
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/ChromeClie...>
- WebTextCheckingCompletionImpl
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/TextChecke...>

The abstract factory would also be useful for places where ::Create is
being called on the impl (e.g. here
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebSharedW...>).
In short it's the easiest way to tease apart all of these different
dependency patterns. It's also the easiest for us to revert - just delete
the factory and fix all of the call sites.



On Tue, May 23, 2017 at 1:01 PM, <dcheng@chromium.org> wrote:

> Ah, that's a good point. However, I think things like ChromeClient are
> probably
> things we don't want to be mocking out in the future anyway: it was
> historically
> just a way to call into the web directory. I'm guessing (I may be overly
> optimistic here) that we will eventually only need to mock out the public
> API
> layer for testing: we already do this today for tests in Source/web/tests,
> and
> once web and core are merged, there's no reason core tests can't do that as
> well.
>
> On 2017/05/23 02:15:47, slangley wrote:
> > In this case, mocking out ChromeClient would be the most obvious example.
> >
> > On Tue, May 23, 2017 at 12:02 PM, <mailto:dcheng@chromium.org> wrote:
> >
> > > On 2017/05/23 01:59:58, slangley wrote:
> > > > Ahhh - so your suggestion is to add per class factory methods
> instead?
> > > >
> > > > Ideally it would be better to decouple object creation from usage
> tho,
> > > > which that wouldn't achieve.
> > >
> > > Can you help me understand this comment better with an example? It
> seems
> > > to me
> > > like once we no longer have the Impl we'll just refer to ChromeClass,
> etc
> > > directly anyway.
> > >
> > > >
> > > > On Tue, May 23, 2017 at 11:52 AM, <mailto:dcheng@chromium.org>
> wrote:
> > > >
> > > > > On 2017/05/23 01:49:43, slangley wrote:
> > > > > > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > > > > > On 2017/05/23 00:00:11, slangley wrote:
> > > > > > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > > > > > Sorry for the review delay! This slipped off my head...
> > > > > > > > >
> > > > > > > > > How many methods are going to be added to WebFactory? We
> > > already
> > > > > have
> > > > > > > > ChromeClient to bypass the inverted dependency from core/ to
> > > web/. I
> > > > > think
> > > > > > we
> > > > > > > > should just use ChromeClient instead of introducing a new
> class
> > > that
> > > > > does
> > > > > a
> > > > > > > > similar thing.
> > > > > > > > >
> > > > > > > > > Can we move the contents of WebInitializer to
> > > WebKit::Initialize
> > > > > (this
> > > > > is
> > > > > > > > essentially web/ layer's initializer)?
> > > > > > > >
> > > > > > > > We want to leverage this technique to construct many of the
> *Impl
> > > > > classes
> > > > > > that
> > > > > > > > are in Source/web/ today.
> > > > > > > >
> > > > > > > > This would appear to be the cleanest way to achieve this - I
> > > don't
> > > > > think
> > > > > we
> > > > > > want
> > > > > > > > to try and wedge this into ChromeClient for all of the use
> cases
> > > > > (and it
> > > > > may
> > > > > > not
> > > > > > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > > > > > AnimationWorkletProxyClientImpl to name a few).
> > > > > > > >
> > > > > > > > This approach provides a nice separation of concerns.
> > > > > > >
> > > > > > > Another alternative would be to define ChromeClient::Create:
> would
> > > > > that work
> > > > > > for this? The nice part is this would be a natural progression
> of the
> > > > > code as
> > > > > we
> > > > > > collapse the impl classes into the interfaces.
> > > > > >
> > > > > > It's not clear (to me) the benefit of adding something like this
> to
> > > > > ChromeClient
> > > > > > that already has a large surface area, opposed to a dedicated
> class
> > > for
> > > > > object
> > > > > > construction. It potentially makes it harder for use to move
> classes
> > > out
> > > > > of
> > > > > Web/
> > > > > > as ChromeClientImpl would be stuck there.
> > > > >
> > > > > Sorry, to be clear, my suggestion is:
> > > > >
> > > > > ChromeClient::Create() would be defined in ChromeClient.h and be
> > > defined in
> > > > > ChromeClientImpl.cpp.
> > > > >
> > > > > Similarly, any other classes that need to be constructed would be
> > > defined
> > > > > in the
> > > > > base interface header, but defined in the implementation cpp.
> > > > >
> > > > > As we collapse the class hierachy down, we won't need to change the
> > > > > Create()
> > > > > function (unless we want to get rid of it), since ChromeClientImpl
> > > folds
> > > > > into
> > > > > ChromeClient, etc.
> > > > >
> > > > > https://codereview.chromium.org/2887523003/
> > > > >
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google
> Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it,
> send an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > > https://codereview.chromium.org/2887523003/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
> https://codereview.chromium.org/2887523003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2017-05-23 03:45:12]

On 2017/05/23 03:26:35, slangley wrote:
> Well we kinda mock it today with EmptyChromeClient in places, although it
> not solely used for tests. We cheat a bit by making the members public
>
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page...>
> so
> you can poke them.

I think as we fold web into core, ChromeClient will not be the right place to
have mocks in these tests. Mocking at this level risks having behavior that
diverges from what the actual impl classes in web do.

> 
> The other use case for having an abstract factory rather than static Create
> methods is where the Impl class directly implements a class defined in
> public/web - where we cannot change the public API.
> 
> examples:
> - WebFileChooserCompletionImpl
>
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/ChromeClie...>
> - WebTextCheckingCompletionImpl
>
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/TextChecke...>
> 
> The abstract factory would also be useful for places where ::Create is
> being called on the impl (e.g. here
>
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebSharedW...>).
> In short it's the easiest way to tease apart all of these different
> dependency patterns. It's also the easiest for us to revert - just delete
> the factory and fix all of the call sites.

I think it might be easier to justify an abstract factory as a separate task
improving Blink test coverage (i.e. show how this concretely improves
testability of various code in Blink).

Specifically for something like WebFileChooserCompletionImpl, I'm not sure we
want to substitute another implementation. It seems like abstraction point can
be WebFrameClient::RunFileChooser, which is a virtual. A similar comment applies
to WebTextCheckingCompletionImpl: the specifics are an implementation detail
which is just to plumb things between core and web, so the real thing to test is
the public embedder API (by calling WebLocalFrame::SetTextCheckClient).

> 
> 
> 
> On Tue, May 23, 2017 at 1:01 PM, <mailto:dcheng@chromium.org> wrote:
> 
> > Ah, that's a good point. However, I think things like ChromeClient are
> > probably
> > things we don't want to be mocking out in the future anyway: it was
> > historically
> > just a way to call into the web directory. I'm guessing (I may be overly
> > optimistic here) that we will eventually only need to mock out the public
> > API
> > layer for testing: we already do this today for tests in Source/web/tests,
> > and
> > once web and core are merged, there's no reason core tests can't do that as
> > well.
> >
> > On 2017/05/23 02:15:47, slangley wrote:
> > > In this case, mocking out ChromeClient would be the most obvious example.
> > >
> > > On Tue, May 23, 2017 at 12:02 PM, <mailto:dcheng@chromium.org> wrote:
> > >
> > > > On 2017/05/23 01:59:58, slangley wrote:
> > > > > Ahhh - so your suggestion is to add per class factory methods
> > instead?
> > > > >
> > > > > Ideally it would be better to decouple object creation from usage
> > tho,
> > > > > which that wouldn't achieve.
> > > >
> > > > Can you help me understand this comment better with an example? It
> > seems
> > > > to me
> > > > like once we no longer have the Impl we'll just refer to ChromeClass,
> > etc
> > > > directly anyway.
> > > >
> > > > >
> > > > > On Tue, May 23, 2017 at 11:52 AM, <mailto:dcheng@chromium.org>
> > wrote:
> > > > >
> > > > > > On 2017/05/23 01:49:43, slangley wrote:
> > > > > > > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > > > > > > On 2017/05/23 00:00:11, slangley wrote:
> > > > > > > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > > > > > > Sorry for the review delay! This slipped off my head...
> > > > > > > > > >
> > > > > > > > > > How many methods are going to be added to WebFactory? We
> > > > already
> > > > > > have
> > > > > > > > > ChromeClient to bypass the inverted dependency from core/ to
> > > > web/. I
> > > > > > think
> > > > > > > we
> > > > > > > > > should just use ChromeClient instead of introducing a new
> > class
> > > > that
> > > > > > does
> > > > > > a
> > > > > > > > > similar thing.
> > > > > > > > > >
> > > > > > > > > > Can we move the contents of WebInitializer to
> > > > WebKit::Initialize
> > > > > > (this
> > > > > > is
> > > > > > > > > essentially web/ layer's initializer)?
> > > > > > > > >
> > > > > > > > > We want to leverage this technique to construct many of the
> > *Impl
> > > > > > classes
> > > > > > > that
> > > > > > > > > are in Source/web/ today.
> > > > > > > > >
> > > > > > > > > This would appear to be the cleanest way to achieve this - I
> > > > don't
> > > > > > think
> > > > > > we
> > > > > > > want
> > > > > > > > > to try and wedge this into ChromeClient for all of the use
> > cases
> > > > > > (and it
> > > > > > may
> > > > > > > not
> > > > > > > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > > > > > > AnimationWorkletProxyClientImpl to name a few).
> > > > > > > > >
> > > > > > > > > This approach provides a nice separation of concerns.
> > > > > > > >
> > > > > > > > Another alternative would be to define ChromeClient::Create:
> > would
> > > > > > that work
> > > > > > > for this? The nice part is this would be a natural progression
> > of the
> > > > > > code as
> > > > > > we
> > > > > > > collapse the impl classes into the interfaces.
> > > > > > >
> > > > > > > It's not clear (to me) the benefit of adding something like this
> > to
> > > > > > ChromeClient
> > > > > > > that already has a large surface area, opposed to a dedicated
> > class
> > > > for
> > > > > > object
> > > > > > > construction. It potentially makes it harder for use to move
> > classes
> > > > out
> > > > > > of
> > > > > > Web/
> > > > > > > as ChromeClientImpl would be stuck there.
> > > > > >
> > > > > > Sorry, to be clear, my suggestion is:
> > > > > >
> > > > > > ChromeClient::Create() would be defined in ChromeClient.h and be
> > > > defined in
> > > > > > ChromeClientImpl.cpp.
> > > > > >
> > > > > > Similarly, any other classes that need to be constructed would be
> > > > defined
> > > > > > in the
> > > > > > base interface header, but defined in the implementation cpp.
> > > > > >
> > > > > > As we collapse the class hierachy down, we won't need to change the
> > > > > > Create()
> > > > > > function (unless we want to get rid of it), since ChromeClientImpl
> > > > folds
> > > > > > into
> > > > > > ChromeClient, etc.
> > > > > >
> > > > > > https://codereview.chromium.org/2887523003/
> > > > > >
> > > > >
> > > > > --
> > > > > You received this message because you are subscribed to the Google
> > Groups
> > > > > "Chromium-reviews" group.
> > > > > To unsubscribe from this group and stop receiving emails from it,
> > send an
> > > > email
> > > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > > >
> > > > https://codereview.chromium.org/2887523003/
> > > >
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> > https://codereview.chromium.org/2887523003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[slangley, 2017-05-23 03:56:53]

Yes to be clear the reason for having this right now is to tease apart all
of these classes so we can move them from web to new homes in modules or
core - while they are many entangled references it makes it a hard job to
move things in logical units. The factory lets us localize these
dependencies to one location and will cater for calls to new or static
Create methods. The other suggestions are not going to cover 100% of our
use cases (adding static Create to the abstract class for example).

Any discussions of mocking etc are really just hypothetical further
potential use cases, but are not in any way a motivation for this CL. Does
that make sense?

On Tue, May 23, 2017 at 1:45 PM, <dcheng@chromium.org> wrote:

> On 2017/05/23 03:26:35, slangley wrote:
> > Well we kinda mock it today with EmptyChromeClient in places, although it
> > not solely used for tests. We cheat a bit by making the members public
> >
> <https://cs.chromium.org/chromium/src/third_party/
> WebKit/Source/core/page/Page.h?rcl=df82c2f1f609cfe5cead26794eeb85
> 320594c904&l=100&gt;
> > so
> > you can poke them.
>
> I think as we fold web into core, ChromeClient will not be the right place
> to
> have mocks in these tests. Mocking at this level risks having behavior that
> diverges from what the actual impl classes in web do.
>
> >
> > The other use case for having an abstract factory rather than static
> Create
> > methods is where the Impl class directly implements a class defined in
> > public/web - where we cannot change the public API.
> >
> > examples:
> > - WebFileChooserCompletionImpl
> >
> <https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/
> ChromeClientImpl.cpp?rcl=489ea56faf3d58a0c940cec510a1dc514747d64c&l=758&gt
> ;
> > - WebTextCheckingCompletionImpl
> >
> <https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/
> TextCheckerClientImpl.cpp?rcl=489ea56faf3d58a0c940cec510a1dc
> 514747d64c&l=56&gt;
> >
> > The abstract factory would also be useful for places where ::Create is
> > being called on the impl (e.g. here
> >
> <https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/
> WebSharedWorkerImpl.cpp?rcl=e65dd51e10accc80aa2be79129912f47357cb958&l=341
> >).
> > In short it's the easiest way to tease apart all of these different
> > dependency patterns. It's also the easiest for us to revert - just delete
> > the factory and fix all of the call sites.
>
> I think it might be easier to justify an abstract factory as a separate
> task
> improving Blink test coverage (i.e. show how this concretely improves
> testability of various code in Blink).
>
> Specifically for something like WebFileChooserCompletionImpl, I'm not sure
> we
> want to substitute another implementation. It seems like abstraction point
> can
> be WebFrameClient::RunFileChooser, which is a virtual. A similar comment
> applies
> to WebTextCheckingCompletionImpl: the specifics are an implementation
> detail
> which is just to plumb things between core and web, so the real thing to
> test is
> the public embedder API (by calling WebLocalFrame::SetTextCheckClient).
>
>
> >
> >
> >
> > On Tue, May 23, 2017 at 1:01 PM, <mailto:dcheng@chromium.org> wrote:
> >
> > > Ah, that's a good point. However, I think things like ChromeClient are
> > > probably
> > > things we don't want to be mocking out in the future anyway: it was
> > > historically
> > > just a way to call into the web directory. I'm guessing (I may be
> overly
> > > optimistic here) that we will eventually only need to mock out the
> public
> > > API
> > > layer for testing: we already do this today for tests in
> Source/web/tests,
> > > and
> > > once web and core are merged, there's no reason core tests can't do
> that as
> > > well.
> > >
> > > On 2017/05/23 02:15:47, slangley wrote:
> > > > In this case, mocking out ChromeClient would be the most obvious
> example.
> > > >
> > > > On Tue, May 23, 2017 at 12:02 PM, <mailto:dcheng@chromium.org>
> wrote:
> > > >
> > > > > On 2017/05/23 01:59:58, slangley wrote:
> > > > > > Ahhh - so your suggestion is to add per class factory methods
> > > instead?
> > > > > >
> > > > > > Ideally it would be better to decouple object creation from usage
> > > tho,
> > > > > > which that wouldn't achieve.
> > > > >
> > > > > Can you help me understand this comment better with an example? It
> > > seems
> > > > > to me
> > > > > like once we no longer have the Impl we'll just refer to
> ChromeClass,
> > > etc
> > > > > directly anyway.
> > > > >
> > > > > >
> > > > > > On Tue, May 23, 2017 at 11:52 AM, <mailto:dcheng@chromium.org>
> > > wrote:
> > > > > >
> > > > > > > On 2017/05/23 01:49:43, slangley wrote:
> > > > > > > > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > > > > > > > On 2017/05/23 00:00:11, slangley wrote:
> > > > > > > > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > > > > > > > Sorry for the review delay! This slipped off my head...
> > > > > > > > > > >
> > > > > > > > > > > How many methods are going to be added to WebFactory?
> We
> > > > > already
> > > > > > > have
> > > > > > > > > > ChromeClient to bypass the inverted dependency from
> core/ to
> > > > > web/. I
> > > > > > > think
> > > > > > > > we
> > > > > > > > > > should just use ChromeClient instead of introducing a new
> > > class
> > > > > that
> > > > > > > does
> > > > > > > a
> > > > > > > > > > similar thing.
> > > > > > > > > > >
> > > > > > > > > > > Can we move the contents of WebInitializer to
> > > > > WebKit::Initialize
> > > > > > > (this
> > > > > > > is
> > > > > > > > > > essentially web/ layer's initializer)?
> > > > > > > > > >
> > > > > > > > > > We want to leverage this technique to construct many of
> the
> > > *Impl
> > > > > > > classes
> > > > > > > > that
> > > > > > > > > > are in Source/web/ today.
> > > > > > > > > >
> > > > > > > > > > This would appear to be the cleanest way to achieve this
> - I
> > > > > don't
> > > > > > > think
> > > > > > > we
> > > > > > > > want
> > > > > > > > > > to try and wedge this into ChromeClient for all of the
> use
> > > cases
> > > > > > > (and it
> > > > > > > may
> > > > > > > > not
> > > > > > > > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > > > > > > > AnimationWorkletProxyClientImpl to name a few).
> > > > > > > > > >
> > > > > > > > > > This approach provides a nice separation of concerns.
> > > > > > > > >
> > > > > > > > > Another alternative would be to define
> ChromeClient::Create:
> > > would
> > > > > > > that work
> > > > > > > > for this? The nice part is this would be a natural
> progression
> > > of the
> > > > > > > code as
> > > > > > > we
> > > > > > > > collapse the impl classes into the interfaces.
> > > > > > > >
> > > > > > > > It's not clear (to me) the benefit of adding something like
> this
> > > to
> > > > > > > ChromeClient
> > > > > > > > that already has a large surface area, opposed to a dedicated
> > > class
> > > > > for
> > > > > > > object
> > > > > > > > construction. It potentially makes it harder for use to move
> > > classes
> > > > > out
> > > > > > > of
> > > > > > > Web/
> > > > > > > > as ChromeClientImpl would be stuck there.
> > > > > > >
> > > > > > > Sorry, to be clear, my suggestion is:
> > > > > > >
> > > > > > > ChromeClient::Create() would be defined in ChromeClient.h and
> be
> > > > > defined in
> > > > > > > ChromeClientImpl.cpp.
> > > > > > >
> > > > > > > Similarly, any other classes that need to be constructed would
> be
> > > > > defined
> > > > > > > in the
> > > > > > > base interface header, but defined in the implementation cpp.
> > > > > > >
> > > > > > > As we collapse the class hierachy down, we won't need to
> change the
> > > > > > > Create()
> > > > > > > function (unless we want to get rid of it), since
> ChromeClientImpl
> > > > > folds
> > > > > > > into
> > > > > > > ChromeClient, etc.
> > > > > > >
> > > > > > > https://codereview.chromium.org/2887523003/
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > You received this message because you are subscribed to the
> Google
> > > Groups
> > > > > > "Chromium-reviews" group.
> > > > > > To unsubscribe from this group and stop receiving emails from it,
> > > send an
> > > > > email
> > > > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > >
> > > > > https://codereview.chromium.org/2887523003/
> > > > >
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google
> Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it,
> send an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > > https://codereview.chromium.org/2887523003/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2887523003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[slangley, 2017-05-23 03:56:54]

Yes to be clear the reason for having this right now is to tease apart all
of these classes so we can move them from web to new homes in modules or
core - while they are many entangled references it makes it a hard job to
move things in logical units. The factory lets us localize these
dependencies to one location and will cater for calls to new or static
Create methods. The other suggestions are not going to cover 100% of our
use cases (adding static Create to the abstract class for example).

Any discussions of mocking etc are really just hypothetical further
potential use cases, but are not in any way a motivation for this CL. Does
that make sense?

On Tue, May 23, 2017 at 1:45 PM, <dcheng@chromium.org> wrote:

> On 2017/05/23 03:26:35, slangley wrote:
> > Well we kinda mock it today with EmptyChromeClient in places, although it
> > not solely used for tests. We cheat a bit by making the members public
> >
> <https://cs.chromium.org/chromium/src/third_party/
> WebKit/Source/core/page/Page.h?rcl=df82c2f1f609cfe5cead26794eeb85
> 320594c904&l=100&gt;
> > so
> > you can poke them.
>
> I think as we fold web into core, ChromeClient will not be the right place
> to
> have mocks in these tests. Mocking at this level risks having behavior that
> diverges from what the actual impl classes in web do.
>
> >
> > The other use case for having an abstract factory rather than static
> Create
> > methods is where the Impl class directly implements a class defined in
> > public/web - where we cannot change the public API.
> >
> > examples:
> > - WebFileChooserCompletionImpl
> >
> <https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/
> ChromeClientImpl.cpp?rcl=489ea56faf3d58a0c940cec510a1dc514747d64c&l=758&gt
> ;
> > - WebTextCheckingCompletionImpl
> >
> <https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/
> TextCheckerClientImpl.cpp?rcl=489ea56faf3d58a0c940cec510a1dc
> 514747d64c&l=56&gt;
> >
> > The abstract factory would also be useful for places where ::Create is
> > being called on the impl (e.g. here
> >
> <https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/
> WebSharedWorkerImpl.cpp?rcl=e65dd51e10accc80aa2be79129912f47357cb958&l=341
> >).
> > In short it's the easiest way to tease apart all of these different
> > dependency patterns. It's also the easiest for us to revert - just delete
> > the factory and fix all of the call sites.
>
> I think it might be easier to justify an abstract factory as a separate
> task
> improving Blink test coverage (i.e. show how this concretely improves
> testability of various code in Blink).
>
> Specifically for something like WebFileChooserCompletionImpl, I'm not sure
> we
> want to substitute another implementation. It seems like abstraction point
> can
> be WebFrameClient::RunFileChooser, which is a virtual. A similar comment
> applies
> to WebTextCheckingCompletionImpl: the specifics are an implementation
> detail
> which is just to plumb things between core and web, so the real thing to
> test is
> the public embedder API (by calling WebLocalFrame::SetTextCheckClient).
>
>
> >
> >
> >
> > On Tue, May 23, 2017 at 1:01 PM, <mailto:dcheng@chromium.org> wrote:
> >
> > > Ah, that's a good point. However, I think things like ChromeClient are
> > > probably
> > > things we don't want to be mocking out in the future anyway: it was
> > > historically
> > > just a way to call into the web directory. I'm guessing (I may be
> overly
> > > optimistic here) that we will eventually only need to mock out the
> public
> > > API
> > > layer for testing: we already do this today for tests in
> Source/web/tests,
> > > and
> > > once web and core are merged, there's no reason core tests can't do
> that as
> > > well.
> > >
> > > On 2017/05/23 02:15:47, slangley wrote:
> > > > In this case, mocking out ChromeClient would be the most obvious
> example.
> > > >
> > > > On Tue, May 23, 2017 at 12:02 PM, <mailto:dcheng@chromium.org>
> wrote:
> > > >
> > > > > On 2017/05/23 01:59:58, slangley wrote:
> > > > > > Ahhh - so your suggestion is to add per class factory methods
> > > instead?
> > > > > >
> > > > > > Ideally it would be better to decouple object creation from usage
> > > tho,
> > > > > > which that wouldn't achieve.
> > > > >
> > > > > Can you help me understand this comment better with an example? It
> > > seems
> > > > > to me
> > > > > like once we no longer have the Impl we'll just refer to
> ChromeClass,
> > > etc
> > > > > directly anyway.
> > > > >
> > > > > >
> > > > > > On Tue, May 23, 2017 at 11:52 AM, <mailto:dcheng@chromium.org>
> > > wrote:
> > > > > >
> > > > > > > On 2017/05/23 01:49:43, slangley wrote:
> > > > > > > > On 2017/05/23 at 00:18:47, dcheng wrote:
> > > > > > > > > On 2017/05/23 00:00:11, slangley wrote:
> > > > > > > > > > On 2017/05/22 at 13:40:40, haraken wrote:
> > > > > > > > > > > Sorry for the review delay! This slipped off my head...
> > > > > > > > > > >
> > > > > > > > > > > How many methods are going to be added to WebFactory?
> We
> > > > > already
> > > > > > > have
> > > > > > > > > > ChromeClient to bypass the inverted dependency from
> core/ to
> > > > > web/. I
> > > > > > > think
> > > > > > > > we
> > > > > > > > > > should just use ChromeClient instead of introducing a new
> > > class
> > > > > that
> > > > > > > does
> > > > > > > a
> > > > > > > > > > similar thing.
> > > > > > > > > > >
> > > > > > > > > > > Can we move the contents of WebInitializer to
> > > > > WebKit::Initialize
> > > > > > > (this
> > > > > > > is
> > > > > > > > > > essentially web/ layer's initializer)?
> > > > > > > > > >
> > > > > > > > > > We want to leverage this technique to construct many of
> the
> > > *Impl
> > > > > > > classes
> > > > > > > > that
> > > > > > > > > > are in Source/web/ today.
> > > > > > > > > >
> > > > > > > > > > This would appear to be the cleanest way to achieve this
> - I
> > > > > don't
> > > > > > > think
> > > > > > > we
> > > > > > > > want
> > > > > > > > > > to try and wedge this into ChromeClient for all of the
> use
> > > cases
> > > > > > > (and it
> > > > > > > may
> > > > > > > > not
> > > > > > > > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
> > > > > > > > > > AnimationWorkletProxyClientImpl to name a few).
> > > > > > > > > >
> > > > > > > > > > This approach provides a nice separation of concerns.
> > > > > > > > >
> > > > > > > > > Another alternative would be to define
> ChromeClient::Create:
> > > would
> > > > > > > that work
> > > > > > > > for this? The nice part is this would be a natural
> progression
> > > of the
> > > > > > > code as
> > > > > > > we
> > > > > > > > collapse the impl classes into the interfaces.
> > > > > > > >
> > > > > > > > It's not clear (to me) the benefit of adding something like
> this
> > > to
> > > > > > > ChromeClient
> > > > > > > > that already has a large surface area, opposed to a dedicated
> > > class
> > > > > for
> > > > > > > object
> > > > > > > > construction. It potentially makes it harder for use to move
> > > classes
> > > > > out
> > > > > > > of
> > > > > > > Web/
> > > > > > > > as ChromeClientImpl would be stuck there.
> > > > > > >
> > > > > > > Sorry, to be clear, my suggestion is:
> > > > > > >
> > > > > > > ChromeClient::Create() would be defined in ChromeClient.h and
> be
> > > > > defined in
> > > > > > > ChromeClientImpl.cpp.
> > > > > > >
> > > > > > > Similarly, any other classes that need to be constructed would
> be
> > > > > defined
> > > > > > > in the
> > > > > > > base interface header, but defined in the implementation cpp.
> > > > > > >
> > > > > > > As we collapse the class hierachy down, we won't need to
> change the
> > > > > > > Create()
> > > > > > > function (unless we want to get rid of it), since
> ChromeClientImpl
> > > > > folds
> > > > > > > into
> > > > > > > ChromeClient, etc.
> > > > > > >
> > > > > > > https://codereview.chromium.org/2887523003/
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > You received this message because you are subscribed to the
> Google
> > > Groups
> > > > > > "Chromium-reviews" group.
> > > > > > To unsubscribe from this group and stop receiving emails from it,
> > > send an
> > > > > email
> > > > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > > > >
> > > > > https://codereview.chromium.org/2887523003/
> > > > >
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google
> Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it,
> send an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > > https://codereview.chromium.org/2887523003/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2887523003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[slangley, 2017-05-23 06:39:21]

Sorry, after all of that discussion I finally remembered why static ctor()
is not going to work well in this case.

When we try and use static ctor() we end up with linker errors if the
static method is defined in core/ but implemented in web/ and we're calling
it from core.

I'll try and illustrate:

In this case we cave Foo::Create() being called from BarImpl.cpp and
Bar::Create() being called from FooImpl.cpp

In core:     Foo.h                  Bar.h

In Web:    FooImpl.cpp  <--> BarImpl.cpp

If we move BarImpl.cpp to core, it can still see the defintion of
Foo::Create in Foo.h so compilation is fine but the linker will not be able
to find the implementation as it is in web/ and we don't link web/ to
core/, so the link step fails (on windows).

We can fix this with the factory pattern as it's not statically defining
the constructor so the linker does not need the implementation.

Daniel is concerned that if we introduce the factory then people will start
using it for other purposes than what we intend here. I think we can solve
this by making the Impl a friend of the abstract factory and making the
SetInstance method private.



On Tue, May 23, 2017 at 1:56 PM, Stuart Langley <slangley@chromium.org>
wrote:

> Yes to be clear the reason for having this right now is to tease apart all
> of these classes so we can move them from web to new homes in modules or
> core - while they are many entangled references it makes it a hard job to
> move things in logical units. The factory lets us localize these
> dependencies to one location and will cater for calls to new or static
> Create methods. The other suggestions are not going to cover 100% of our
> use cases (adding static Create to the abstract class for example).
>
> Any discussions of mocking etc are really just hypothetical further
> potential use cases, but are not in any way a motivation for this CL. Does
> that make sense?
>
> On Tue, May 23, 2017 at 1:45 PM, <dcheng@chromium.org> wrote:
>
>> On 2017/05/23 03:26:35, slangley wrote:
>> > Well we kinda mock it today with EmptyChromeClient in places, although
>> it
>> > not solely used for tests. We cheat a bit by making the members public
>> >
>> <https://cs.chromium.org/chromium/src/third_party/WebKit/
>> Source/core/page/Page.h?rcl=df82c2f1f609cfe5cead26794eeb8532
>> 0594c904&l=100&gt;
>> > so
>> > you can poke them.
>>
>> I think as we fold web into core, ChromeClient will not be the right
>> place to
>> have mocks in these tests. Mocking at this level risks having behavior
>> that
>> diverges from what the actual impl classes in web do.
>>
>> >
>> > The other use case for having an abstract factory rather than static
>> Create
>> > methods is where the Impl class directly implements a class defined in
>> > public/web - where we cannot change the public API.
>> >
>> > examples:
>> > - WebFileChooserCompletionImpl
>> >
>> <https://cs.chromium.org/chromium/src/third_party/WebKit/
>> Source/web/ChromeClientImpl.cpp?rcl=489ea56faf3d58a0c940ce
>> c510a1dc514747d64c&l=758&gt;
>> > - WebTextCheckingCompletionImpl
>> >
>> <https://cs.chromium.org/chromium/src/third_party/WebKit/
>> Source/web/TextCheckerClientImpl.cpp?rcl=489ea56faf3d58a0c94
>> 0cec510a1dc514747d64c&l=56&gt;
>> >
>> > The abstract factory would also be useful for places where ::Create is
>> > being called on the impl (e.g. here
>> >
>> <https://cs.chromium.org/chromium/src/third_party/WebKit/
>> Source/web/WebSharedWorkerImpl.cpp?rcl=e65dd51e10accc80aa2be
>> 79129912f47357cb958&l=341>).
>> > In short it's the easiest way to tease apart all of these different
>> > dependency patterns. It's also the easiest for us to revert - just
>> delete
>> > the factory and fix all of the call sites.
>>
>> I think it might be easier to justify an abstract factory as a separate
>> task
>> improving Blink test coverage (i.e. show how this concretely improves
>> testability of various code in Blink).
>>
>> Specifically for something like WebFileChooserCompletionImpl, I'm not
>> sure we
>> want to substitute another implementation. It seems like abstraction
>> point can
>> be WebFrameClient::RunFileChooser, which is a virtual. A similar comment
>> applies
>> to WebTextCheckingCompletionImpl: the specifics are an implementation
>> detail
>> which is just to plumb things between core and web, so the real thing to
>> test is
>> the public embedder API (by calling WebLocalFrame::SetTextCheckClient).
>>
>>
>> >
>> >
>> >
>> > On Tue, May 23, 2017 at 1:01 PM, <mailto:dcheng@chromium.org> wrote:
>> >
>> > > Ah, that's a good point. However, I think things like ChromeClient are
>> > > probably
>> > > things we don't want to be mocking out in the future anyway: it was
>> > > historically
>> > > just a way to call into the web directory. I'm guessing (I may be
>> overly
>> > > optimistic here) that we will eventually only need to mock out the
>> public
>> > > API
>> > > layer for testing: we already do this today for tests in
>> Source/web/tests,
>> > > and
>> > > once web and core are merged, there's no reason core tests can't do
>> that as
>> > > well.
>> > >
>> > > On 2017/05/23 02:15:47, slangley wrote:
>> > > > In this case, mocking out ChromeClient would be the most obvious
>> example.
>> > > >
>> > > > On Tue, May 23, 2017 at 12:02 PM, <mailto:dcheng@chromium.org>
>> wrote:
>> > > >
>> > > > > On 2017/05/23 01:59:58, slangley wrote:
>> > > > > > Ahhh - so your suggestion is to add per class factory methods
>> > > instead?
>> > > > > >
>> > > > > > Ideally it would be better to decouple object creation from
>> usage
>> > > tho,
>> > > > > > which that wouldn't achieve.
>> > > > >
>> > > > > Can you help me understand this comment better with an example? It
>> > > seems
>> > > > > to me
>> > > > > like once we no longer have the Impl we'll just refer to
>> ChromeClass,
>> > > etc
>> > > > > directly anyway.
>> > > > >
>> > > > > >
>> > > > > > On Tue, May 23, 2017 at 11:52 AM, <mailto:dcheng@chromium.org>
>> > > wrote:
>> > > > > >
>> > > > > > > On 2017/05/23 01:49:43, slangley wrote:
>> > > > > > > > On 2017/05/23 at 00:18:47, dcheng wrote:
>> > > > > > > > > On 2017/05/23 00:00:11, slangley wrote:
>> > > > > > > > > > On 2017/05/22 at 13:40:40, haraken wrote:
>> > > > > > > > > > > Sorry for the review delay! This slipped off my
>> head...
>> > > > > > > > > > >
>> > > > > > > > > > > How many methods are going to be added to WebFactory?
>> We
>> > > > > already
>> > > > > > > have
>> > > > > > > > > > ChromeClient to bypass the inverted dependency from
>> core/ to
>> > > > > web/. I
>> > > > > > > think
>> > > > > > > > we
>> > > > > > > > > > should just use ChromeClient instead of introducing a
>> new
>> > > class
>> > > > > that
>> > > > > > > does
>> > > > > > > a
>> > > > > > > > > > similar thing.
>> > > > > > > > > > >
>> > > > > > > > > > > Can we move the contents of WebInitializer to
>> > > > > WebKit::Initialize
>> > > > > > > (this
>> > > > > > > is
>> > > > > > > > > > essentially web/ layer's initializer)?
>> > > > > > > > > >
>> > > > > > > > > > We want to leverage this technique to construct many of
>> the
>> > > *Impl
>> > > > > > > classes
>> > > > > > > > that
>> > > > > > > > > > are in Source/web/ today.
>> > > > > > > > > >
>> > > > > > > > > > This would appear to be the cleanest way to achieve
>> this - I
>> > > > > don't
>> > > > > > > think
>> > > > > > > we
>> > > > > > > > want
>> > > > > > > > > > to try and wedge this into ChromeClient for all of the
>> use
>> > > cases
>> > > > > > > (and it
>> > > > > > > may
>> > > > > > > > not
>> > > > > > > > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
>> > > > > > > > > > AnimationWorkletProxyClientImpl to name a few).
>> > > > > > > > > >
>> > > > > > > > > > This approach provides a nice separation of concerns.
>> > > > > > > > >
>> > > > > > > > > Another alternative would be to define
>> ChromeClient::Create:
>> > > would
>> > > > > > > that work
>> > > > > > > > for this? The nice part is this would be a natural
>> progression
>> > > of the
>> > > > > > > code as
>> > > > > > > we
>> > > > > > > > collapse the impl classes into the interfaces.
>> > > > > > > >
>> > > > > > > > It's not clear (to me) the benefit of adding something like
>> this
>> > > to
>> > > > > > > ChromeClient
>> > > > > > > > that already has a large surface area, opposed to a
>> dedicated
>> > > class
>> > > > > for
>> > > > > > > object
>> > > > > > > > construction. It potentially makes it harder for use to move
>> > > classes
>> > > > > out
>> > > > > > > of
>> > > > > > > Web/
>> > > > > > > > as ChromeClientImpl would be stuck there.
>> > > > > > >
>> > > > > > > Sorry, to be clear, my suggestion is:
>> > > > > > >
>> > > > > > > ChromeClient::Create() would be defined in ChromeClient.h and
>> be
>> > > > > defined in
>> > > > > > > ChromeClientImpl.cpp.
>> > > > > > >
>> > > > > > > Similarly, any other classes that need to be constructed
>> would be
>> > > > > defined
>> > > > > > > in the
>> > > > > > > base interface header, but defined in the implementation cpp.
>> > > > > > >
>> > > > > > > As we collapse the class hierachy down, we won't need to
>> change the
>> > > > > > > Create()
>> > > > > > > function (unless we want to get rid of it), since
>> ChromeClientImpl
>> > > > > folds
>> > > > > > > into
>> > > > > > > ChromeClient, etc.
>> > > > > > >
>> > > > > > > https://codereview.chromium.org/2887523003/
>> > > > > > >
>> > > > > >
>> > > > > > --
>> > > > > > You received this message because you are subscribed to the
>> Google
>> > > Groups
>> > > > > > "Chromium-reviews" group.
>> > > > > > To unsubscribe from this group and stop receiving emails from
>> it,
>> > > send an
>> > > > > email
>> > > > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>> > > > >
>> > > > > https://codereview.chromium.org/2887523003/
>> > > > >
>> > > >
>> > > > --
>> > > > You received this message because you are subscribed to the Google
>> Groups
>> > > > "Chromium-reviews" group.
>> > > > To unsubscribe from this group and stop receiving emails from it,
>> send an
>> > > email
>> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>> > >
>> > > https://codereview.chromium.org/2887523003/
>> > >
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>> Groups
>> > "Chromium-reviews" group.
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>>
>>
>> https://codereview.chromium.org/2887523003/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[slangley, 2017-05-23 06:39:22]

Sorry, after all of that discussion I finally remembered why static ctor()
is not going to work well in this case.

When we try and use static ctor() we end up with linker errors if the
static method is defined in core/ but implemented in web/ and we're calling
it from core.

I'll try and illustrate:

In this case we cave Foo::Create() being called from BarImpl.cpp and
Bar::Create() being called from FooImpl.cpp

In core:     Foo.h                  Bar.h

In Web:    FooImpl.cpp  <--> BarImpl.cpp

If we move BarImpl.cpp to core, it can still see the defintion of
Foo::Create in Foo.h so compilation is fine but the linker will not be able
to find the implementation as it is in web/ and we don't link web/ to
core/, so the link step fails (on windows).

We can fix this with the factory pattern as it's not statically defining
the constructor so the linker does not need the implementation.

Daniel is concerned that if we introduce the factory then people will start
using it for other purposes than what we intend here. I think we can solve
this by making the Impl a friend of the abstract factory and making the
SetInstance method private.



On Tue, May 23, 2017 at 1:56 PM, Stuart Langley <slangley@chromium.org>
wrote:

> Yes to be clear the reason for having this right now is to tease apart all
> of these classes so we can move them from web to new homes in modules or
> core - while they are many entangled references it makes it a hard job to
> move things in logical units. The factory lets us localize these
> dependencies to one location and will cater for calls to new or static
> Create methods. The other suggestions are not going to cover 100% of our
> use cases (adding static Create to the abstract class for example).
>
> Any discussions of mocking etc are really just hypothetical further
> potential use cases, but are not in any way a motivation for this CL. Does
> that make sense?
>
> On Tue, May 23, 2017 at 1:45 PM, <dcheng@chromium.org> wrote:
>
>> On 2017/05/23 03:26:35, slangley wrote:
>> > Well we kinda mock it today with EmptyChromeClient in places, although
>> it
>> > not solely used for tests. We cheat a bit by making the members public
>> >
>> <https://cs.chromium.org/chromium/src/third_party/WebKit/
>> Source/core/page/Page.h?rcl=df82c2f1f609cfe5cead26794eeb8532
>> 0594c904&l=100&gt;
>> > so
>> > you can poke them.
>>
>> I think as we fold web into core, ChromeClient will not be the right
>> place to
>> have mocks in these tests. Mocking at this level risks having behavior
>> that
>> diverges from what the actual impl classes in web do.
>>
>> >
>> > The other use case for having an abstract factory rather than static
>> Create
>> > methods is where the Impl class directly implements a class defined in
>> > public/web - where we cannot change the public API.
>> >
>> > examples:
>> > - WebFileChooserCompletionImpl
>> >
>> <https://cs.chromium.org/chromium/src/third_party/WebKit/
>> Source/web/ChromeClientImpl.cpp?rcl=489ea56faf3d58a0c940ce
>> c510a1dc514747d64c&l=758&gt;
>> > - WebTextCheckingCompletionImpl
>> >
>> <https://cs.chromium.org/chromium/src/third_party/WebKit/
>> Source/web/TextCheckerClientImpl.cpp?rcl=489ea56faf3d58a0c94
>> 0cec510a1dc514747d64c&l=56&gt;
>> >
>> > The abstract factory would also be useful for places where ::Create is
>> > being called on the impl (e.g. here
>> >
>> <https://cs.chromium.org/chromium/src/third_party/WebKit/
>> Source/web/WebSharedWorkerImpl.cpp?rcl=e65dd51e10accc80aa2be
>> 79129912f47357cb958&l=341>).
>> > In short it's the easiest way to tease apart all of these different
>> > dependency patterns. It's also the easiest for us to revert - just
>> delete
>> > the factory and fix all of the call sites.
>>
>> I think it might be easier to justify an abstract factory as a separate
>> task
>> improving Blink test coverage (i.e. show how this concretely improves
>> testability of various code in Blink).
>>
>> Specifically for something like WebFileChooserCompletionImpl, I'm not
>> sure we
>> want to substitute another implementation. It seems like abstraction
>> point can
>> be WebFrameClient::RunFileChooser, which is a virtual. A similar comment
>> applies
>> to WebTextCheckingCompletionImpl: the specifics are an implementation
>> detail
>> which is just to plumb things between core and web, so the real thing to
>> test is
>> the public embedder API (by calling WebLocalFrame::SetTextCheckClient).
>>
>>
>> >
>> >
>> >
>> > On Tue, May 23, 2017 at 1:01 PM, <mailto:dcheng@chromium.org> wrote:
>> >
>> > > Ah, that's a good point. However, I think things like ChromeClient are
>> > > probably
>> > > things we don't want to be mocking out in the future anyway: it was
>> > > historically
>> > > just a way to call into the web directory. I'm guessing (I may be
>> overly
>> > > optimistic here) that we will eventually only need to mock out the
>> public
>> > > API
>> > > layer for testing: we already do this today for tests in
>> Source/web/tests,
>> > > and
>> > > once web and core are merged, there's no reason core tests can't do
>> that as
>> > > well.
>> > >
>> > > On 2017/05/23 02:15:47, slangley wrote:
>> > > > In this case, mocking out ChromeClient would be the most obvious
>> example.
>> > > >
>> > > > On Tue, May 23, 2017 at 12:02 PM, <mailto:dcheng@chromium.org>
>> wrote:
>> > > >
>> > > > > On 2017/05/23 01:59:58, slangley wrote:
>> > > > > > Ahhh - so your suggestion is to add per class factory methods
>> > > instead?
>> > > > > >
>> > > > > > Ideally it would be better to decouple object creation from
>> usage
>> > > tho,
>> > > > > > which that wouldn't achieve.
>> > > > >
>> > > > > Can you help me understand this comment better with an example? It
>> > > seems
>> > > > > to me
>> > > > > like once we no longer have the Impl we'll just refer to
>> ChromeClass,
>> > > etc
>> > > > > directly anyway.
>> > > > >
>> > > > > >
>> > > > > > On Tue, May 23, 2017 at 11:52 AM, <mailto:dcheng@chromium.org>
>> > > wrote:
>> > > > > >
>> > > > > > > On 2017/05/23 01:49:43, slangley wrote:
>> > > > > > > > On 2017/05/23 at 00:18:47, dcheng wrote:
>> > > > > > > > > On 2017/05/23 00:00:11, slangley wrote:
>> > > > > > > > > > On 2017/05/22 at 13:40:40, haraken wrote:
>> > > > > > > > > > > Sorry for the review delay! This slipped off my
>> head...
>> > > > > > > > > > >
>> > > > > > > > > > > How many methods are going to be added to WebFactory?
>> We
>> > > > > already
>> > > > > > > have
>> > > > > > > > > > ChromeClient to bypass the inverted dependency from
>> core/ to
>> > > > > web/. I
>> > > > > > > think
>> > > > > > > > we
>> > > > > > > > > > should just use ChromeClient instead of introducing a
>> new
>> > > class
>> > > > > that
>> > > > > > > does
>> > > > > > > a
>> > > > > > > > > > similar thing.
>> > > > > > > > > > >
>> > > > > > > > > > > Can we move the contents of WebInitializer to
>> > > > > WebKit::Initialize
>> > > > > > > (this
>> > > > > > > is
>> > > > > > > > > > essentially web/ layer's initializer)?
>> > > > > > > > > >
>> > > > > > > > > > We want to leverage this technique to construct many of
>> the
>> > > *Impl
>> > > > > > > classes
>> > > > > > > > that
>> > > > > > > > > > are in Source/web/ today.
>> > > > > > > > > >
>> > > > > > > > > > This would appear to be the cleanest way to achieve
>> this - I
>> > > > > don't
>> > > > > > > think
>> > > > > > > we
>> > > > > > > > want
>> > > > > > > > > > to try and wedge this into ChromeClient for all of the
>> use
>> > > cases
>> > > > > > > (and it
>> > > > > > > may
>> > > > > > > > not
>> > > > > > > > > > even be possible: e.g: CompositorWorkerProxyClientImpl,
>> > > > > > > > > > AnimationWorkletProxyClientImpl to name a few).
>> > > > > > > > > >
>> > > > > > > > > > This approach provides a nice separation of concerns.
>> > > > > > > > >
>> > > > > > > > > Another alternative would be to define
>> ChromeClient::Create:
>> > > would
>> > > > > > > that work
>> > > > > > > > for this? The nice part is this would be a natural
>> progression
>> > > of the
>> > > > > > > code as
>> > > > > > > we
>> > > > > > > > collapse the impl classes into the interfaces.
>> > > > > > > >
>> > > > > > > > It's not clear (to me) the benefit of adding something like
>> this
>> > > to
>> > > > > > > ChromeClient
>> > > > > > > > that already has a large surface area, opposed to a
>> dedicated
>> > > class
>> > > > > for
>> > > > > > > object
>> > > > > > > > construction. It potentially makes it harder for use to move
>> > > classes
>> > > > > out
>> > > > > > > of
>> > > > > > > Web/
>> > > > > > > > as ChromeClientImpl would be stuck there.
>> > > > > > >
>> > > > > > > Sorry, to be clear, my suggestion is:
>> > > > > > >
>> > > > > > > ChromeClient::Create() would be defined in ChromeClient.h and
>> be
>> > > > > defined in
>> > > > > > > ChromeClientImpl.cpp.
>> > > > > > >
>> > > > > > > Similarly, any other classes that need to be constructed
>> would be
>> > > > > defined
>> > > > > > > in the
>> > > > > > > base interface header, but defined in the implementation cpp.
>> > > > > > >
>> > > > > > > As we collapse the class hierachy down, we won't need to
>> change the
>> > > > > > > Create()
>> > > > > > > function (unless we want to get rid of it), since
>> ChromeClientImpl
>> > > > > folds
>> > > > > > > into
>> > > > > > > ChromeClient, etc.
>> > > > > > >
>> > > > > > > https://codereview.chromium.org/2887523003/
>> > > > > > >
>> > > > > >
>> > > > > > --
>> > > > > > You received this message because you are subscribed to the
>> Google
>> > > Groups
>> > > > > > "Chromium-reviews" group.
>> > > > > > To unsubscribe from this group and stop receiving emails from
>> it,
>> > > send an
>> > > > > email
>> > > > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>> > > > >
>> > > > > https://codereview.chromium.org/2887523003/
>> > > > >
>> > > >
>> > > > --
>> > > > You received this message because you are subscribed to the Google
>> Groups
>> > > > "Chromium-reviews" group.
>> > > > To unsubscribe from this group and stop receiving emails from it,
>> send an
>> > > email
>> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>> > >
>> > > https://codereview.chromium.org/2887523003/
>> > >
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>> Groups
>> > "Chromium-reviews" group.
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>>
>>
>> https://codereview.chromium.org/2887523003/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sashab, 2017-05-29 04:32:38]

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

[commit-bot: I haz the power, 2017-05-29 04:32:51]

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-29 04:34:01]

Thanks slangley@ for the great explanation. Yes, it is not possible to put this
code in ChromeClient due to linker errors.

I've made the following main changes, based on the rest of the discussion:
- Moved WebFactory into core/exported
- Inlined WebInitializer into WebKit::Initialize
- Made SetInstance() take a reference instead of a pointer

PTAL again dcheng and haraken, the patch is also a lot smaller now.

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/WebFactory.cpp (right):

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/WebFactory.cpp:19: DCHECK(factory);
On 2017/05/22 at 09:16:49, dcheng wrote:
> Nit: in Blink, it's more customary to pass never-null mutable pointers as
mutable refs.

Done.

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

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebKit.cpp:82:
DEFINE_STATIC_LOCAL(std::unique_ptr<WebInitializer>, initializer,
On 2017/05/22 at 09:16:49, dcheng wrote:
> Nit: Just say WebInitializer directly here; statics defined by this macro are
leaked on destruction anyway.

Removed this code and inlined into Initialize method.

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

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:342:
chrome_client_(WebFactory::GetInstance().CreateChromeClient(this)),
On 2017/05/23 at 01:56:35, haraken wrote:
> Just to clarify: How will this code be in the end state? Will this be reverted
back to:
> 
>   chrome_client_impl_(ChromeClientImpl::Create(this))
> 
> ?
> 
> (In the real end state we should remove ChromeClientImpl though.)

Yes to both. :)

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2887523003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.h:609: Persistent<blink::ChromeClient>
chrome_client_;
On 2017/05/23 at 01:56:35, haraken wrote:
> Drop blink::.

Can't, name clash with method above :( Added comment.

[slangley, 2017-05-29 04:42:57]

I believe Daniels main concern was ppl overloading the use of this factory.

We should make SetInstance() private and give WebFactoryImpl friendship so there
are no other users trying to use WebFactory.

[commit-bot: I haz the power, 2017-05-29 07:14:44]

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

[commit-bot: I haz the power, 2017-05-29 07:14:45]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-29 08:02:30]

LGTM with comments addressed

(I'd slightly prefer SetInstance to be private but I don't feel super strongly
about this)

https://codereview.chromium.org/2887523003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2887523003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.h:607: // method.
Huh... maybe let's just rename ChromeClient() to GetChromeClient() since that's
how we generally resolved the other naming conflicts.

[haraken, 2017-05-29 08:39:37]

LGTM

[sashab, 2017-05-30 02:54:13]

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

[sashab, 2017-05-30 02:54:15]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-30 02:54:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
slangley@chromium.org, haraken@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2887523003/#ps80001
(title: "Added TODOs for review feedback")

[commit-bot: I haz the power, 2017-05-30 02:54:41]

CQ is trying da patch.

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

[sashab, 2017-05-30 02:56:18]

Added TODO to make SetInstance private, since there are multiple ways to do this
lets discuss on a follow-up patch.

Thanks all!

https://codereview.chromium.org/2887523003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2887523003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.h:607: // method.
On 2017/05/29 at 08:02:30, dcheng wrote:
> Huh... maybe let's just rename ChromeClient() to GetChromeClient() since
that's how we generally resolved the other naming conflicts.

Oh yeah, forgot about the Get prefix trick :) Will do that.

It's a non-tiny change since ChromeClient() is inherited from WebView... Added
TODO and will do in follow-up patch.

[commit-bot: I haz the power, 2017-05-30 04:09:34]

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

[commit-bot: I haz the power, 2017-05-30 04:09:35]

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

[sashab, 2017-05-30 04:13:42]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-05-30 04:13:56]

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-30 05:17:49]

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

[commit-bot: I haz the power, 2017-05-30 05:17:51]

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

[dcheng, 2017-05-30 06:21:05]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-05-30 06:21:20]

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-30 07:16:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496125265727930,
"parent_rev": "06c4e174ed940045202174b231ff02225088395d", "commit_rev":
"8d376e7d11c6d2efe83f46fa043f036b0f6246fe"}

[commit-bot: I haz the power, 2017-05-30 07:16:31]

Description was changed from

==========
Introduced WebFactory and WebFactoryImpl for constructing web/ classes

Introduced WebFactory and WebFactoryImpl for constructing web/ classes
outside of core/.

This is a temporary class that will be removed once web/ is removed.

BUG=712963
==========

to

==========
Introduced WebFactory and WebFactoryImpl for constructing web/ classes

Introduced WebFactory and WebFactoryImpl for constructing web/ classes
outside of core/.

This is a temporary class that will be removed once web/ is removed.

BUG=712963

Review-Url: https://codereview.chromium.org/2887523003
Cr-Commit-Position: refs/heads/master@{#475451}
Committed:
https://chromium.googlesource.com/chromium/src/+/8d376e7d11c6d2efe83f46fa043f...
==========

[commit-bot: I haz the power, 2017-05-30 07:16:33]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/8d376e7d11c6d2efe83f46fa043f...