Replace LayoutPart::GetFrameViewBase with GetNodeFrameView

Replace LayoutPart::GetFrameViewBase with ChildFrameView

GetChildFrameView returns FrameView associated with local Node
if node is HTMLFrameOwnerElement or nullptr.

The method name GetFrameView is already declared in
LayoutObject which returns Document().View(), so a different
name was required.

BUG=637460
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2810873007
Cr-Commit-Position: refs/heads/master@{#464325}
Committed: https://chromium.googlesource.com/chromium/src/+/5d84a680108877c6a9c526b3e0711913a85245e5

[joelhockey, 2017-04-12 07:08:34]

Description was changed from

==========
Replace LayoutPart::GetFrameViewBase with GetNodeFrameView

GetNodeFrameView returns FrameView associated with local Node
if node is HTMLFrameOwnerElement or nullptr.

The method name GetFrameView is already declared in
LayoutObject which returns Document().View(), so a different
name was required.

BUG=637460
==========

to

==========
Replace LayoutPart::GetFrameViewBase with GetNodeFrameView

GetNodeFrameView returns FrameView associated with local Node
if node is HTMLFrameOwnerElement or nullptr.

The method name GetFrameView is already declared in
LayoutObject which returns Document().View(), so a different
name was required.

BUG=637460
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[joelhockey, 2017-04-12 07:11:38]

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

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

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

[joelhockey, 2017-04-12 07:14:59]

joelhockey@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org

[haraken, 2017-04-12 07:21:01]

https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutPart.h (right):

https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutPart.h:55: // the FrameView
associated with the root Document Frame.

I'm wondering when the two FrameViews can differ.

dcheng@: Do you have any insight?

[commit-bot: I haz the power, 2017-04-12 08:57:46]

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

[commit-bot: I haz the power, 2017-04-12 08:57:46]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-04-12 23:44:37]

The CL LGTM, but I would recommend trying to think of a less ambiguous name.
"NodeFrameView" doesn't make it nearly clear enough.

Maybe GetChildFrameFrameView() or something?

https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutPart.cpp:199: FrameView*
child_frame_view = GetNodeFrameView();
Can we just use frame_view directly here? It seems like this is the same as
frame_view on line 182?

https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutPart.h (right):

https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutPart.h:55: // the FrameView
associated with the root Document Frame.
On 2017/04/12 07:21:01, haraken wrote:
> 
> I'm wondering when the two FrameViews can differ.
> 
> dcheng@: Do you have any insight?

The difference is kind of subtle, but my understanding is:

LayoutObject::GetFrameView -> returns the containing Document's Frame's
FrameView. This is the frame that the LayoutObject is in, i.e. the parent frame.

LayoutPart::GetNodeFrameView -> returns the contained Document's Frame's
FrameView. This is the FrameView for the child frame.

[haraken, 2017-04-12 23:48:21]

On 2017/04/12 23:44:37, dcheng wrote:
> The CL LGTM, but I would recommend trying to think of a less ambiguous name.
> "NodeFrameView" doesn't make it nearly clear enough.
> 
> Maybe GetChildFrameFrameView() or something?
> 
>
https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right):
> 
>
https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutPart.cpp:199: FrameView*
> child_frame_view = GetNodeFrameView();
> Can we just use frame_view directly here? It seems like this is the same as
> frame_view on line 182?
> 
>
https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutPart.h (right):
> 
>
https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutPart.h:55: // the FrameView
> associated with the root Document Frame.
> On 2017/04/12 07:21:01, haraken wrote:
> > 
> > I'm wondering when the two FrameViews can differ.
> > 
> > dcheng@: Do you have any insight?
> 
> The difference is kind of subtle, but my understanding is:
> 
> LayoutObject::GetFrameView -> returns the containing Document's Frame's
> FrameView. This is the frame that the LayoutObject is in, i.e. the parent
frame.
> 
> LayoutPart::GetNodeFrameView -> returns the contained Document's Frame's
> FrameView. This is the FrameView for the child frame.

Ah, that makes sense. GetChildFrameView() sounds nice.

LGTM.

[joelhockey, 2017-04-13 00:21:12]

> > Maybe GetChildFrameFrameView() or something?
> Ah, that makes sense. GetChildFrameView() sounds nice.

I think from 'new blink style guide', we should omit the 'Get' (use bare words
for getters).  This just leaves either:
1/ ChildFrameFrameView
2/ ChildFrameView

I like 2.  dcheng, is that ok with you?

[dcheng, 2017-04-13 00:22:07]

On 2017/04/13 00:21:12, joelhockey wrote:
> > > Maybe GetChildFrameFrameView() or something?
> > Ah, that makes sense. GetChildFrameView() sounds nice.
> 
> I think from 'new blink style guide', we should omit the 'Get' (use bare words
> for getters).  This just leaves either:
> 1/ ChildFrameFrameView
> 2/ ChildFrameView
> 
> I like 2.  dcheng, is that ok with you?

SGTM

[dcheng, 2017-04-13 00:22:51]

