Part 1 Of Renaming FrameLoaderClient to LocalFrameClient.

Part 1 Of Renaming FrameLoaderClient to LocalFrameClient.

*** This patch is a rename-only change; no logic changes. ***

This patch does the following:

a. Rename FrameLoaderClient related files to LocalFrameClient.
b. Create an alias for LocalFrameClient to FrameLoaderClient.
c. Where FrameLoaderClient was being forward declared, either
  i. Replace with alias to LocalFrameClient, or
  ii. Just Use LocalFrameClient directly where possible.

Subsequent patches will start moving usage of FrameLoaderClient to
LocalFrameClient in a set of logically associated change lists.

BUG=694773
Review-Url: https://codereview.chromium.org/2712033002
Cr-Commit-Position: refs/heads/master@{#452781}
Committed: https://chromium.googlesource.com/chromium/src/+/445e237611d315d10ba6f7d97b6f40fc5cb404de

[slangley, 2017-02-23 23:44:27]

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

[slangley, 2017-02-23 23:48:17]

Giving up on trying to push it through all at once, this is the first CL of
performing the rename is much much smaller patches.

[sashab, 2017-02-24 00:27:09]

How does this patch remove nearly 2000 lines? Does the rename really remove that
much functionality?

Maybe add a d. to the CL description, "remove files where xxx*

Also "declared" (sic) in CL description

Other than those nits, nice work! LGTM

https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:45: 
Is this needed? If it was done automatically by git cl format then its fine.

https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:73: using
RedirectStatus = ResourceRequest::RedirectStatus;
Do we prefer typedef or using? I'd check with dcheng, maybe it doesn't matter

[slangley, 2017-02-24 00:32:08]

Description was changed from

==========
Part 1 Of Renaming FrameLoaderClient to LocalFrameClient.

*** This patch is a rename-only change; no logic changes. ***

This patch does the following:

a. Rename FrameLoaderClient related files to LocalFrameClient.
b. Create an alias for LocalFrameClient to FrameLoaderClient.
c. Where FrameLoaderClient was being forward decalared, either
  i. Replace with alias to LocalFrameClient, or
  ii. Just Use LocalFrameClient directly where possible.

Subsequent patches will start moving usage of FrameLoaderClient to
LocalFrameClient in a set of logically associated change lists.

BUG=694773
==========

to

==========
Part 1 Of Renaming FrameLoaderClient to LocalFrameClient.

*** This patch is a rename-only change; no logic changes. ***

This patch does the following:

a. Rename FrameLoaderClient related files to LocalFrameClient.
b. Create an alias for LocalFrameClient to FrameLoaderClient.
c. Where FrameLoaderClient was being forward declared, either
  i. Replace with alias to LocalFrameClient, or
  ii. Just Use LocalFrameClient directly where possible.

Subsequent patches will start moving usage of FrameLoaderClient to
LocalFrameClient in a set of logically associated change lists.

BUG=694773
==========

[slangley, 2017-02-24 00:35:57]

On 2017/02/24 at 00:27:09, sashab wrote:
> How does this patch remove nearly 2000 lines? Does the rename really remove
that much functionality?
> 
> Maybe add a d. to the CL description, "remove files where xxx*
> 
> Also "declared" (sic) in CL description
> 
> Other than those nits, nice work! LGTM
> 
>
https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):
> 
>
https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:45: 
> Is this needed? If it was done automatically by git cl format then its fine.
> 
>
https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:73: using
RedirectStatus = ResourceRequest::RedirectStatus;
> Do we prefer typedef or using? I'd check with dcheng, maybe it doesn't matter

Thanks!


The deleted lines are by virtual of renaming the files and then adding the
alias, which is what I already tried to describe in the current patch
description.

[slangley, 2017-02-24 00:36:19]

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

[slangley, 2017-02-24 00:39:46]

https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:45: 
On 2017/02/24 at 00:27:09, sashab wrote:
> Is this needed? If it was done automatically by git cl format then its fine.

on the last cl dcheng@ said there should be vertical space between system and
user headers.

https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:73: using
RedirectStatus = ResourceRequest::RedirectStatus;
On 2017/02/24 at 00:27:09, sashab wrote:
> Do we prefer typedef or using? I'd check with dcheng, maybe it doesn't matter

typedef is how to alias types, right?

[sashab, 2017-02-24 00:44:32]

https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:45: 
On 2017/02/24 at 00:39:46, slangley wrote:
> On 2017/02/24 at 00:27:09, sashab wrote:
> > Is this needed? If it was done automatically by git cl format then its fine.
> 
> on the last cl dcheng@ said there should be vertical space between system and
user headers.

Cool.

https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:73: using
RedirectStatus = ResourceRequest::RedirectStatus;
On 2017/02/24 at 00:39:46, slangley wrote:
> On 2017/02/24 at 00:27:09, sashab wrote:
> > Do we prefer typedef or using? I'd check with dcheng, maybe it doesn't
matter
> 
> typedef is how to alias types, right?

