Move WebAssociatedURLLoaderImpl.* from web/ -> core/export/.

Move WebAssociatedURLLoaderImpl.* from web/ -> core/export/.

WebAssociatedURLLoaderImpl had a dependency on WebLocalFrameImpl just so it could
call a method to obtain the document for the frame. To break this dependency the
ctor parameter was changed to take a Document* directly.

BUG=708879
Review-Url: https://codereview.chromium.org/2831833002
Cr-Commit-Position: refs/heads/master@{#466601}
Committed: https://chromium.googlesource.com/chromium/src/+/fd61327a91c26467fae63304cc1ee343405344e6

[slangley, 2017-04-20 01:12:28]

Description was changed from

==========
Move WebAssociatedURLLoaderImpl.* from web/ -> core/export/.

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

to

==========
Move WebAssociatedURLLoaderImpl.* from web/ -> core/export/.

WebAssociatedURLLoaderImpl had a dependency on WebLocalFrameImpl just so it
could call a method to obtain the document for the frame. To break this
dependency the ctor parameter was changed to take a Document* directly.

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

[slangley, 2017-04-20 01:12:28]

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

[slangley, 2017-04-20 01:18:42]

Description was changed from

==========
Move WebAssociatedURLLoaderImpl.* from web/ -> core/export/.

WebAssociatedURLLoaderImpl had a dependency on WebLocalFrameImpl just so it
could call a method to obtain the document for the frame. To break this
dependency the ctor parameter was changed to take a Document* directly.

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

to

==========
Move WebAssociatedURLLoaderImpl.* from web/ -> core/export/.

WebAssociatedURLLoaderImpl had a dependency on WebLocalFrameImpl just so it
could
call a method to obtain the document for the frame. To break this dependency the
ctor parameter was changed to take a Document* directly.

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

[sashab, 2017-04-20 01:20:04]

lgtm

[slangley, 2017-04-20 01:22:34]

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

[slangley, 2017-04-20 01:22:35]

haraken@ - ptal

[haraken, 2017-04-20 07:34:28]

LGTM

https://codereview.chromium.org/2831833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/exported/BUILD.gn (right):

https://codereview.chromium.org/2831833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/exported/BUILD.gn:9:
"WebAssociatedURLLoaderImpl.h",

I'd prefer putting only .cpp implementation files of public APIs in
xxx/exported/. (platform/exported/ mostly follows the rule.)

So can we probably move these files to core/loader/ or somewhere?

[slangley, 2017-04-21 00:21:03]

On 2017/04/20 at 07:34:28, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/2831833002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/exported/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2831833002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/exported/BUILD.gn:9:
"WebAssociatedURLLoaderImpl.h",
> 
> I'd prefer putting only .cpp implementation files of public APIs in
xxx/exported/. (platform/exported/ mostly follows the rule.)
> 
> So can we probably move these files to core/loader/ or somewhere?

In platform/exported if the impl pattern is being used then the header file is
also in platform/exported
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/expor...).
Ideally we can remove these impls but in this case it would take some work that
would be better suited for a followup CL.

In this case, it's implementing "public/web/WebAssociatedURLLoader.h" so
core/exported should be the right place to put it per our previous
conversations, right?

[slangley, 2017-04-21 00:21:49]

On 2017/04/21 at 00:21:03, slangley wrote:
> On 2017/04/20 at 07:34:28, haraken wrote:
> > LGTM
> > 
> >
https://codereview.chromium.org/2831833002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/exported/BUILD.gn (right):
> > 
> >
https://codereview.chromium.org/2831833002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/exported/BUILD.gn:9:
"WebAssociatedURLLoaderImpl.h",
> > 
> > I'd prefer putting only .cpp implementation files of public APIs in
xxx/exported/. (platform/exported/ mostly follows the rule.)
> > 
> > So can we probably move these files to core/loader/ or somewhere?
> 
> In platform/exported if the impl pattern is being used then the header file is
also in platform/exported
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/expor...).
Ideally we can remove these impls but in this case it would take some work that
would be better suited for a followup CL.
> 
> In this case, it's implementing "public/web/WebAssociatedURLLoader.h" so
core/exported should be the right place to put it per our previous
conversations, right?

As this is the root of a series of patches i'll submit - if you feel strongly
about the points you raised above I can address them in a followup CL.