On 2017/04/13 00:22:07, dcheng wrote:
> On 2017/04/13 00:21:12, joelhockey wrote:
> > > > Maybe GetChildFrameFrameView() or something?
> > > Ah, that makes sense. GetChildFrameView() sounds nice.
> > 
> > I think from 'new blink style guide', we should omit the 'Get' (use bare
words
> > for getters).  This just leaves either:
> > 1/ ChildFrameFrameView
> > 2/ ChildFrameView
> > 
> > I like 2.  dcheng, is that ok with you?
> 
> SGTM

FWIW, the rationale behind ChildFrameFrameView was that we want to get rid of
the widget hierarchy. So once that happens, it would be the frame view of the
child frame, not a child frame view. But I don't feel strongly about this.

[joelhockey, 2017-04-13 00:31:18]

Description was changed from

==========
Replace LayoutPart::GetFrameViewBase with GetNodeFrameView

GetNodeFrameView returns FrameView associated with local Node
if node is HTMLFrameOwnerElement or nullptr.

The method name GetFrameView is already declared in
LayoutObject which returns Document().View(), so a different
name was required.

BUG=637460
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace LayoutPart::GetFrameViewBase with GetChildFrameView

GetChildFrameView returns FrameView associated with local Node
if node is HTMLFrameOwnerElement or nullptr.

The method name GetFrameView is already declared in
LayoutObject which returns Document().View(), so a different
name was required.

BUG=637460
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[joelhockey, 2017-04-13 00:31:32]

The CQ bit was checked by joelhockey@chromium.org

[joelhockey, 2017-04-13 00:31:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2810873007/#ps20001
(title: "s/GetNodeFrameView/ChildFrameView")

[commit-bot: I haz the power, 2017-04-13 00:32:18]

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

[joelhockey, 2017-04-13 00:37:02]

https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/2810873007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutPart.cpp:199: FrameView*
child_frame_view = GetNodeFrameView();
On 2017/04/12 at 23:44:37, dcheng wrote:
> Can we just use frame_view directly here? It seems like this is the same as
frame_view on line 182?

Yes, I can't see how this value could change, so no need for child_frame_view,
just reuse frame_view.  I have also renamed other child_frame_view var to
frame_view for consistency within the file.

[joelhockey, 2017-04-13 00:39:57]

The CQ bit was checked by joelhockey@chromium.org

[joelhockey, 2017-04-13 00:39:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2810873007/#ps40001
(title: "No need for child_frame_view var")

[commit-bot: I haz the power, 2017-04-13 00:41:01]

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

[joelhockey, 2017-04-13 00:50:50]

Description was changed from

==========
Replace LayoutPart::GetFrameViewBase with GetChildFrameView

GetChildFrameView returns FrameView associated with local Node
if node is HTMLFrameOwnerElement or nullptr.

The method name GetFrameView is already declared in
LayoutObject which returns Document().View(), so a different
name was required.

BUG=637460
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace LayoutPart::GetFrameViewBase with ChildFrameView

GetChildFrameView returns FrameView associated with local Node
if node is HTMLFrameOwnerElement or nullptr.

The method name GetFrameView is already declared in
LayoutObject which returns Document().View(), so a different
name was required.

BUG=637460
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

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

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

[commit-bot: I haz the power, 2017-04-13 02:05:32]

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

[joelhockey, 2017-04-13 02:37:12]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-04-13 02:37: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-04-13 04:01:39]

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

[commit-bot: I haz the power, 2017-04-13 04:01:40]

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

[joelhockey, 2017-04-13 04:10:22]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-04-13 04:10:58]

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-13 05:34:52]

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

[commit-bot: I haz the power, 2017-04-13 05:34:55]

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

[joelhockey, 2017-04-13 05:39:28]

The CQ bit was checked by joelhockey@chromium.org

[commit-bot: I haz the power, 2017-04-13 05:39:51]

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-13 06:51:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492061968954740,
"parent_rev": "cf1686e9b3003b61af2c2d6e9793bfcb3627e9c9", "commit_rev":
"5d84a680108877c6a9c526b3e0711913a85245e5"}

[commit-bot: I haz the power, 2017-04-13 06:52:09]

Description was changed from

==========
Replace LayoutPart::GetFrameViewBase with ChildFrameView

GetChildFrameView returns FrameView associated with local Node
if node is HTMLFrameOwnerElement or nullptr.

The method name GetFrameView is already declared in
LayoutObject which returns Document().View(), so a different
name was required.

BUG=637460
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace LayoutPart::GetFrameViewBase with ChildFrameView

GetChildFrameView returns FrameView associated with local Node
if node is HTMLFrameOwnerElement or nullptr.

The method name GetFrameView is already declared in
LayoutObject which returns Document().View(), so a different
name was required.

BUG=637460
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2810873007
Cr-Commit-Position: refs/heads/master@{#464325}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d84a680108877c6a9c526b3e071...
==========

[commit-bot: I haz the power, 2017-04-13 06:52:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5d84a680108877c6a9c526b3e071...