You can also use "using", which I think is the C++ way (see the line below).
"typedef" is from C.

Our C++ website says:
https://chromium-cpp.appspot.com/
C++ 'using' keyword - "Use instead of typedef, unless the header needs to be
compatible with C."

So I'd say we should find and replace "typedef LocalFrameClient
FrameLoaderClient" with "using FrameLoaderClient = LocalFrameClient".

In this case, it does the exact same thing. Maybe there are plans for a global
find+replace later.

[slangley, 2017-02-24 01:34:37]

sashab@ ptal

changed from typedef to using.

also, TIL that you need to add new files to a patch set using "add" manually -
so the new files were not in previous patch sets. So they're now there as well,
which is nice.

[haraken, 2017-02-24 02:26:05]

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

[haraken, 2017-02-24 02:26:06]

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

https://codereview.chromium.org/2712033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:72: using
FrameLoaderClient = LocalFrameClient;

Yes, new code in Blink prefers using than typedef.

https://codereview.chromium.org/2712033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:72: using
FrameLoaderClient = LocalFrameClient;

It looks strange that this using statement is written in
ContentSecurityPolicy.h. Can we move this to FrameLoaderClient.h?

https://codereview.chromium.org/2712033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/2712033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.h:70: using
FrameLoaderClient = LocalFrameClient;

Avoid writing the using statement in multiple places.

[dcheng, 2017-02-24 02:32:36]

On 2017/02/24 00:35:57, slangley wrote:
> On 2017/02/24 at 00:27:09, sashab wrote:
> > How does this patch remove nearly 2000 lines? Does the rename really remove
> that much functionality?
> > 
> > Maybe add a d. to the CL description, "remove files where xxx*
> > 
> > Also "declared" (sic) in CL description
> > 
> > Other than those nits, nice work! LGTM
> > 
> >
>
https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h
(right):
> > 
> >
>
https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:45: 
> > Is this needed? If it was done automatically by git cl format then its fine.
> > 
> >
>
https://codereview.chromium.org/2712033002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:73: using
> RedirectStatus = ResourceRequest::RedirectStatus;
> > Do we prefer typedef or using? I'd check with dcheng, maybe it doesn't
matter
> 
> Thanks!
> 
> 
> The deleted lines are by virtual of renaming the files and then adding the
> alias, which is what I already tried to describe in the current patch
> description.

Specifically, what happens with a move is:
- all the lines in the removed file are counted as removed
- if git decides the new file is branched from the old file, then only the
changed lines will be counted as added

You can see this because LocalFrameClient.{cpp,h} is marked as A+, rather than
A.

+1 to what haraken@ said about using 'using' and moving it into
LocalFrameClient.h itself.

[slangley, 2017-02-24 02:55:06]

In all of the changes "using LocalFrameClient(Impl) = FrameLoaderClient(Impl)"
in this patch, these are forward declarations, so moving it to
LocalFrameClient.h and then having to #include LocalFrameClient.h doesn't seem
like a better solution than this.

Or am I misunderstanding your intention?

[haraken, 2017-02-24 03:13:20]

On 2017/02/24 02:55:06, slangley wrote:
> In all of the changes "using LocalFrameClient(Impl) = FrameLoaderClient(Impl)"
> in this patch, these are forward declarations, so moving it to
> LocalFrameClient.h and then having to #include LocalFrameClient.h doesn't seem
> like a better solution than this.
> 
> Or am I misunderstanding your intention?

Ah... you're right.

BTW why are you failing at landing the original giant CL? If it's just due to a
patch conflict, you can just rebase with ToT and try a couple of times.

[slangley, 2017-02-24 03:26:06]

It seems slicing the CL up and pushing through in small controlled changes
is the right thing to do, so I'm happy to toss the other CL in favor of
this approach.

On Fri, Feb 24, 2017 at 2:13 PM, <haraken@chromium.org> wrote:

> On 2017/02/24 02:55:06, slangley wrote:
> > In all of the changes "using LocalFrameClient(Impl) =
> FrameLoaderClient(Impl)"
> > in this patch, these are forward declarations, so moving it to
> > LocalFrameClient.h and then having to #include LocalFrameClient.h
> doesn't seem
> > like a better solution than this.
> >
> > Or am I misunderstanding your intention?
>
> Ah... you're right.
>
> BTW why are you failing at landing the original giant CL? If it's just due
> to a
> patch conflict, you can just rebase with ToT and try a couple of times.
>
>
>
> https://codereview.chromium.org/2712033002/
>

-- 
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-02-24 03:26:07]

It seems slicing the CL up and pushing through in small controlled changes
is the right thing to do, so I'm happy to toss the other CL in favor of
this approach.

On Fri, Feb 24, 2017 at 2:13 PM, <haraken@chromium.org> wrote:

> On 2017/02/24 02:55:06, slangley wrote:
> > In all of the changes "using LocalFrameClient(Impl) =
> FrameLoaderClient(Impl)"
> > in this patch, these are forward declarations, so moving it to
> > LocalFrameClient.h and then having to #include LocalFrameClient.h
> doesn't seem
> > like a better solution than this.
> >
> > Or am I misunderstanding your intention?
>
> Ah... you're right.
>
> BTW why are you failing at landing the original giant CL? If it's just due
> to a
> patch conflict, you can just rebase with ToT and try a couple of times.
>
>
>
> https://codereview.chromium.org/2712033002/
>

-- 
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-02-24 04:24:03]

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

[commit-bot: I haz the power, 2017-02-24 04:24:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-02-24 04:27:54]

On 2017/02/24 03:26:07, slangley wrote:
> It seems slicing the CL up and pushing through in small controlled changes
> is the right thing to do, so I'm happy to toss the other CL in favor of
> this approach.

In general, smaller patches are definitely preferred. However, it's not really
nice to add a bunch of TODOs in the code base.

Would it be possible to do something like this?

  // FrameLoaderClient should be removed.
  class LocalFrameClient : public FrameLoaderClient { };

[slangley, 2017-02-24 04:41:23]

So I just went ahead and changed all of the forward declarations to
LocalFrameClient and fixed the call sites. Resulted in a few more files being
added to the CL, but it's not as large as the other uber CL.

[slangley, 2017-02-24 04:41:35]

Sorry meant to add ptal.

[haraken, 2017-02-24 04:51:06]

On 2017/02/24 04:41:35, slangley wrote:
> Sorry meant to add ptal.

LGTM!

[slangley, 2017-02-24 05:15:02]

The CQ bit was checked by slangley@chromium.org

[slangley, 2017-02-24 05:15:03]

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/2712033002/#ps80001
(title: "Change all forward declarations of FrameLoaderClient to
LocalFrameClient and fix call sites.")

[commit-bot: I haz the power, 2017-02-24 05:15:20]

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

[dcheng, 2017-02-24 05:30:21]

LGTM

(I have some thoughts on naming, but this CL is fine as-is)

https://codereview.chromium.org/2712033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/2712033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.h:211: LocalFrameClient&
localFrameClient() const;
Minor naming nit: "local" is implied by "Document", so local is redundant.

(It's OK to commit as-is though)

https://codereview.chromium.org/2712033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.h (right):

https://codereview.chromium.org/2712033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.h:180: LocalFrameClient*
localFrameClient() const;
Ditto: fetch/loading implies local

https://codereview.chromium.org/2712033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ProgressTracker.h (right):

https://codereview.chromium.org/2712033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ProgressTracker.h:75: LocalFrameClient*
localFrameClient() const;
Ditto: the 'local' part is implied by progress tracking

[commit-bot: I haz the power, 2017-02-24 07:55:23]

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

[commit-bot: I haz the power, 2017-02-24 07:55:24]

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

[slangley, 2017-02-24 08:39:39]

The CQ bit was checked by slangley@chromium.org

[commit-bot: I haz the power, 2017-02-24 08:39: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-02-24 09:47:51]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487925579792700,
"parent_rev": "7e900589febbfc49da56f18b33750786c24ba14c", "commit_rev":
"445e237611d315d10ba6f7d97b6f40fc5cb404de"}

[commit-bot: I haz the power, 2017-02-24 09:48:52]

Description was changed from

==========
Part 1 Of Renaming FrameLoaderClient to LocalFrameClient.

*** This patch is a rename-only change; no logic changes. ***

This patch does the following:

a. Rename FrameLoaderClient related files to LocalFrameClient.
b. Create an alias for LocalFrameClient to FrameLoaderClient.
c. Where FrameLoaderClient was being forward declared, either
  i. Replace with alias to LocalFrameClient, or
  ii. Just Use LocalFrameClient directly where possible.

Subsequent patches will start moving usage of FrameLoaderClient to
LocalFrameClient in a set of logically associated change lists.

BUG=694773
==========

to

==========
Part 1 Of Renaming FrameLoaderClient to LocalFrameClient.

*** This patch is a rename-only change; no logic changes. ***

This patch does the following:

a. Rename FrameLoaderClient related files to LocalFrameClient.
b. Create an alias for LocalFrameClient to FrameLoaderClient.
c. Where FrameLoaderClient was being forward declared, either
  i. Replace with alias to LocalFrameClient, or
  ii. Just Use LocalFrameClient directly where possible.

Subsequent patches will start moving usage of FrameLoaderClient to
LocalFrameClient in a set of logically associated change lists.

BUG=694773

Review-Url: https://codereview.chromium.org/2712033002
Cr-Commit-Position: refs/heads/master@{#452781}
Committed:
https://chromium.googlesource.com/chromium/src/+/445e237611d315d10ba6f7d97b6f...
==========

[commit-bot: I haz the power, 2017-02-24 09:48:54]

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