Use LocalFrame::client() than getting the FrameLoader and then call client() on it.

Use LocalFrame::client() than getting the FrameLoader and then call
client() on it.

FrameLoader::client() is just calling m_frame->client().

R=yhirano@chromium.org
BUG=671533
Review-Url: https://codereview.chromium.org/2557053002
Cr-Commit-Position: refs/heads/master@{#444332}
Committed: https://chromium.googlesource.com/chromium/src/+/ad61664891105622dd6a46201f97bec138f00f4c

[tyoshino (SeeGerritForStatus), 2016-12-07 08:51:24]

Description was changed from

==========
a

BUG=
==========

to

==========
Use LocalFrame::client() than getting the FrameLoader and then call
client() on it.

FrameLoader::client() is just calling m_frame->client().

BUG=671533
==========

[tyoshino (SeeGerritForStatus), 2016-12-07 10:25:16]

Description was changed from

==========
Use LocalFrame::client() than getting the FrameLoader and then call
client() on it.

FrameLoader::client() is just calling m_frame->client().

BUG=671533
==========

to

==========
Use LocalFrame::client() than getting the FrameLoader and then call
client() on it.

FrameLoader::client() is just calling m_frame->client().

BUG=671533
==========

[tyoshino (SeeGerritForStatus), 2016-12-07 10:29:44]

tyoshino@chromium.org changed reviewers:
+ yhirano@chromium.org

[tyoshino (SeeGerritForStatus), 2016-12-07 10:29:51]

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

[commit-bot: I haz the power, 2016-12-07 10:30:02]

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, 2016-12-07 12:11:01]

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

[commit-bot: I haz the power, 2016-12-07 12:11:01]

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

[yhirano, 2016-12-12 05:59:54]

On 2016/12/07 10:29:47, tyoshino wrote:

LocalFrame::client() returns a FrameLoaderClient pointer. Is loaderClient a
better method name?

[tyoshino (SeeGerritForStatus), 2016-12-12 07:03:43]

On 2016/12/12 05:59:54, yhirano wrote:
> On 2016/12/07 10:29:47, tyoshino wrote:
> 
> LocalFrame::client() returns a FrameLoaderClient pointer. Is loaderClient a
> better method name?

Sounds good. But can I do it in a separate CL? I need to change 7 more files to
do that.

[yhirano, 2016-12-12 07:05:27]

On 2016/12/12 07:03:43, tyoshino wrote:
> On 2016/12/12 05:59:54, yhirano wrote:
> > On 2016/12/07 10:29:47, tyoshino wrote:
> > 
> > LocalFrame::client() returns a FrameLoaderClient pointer. Is loaderClient a
> > better method name?
> 
> Sounds good. But can I do it in a separate CL? I need to change 7 more files
to
> do that.

OK, lgtm

[dcheng, 2016-12-12 07:10:11]

On 2016/12/12 07:05:27, yhirano wrote:
> On 2016/12/12 07:03:43, tyoshino wrote:
> > On 2016/12/12 05:59:54, yhirano wrote:
> > > On 2016/12/07 10:29:47, tyoshino wrote:
> > > 
> > > LocalFrame::client() returns a FrameLoaderClient pointer. Is loaderClient
a
> > > better method name?
> > 
> > Sounds good. But can I do it in a separate CL? I need to change 7 more files
> to
> > do that.
> 
> OK, lgtm

Note that it was intentional at the time I wrote this: LocalFrame::client() is
an overload of Frame::client() and intentionally takes advantage of the fact
that a more-derived class can return a covariant type. FrameLoaderClient also
has many thing that are not loading related at all; in fact, I was thinking of
renaming it LocalFrameClient.

[yhirano, 2016-12-12 07:13:31]