Thanks!

[slangley, 2017-04-21 00:25:18]

The CQ bit was checked by slangley@chromium.org

[commit-bot: I haz the power, 2017-04-21 00:25:41]

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-04-21 00:28:06]

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

[commit-bot: I haz the power, 2017-04-21 00:28:07]

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

[haraken, 2017-04-21 06:30:55]

On 2017/04/21 00:21:03, slangley wrote:
> On 2017/04/20 at 07:34:28, haraken wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/2831833002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/exported/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2831833002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/exported/BUILD.gn:9:
> "WebAssociatedURLLoaderImpl.h",
> > 
> > I'd prefer putting only .cpp implementation files of public APIs in
> xxx/exported/. (platform/exported/ mostly follows the rule.)
> > 
> > So can we probably move these files to core/loader/ or somewhere?
> 
> In platform/exported if the impl pattern is being used then the header file is
> also in platform/exported
>
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/expor...).
> Ideally we can remove these impls but in this case it would take some work
that
> would be better suited for a followup CL.
> 
> In this case, it's implementing "public/web/WebAssociatedURLLoader.h" so
> core/exported should be the right place to put it per our previous
> conversations, right?

I closely looked at those impl files and yeah, your point makes sense.

Actually this seems like a gray zone. Classes that inherit from WebXXX are in
both platform/exported/ and other places (platform/xxx/, core/xxx/, modules/xxx/
etc). Maybe we should consider simply removing the exported/ directory but
anyway I now understand that this is going to be a bike-shed. Sorry about the
noise!

[slangley, 2017-04-21 06:35:42]

The additional methods in the impl definitions makes it quite difficult to
decouple the classes - Moving all of the files will require many
intermediate steps and creation of temporary files to make it possible to
move pieces in logical chunks.

On Fri, Apr 21, 2017 at 4:30 PM, <haraken@chromium.org> wrote:

> On 2017/04/21 00:21:03, slangley wrote:
> > On 2017/04/20 at 07:34:28, haraken wrote:
> > > LGTM
> > >
> > >
> >
> https://codereview.chromium.org/2831833002/diff/1/third_
> party/WebKit/Source/core/exported/BUILD.gn
> > > File third_party/WebKit/Source/core/exported/BUILD.gn (right):
> > >
> > >
> >
> https://codereview.chromium.org/2831833002/diff/1/third_
> party/WebKit/Source/core/exported/BUILD.gn#newcode9
> > > third_party/WebKit/Source/core/exported/BUILD.gn:9:
> > "WebAssociatedURLLoaderImpl.h",
> > >
> > > I'd prefer putting only .cpp implementation files of public APIs in
> > xxx/exported/. (platform/exported/ mostly follows the rule.)
> > >
> > > So can we probably move these files to core/loader/ or somewhere?
> >
> > In platform/exported if the impl pattern is being used then the header
> file is
> > also in platform/exported
> >
> (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/
> exported/WebSpeechSynthesizerClientImpl.h).
> > Ideally we can remove these impls but in this case it would take some
> work
> that
> > would be better suited for a followup CL.
> >
> > In this case, it's implementing "public/web/WebAssociatedURLLoader.h" so
> > core/exported should be the right place to put it per our previous
> > conversations, right?
>
> I closely looked at those impl files and yeah, your point makes sense.
>
> Actually this seems like a gray zone. Classes that inherit from WebXXX are
> in
> both platform/exported/ and other places (platform/xxx/, core/xxx/,
> modules/xxx/
> etc). Maybe we should consider simply removing the exported/ directory but
> anyway I now understand that this is going to be a bike-shed. Sorry about
> the
> noise!
>
>
>
> https://codereview.chromium.org/2831833002/
>

-- 
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-04-21 06:35:42]

The additional methods in the impl definitions makes it quite difficult to
decouple the classes - Moving all of the files will require many
intermediate steps and creation of temporary files to make it possible to
move pieces in logical chunks.

On Fri, Apr 21, 2017 at 4:30 PM, <haraken@chromium.org> wrote:

> On 2017/04/21 00:21:03, slangley wrote:
> > On 2017/04/20 at 07:34:28, haraken wrote:
> > > LGTM
> > >
> > >
> >
> https://codereview.chromium.org/2831833002/diff/1/third_
> party/WebKit/Source/core/exported/BUILD.gn
> > > File third_party/WebKit/Source/core/exported/BUILD.gn (right):
> > >
> > >
> >
> https://codereview.chromium.org/2831833002/diff/1/third_
> party/WebKit/Source/core/exported/BUILD.gn#newcode9
> > > third_party/WebKit/Source/core/exported/BUILD.gn:9:
> > "WebAssociatedURLLoaderImpl.h",
> > >
> > > I'd prefer putting only .cpp implementation files of public APIs in
> > xxx/exported/. (platform/exported/ mostly follows the rule.)
> > >
> > > So can we probably move these files to core/loader/ or somewhere?
> >
> > In platform/exported if the impl pattern is being used then the header
> file is
> > also in platform/exported
> >
> (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/
> exported/WebSpeechSynthesizerClientImpl.h).
> > Ideally we can remove these impls but in this case it would take some
> work
> that
> > would be better suited for a followup CL.
> >
> > In this case, it's implementing "public/web/WebAssociatedURLLoader.h" so
> > core/exported should be the right place to put it per our previous
> > conversations, right?
>
> I closely looked at those impl files and yeah, your point makes sense.
>
> Actually this seems like a gray zone. Classes that inherit from WebXXX are
> in
> both platform/exported/ and other places (platform/xxx/, core/xxx/,
> modules/xxx/
> etc). Maybe we should consider simply removing the exported/ directory but
> anyway I now understand that this is going to be a bike-shed. Sorry about
> the
> noise!
>
>
>
> https://codereview.chromium.org/2831833002/
>

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

[haraken, 2017-04-21 06:39:30]

On 2017/04/21 06:35:42, slangley wrote:
> The additional methods in the impl definitions makes it quite difficult to
> decouple the classes - Moving all of the files will require many
> intermediate steps and creation of temporary files to make it possible to
> move pieces in logical chunks.

I was thinking about just moving the impl files to some specific directories
(e.g., core/editing/) without splitting, but anyway I agree that it wouldn't be
worth an effort. It'd be nicer to focus on removing the web/ layer.

[slangley, 2017-04-23 23:03:23]

The CQ bit was checked by slangley@chromium.org

[commit-bot: I haz the power, 2017-04-23 23:03:28]

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-04-24 00:20:58]

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

[commit-bot: I haz the power, 2017-04-24 00:20:59]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[slangley, 2017-04-24 01:14:32]

haraken@ - I needed to add BLINK_EXPORT to the definition in public/web.

Please confirm that this additional change is OK.

[haraken, 2017-04-24 06:07:21]

LGTM

[slangley, 2017-04-24 06:28:34]

The CQ bit was checked by slangley@chromium.org

[slangley, 2017-04-24 06:28:35]

The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org
Link to the patchset: https://codereview.chromium.org/2831833002/#ps20001
(title: "Need to BLINK_EXPORT WebAssociatedURLLoader for compile to work on
windows.")

[commit-bot: I haz the power, 2017-04-24 06:28:53]

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-04-24 08:54:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493015314517270,
"parent_rev": "af5b63b8f7f30e4dcbada7d2b756c68fa49157a2", "commit_rev":
"fd61327a91c26467fae63304cc1ee343405344e6"}

[commit-bot: I haz the power, 2017-04-24 08:54:28]

Description was changed from

==========
Move WebAssociatedURLLoaderImpl.* from web/ -> core/export/.

WebAssociatedURLLoaderImpl had a dependency on WebLocalFrameImpl just so it
could
call a method to obtain the document for the frame. To break this dependency the
ctor parameter was changed to take a Document* directly.

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

to

==========
Move WebAssociatedURLLoaderImpl.* from web/ -> core/export/.

WebAssociatedURLLoaderImpl had a dependency on WebLocalFrameImpl just so it
could
call a method to obtain the document for the frame. To break this dependency the
ctor parameter was changed to take a Document* directly.

BUG=708879

Review-Url: https://codereview.chromium.org/2831833002
Cr-Commit-Position: refs/heads/master@{#466601}
Committed:
https://chromium.googlesource.com/chromium/src/+/fd61327a91c26467fae63304cc1e...
==========

[commit-bot: I haz the power, 2017-04-24 08:54:30]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/fd61327a91c26467fae63304cc1e...