On 2016/12/12 07:10:11, dcheng wrote:
> On 2016/12/12 07:05:27, yhirano wrote:
> > On 2016/12/12 07:03:43, tyoshino wrote:
> > > On 2016/12/12 05:59:54, yhirano wrote:
> > > > On 2016/12/07 10:29:47, tyoshino wrote:
> > > > 
> > > > LocalFrame::client() returns a FrameLoaderClient pointer. Is
loaderClient
> a
> > > > better method name?
> > > 
> > > Sounds good. But can I do it in a separate CL? I need to change 7 more
files
> > to
> > > do that.
> > 
> > OK, lgtm
> 
> Note that it was intentional at the time I wrote this: LocalFrame::client() is
> an overload of Frame::client() and intentionally takes advantage of the fact
> that a more-derived class can return a covariant type. FrameLoaderClient also
> has many thing that are not loading related at all; in fact, I was thinking of
> renaming it LocalFrameClient.

That sounds good, too. What I'm not comfortable with is the inconsistency
between names.

[dcheng, 2016-12-12 07:23:21]

On 2016/12/12 07:13:31, yhirano wrote:
> On 2016/12/12 07:10:11, dcheng wrote:
> > On 2016/12/12 07:05:27, yhirano wrote:
> > > On 2016/12/12 07:03:43, tyoshino wrote:
> > > > On 2016/12/12 05:59:54, yhirano wrote:
> > > > > On 2016/12/07 10:29:47, tyoshino wrote:
> > > > > 
> > > > > LocalFrame::client() returns a FrameLoaderClient pointer. Is
> loaderClient
> > a
> > > > > better method name?
> > > > 
> > > > Sounds good. But can I do it in a separate CL? I need to change 7 more
> files
> > > to
> > > > do that.
> > > 
> > > OK, lgtm
> > 
> > Note that it was intentional at the time I wrote this: LocalFrame::client()
is
> > an overload of Frame::client() and intentionally takes advantage of the fact
> > that a more-derived class can return a covariant type. FrameLoaderClient
also
> > has many thing that are not loading related at all; in fact, I was thinking
of
> > renaming it LocalFrameClient.
> 
> That sounds good, too. What I'm not comfortable with is the inconsistency
> between names.

In general, I tried to structure Frame/DOMWindow/FrameClient so that the naming
convention is uniform, but the accessors return the local/remote type as
appropriate.

For example, that means:
LocalFrame::client() => returns FrameLoaderCLient
RemoteFrame::client() => returns RemoteFrameClient

LocalDOMWindow::frame() => returns LocalFrame
RemoteDOMWindow::frame() => returns RemoteFrame

Document::frame() => returns LocalFrame

(Note that LocalFrame::domWindow() and RemoteFrame::domWindow() don't currently
do this, because I ran into an #include cycle, but it's something I'd like to
try to fix...)

The goal is for code that handles local frames and remote frames to look
similar: in general, I try to discourage explicit checks for
frame->isLocalFrame(), etc, since it usually means that the right abstraction
isn't being used. My worry about adding a distinct getter is that it might
encourage the opposite.

[tyoshino (SeeGerritForStatus), 2016-12-12 07:23:44]

On 2016/12/12 07:13:31, yhirano wrote:
> On 2016/12/12 07:10:11, dcheng wrote:
> > On 2016/12/12 07:05:27, yhirano wrote:
> > > On 2016/12/12 07:03:43, tyoshino wrote:
> > > > On 2016/12/12 05:59:54, yhirano wrote:
> > > > > On 2016/12/07 10:29:47, tyoshino wrote:
> > > > > 
> > > > > LocalFrame::client() returns a FrameLoaderClient pointer. Is
> loaderClient
> > a
> > > > > better method name?
> > > > 
> > > > Sounds good. But can I do it in a separate CL? I need to change 7 more
> files
> > > to
> > > > do that.
> > > 
> > > OK, lgtm
> > 
> > Note that it was intentional at the time I wrote this: LocalFrame::client()
is
> > an overload of Frame::client() and intentionally takes advantage of the fact
> > that a more-derived class can return a covariant type. FrameLoaderClient
also
> > has many thing that are not loading related at all; in fact, I was thinking
of
> > renaming it LocalFrameClient.
> 
> That sounds good, too. What I'm not comfortable with is the inconsistency
> between names.

Thanks dcheng for the info. So, with hope that the inconsistency will be
resolved at some point, I'll just commit this.

[tyoshino (SeeGerritForStatus), 2016-12-12 07:33:03]

On 2016/12/12 07:23:21, dcheng wrote:
> On 2016/12/12 07:13:31, yhirano wrote:
> > On 2016/12/12 07:10:11, dcheng wrote:
> > > On 2016/12/12 07:05:27, yhirano wrote:
> > > > On 2016/12/12 07:03:43, tyoshino wrote:
> > > > > On 2016/12/12 05:59:54, yhirano wrote:
> > > > > > On 2016/12/07 10:29:47, tyoshino wrote:
> > > > > > 
> > > > > > LocalFrame::client() returns a FrameLoaderClient pointer. Is
> > loaderClient
> > > a
> > > > > > better method name?
> > > > > 
> > > > > Sounds good. But can I do it in a separate CL? I need to change 7 more
> > files
> > > > to
> > > > > do that.
> > > > 
> > > > OK, lgtm
> > > 
> > > Note that it was intentional at the time I wrote this:
LocalFrame::client()
> is
> > > an overload of Frame::client() and intentionally takes advantage of the
fact
> > > that a more-derived class can return a covariant type. FrameLoaderClient
> also
> > > has many thing that are not loading related at all; in fact, I was
thinking
> of
> > > renaming it LocalFrameClient.
> > 
> > That sounds good, too. What I'm not comfortable with is the inconsistency
> > between names.
> 
> In general, I tried to structure Frame/DOMWindow/FrameClient so that the
naming
> convention is uniform, but the accessors return the local/remote type as
> appropriate.
> 
> For example, that means:
> LocalFrame::client() => returns FrameLoaderCLient
> RemoteFrame::client() => returns RemoteFrameClient
> 
> LocalDOMWindow::frame() => returns LocalFrame
> RemoteDOMWindow::frame() => returns RemoteFrame
> 
> Document::frame() => returns LocalFrame
> 
> (Note that LocalFrame::domWindow() and RemoteFrame::domWindow() don't
currently
> do this, because I ran into an #include cycle, but it's something I'd like to
> try to fix...)
> 
> The goal is for code that handles local frames and remote frames to look
> similar: in general, I try to discourage explicit checks for
> frame->isLocalFrame(), etc, since it usually means that the right abstraction
> isn't being used. My worry about adding a distinct getter is that it might
> encourage the opposite.

Got it. I realized that, though, here mainly FrameLoaderClient specific methods
are called. So, inside FrameFetchContext, it might make sense that we use some
name including loader. I'll add a method named frameLoaderClient() to
FrameLoaderClient and use frame()->client() inside it. Does it make sense?

[tyoshino (SeeGerritForStatus), 2016-12-12 07:53:06]

Replaced most of the occurrences with frameLoaderClient() call. Code length
reduction became small, but it should be still useful for making code reading on
codesearch easier.

[tyoshino (SeeGerritForStatus), 2017-01-11 07:39:46]

Description was changed from

==========
Use LocalFrame::client() than getting the FrameLoader and then call
client() on it.

FrameLoader::client() is just calling m_frame->client().

BUG=671533
==========

to

==========
Use LocalFrame::client() than getting the FrameLoader and then call
client() on it.

FrameLoader::client() is just calling m_frame->client().

R=yhirano@chromium.org
BUG=671533
==========

[tyoshino (SeeGerritForStatus), 2017-01-18 09:38:02]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2017-01-18 09:38:03]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2557053002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-18 09:38:30]

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-01-18 11:17:28]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484732282767890,
"parent_rev": "99ebee25a25e308296e1a9b70d450ae0daaafdb4", "commit_rev":
"ad61664891105622dd6a46201f97bec138f00f4c"}

[commit-bot: I haz the power, 2017-01-18 11:17:57]

Description was changed from

==========
Use LocalFrame::client() than getting the FrameLoader and then call
client() on it.

FrameLoader::client() is just calling m_frame->client().

R=yhirano@chromium.org
BUG=671533
==========

to

==========
Use LocalFrame::client() than getting the FrameLoader and then call
client() on it.

FrameLoader::client() is just calling m_frame->client().

R=yhirano@chromium.org
BUG=671533

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

[commit-bot: I haz the power, 2017-01-18 11:17:58]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ad61664891105622dd6a46201f97...