Refactor UniqueNameHelper to use an adapter pattern for code sharing.

Refactor UniqueNameHelper to use an adapter pattern for code sharing.

This currently lives in //content/renderer and uses blink::WebFrame
directly. However, ExplodedFrameState will also need to do unique
name generation in the future when migrating unique name schemes.
Since ExplodedFrameState is in //content/common and can't reference
non-POD Blink types, this CL changes UniqueNameHelper to factor the
blink::WebFrame* specific logic behind an adapter interface.

The trickier part of this is that the adapter interface can't easily
provide lower-level concepts like Parent(), FirstSibling(), etc to
walk the frame tree: the adapter interface is virtual, so it can't
simply manufacture on-the-fly by-value wrappers for the return
values of these methods--the storage would need to be owned by each
RenderFrameImpl *and* RenderFrameProxy.

Instead, the adapter provides higher-level concepts. This will come
in useful in followup patches where methods like GetFramePosition()
can be extracted from ExplodedFrameState's generated unique name
rather than walking ExplodedFrameState's and assuming the ordering
matches DOM insertion order, etc.

This also allows some internal cleanup in UniqueNameHelper to more
cleanly handle the 'create new child frame' case.

BUG=626202, 645123
Review-Url: https://codereview.chromium.org/2902253003
Cr-Commit-Position: refs/heads/master@{#474839}
Committed: https://chromium.googlesource.com/chromium/src/+/61b2c92224f1bb6db51b0d7b9a84a08dd451f658

[dcheng, 2017-05-25 10:54:19]

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

[commit-bot: I haz the power, 2017-05-25 10:54:33]

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-25 12:16:11]

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

[commit-bot: I haz the power, 2017-05-25 12:16:11]

Dry run: 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-25 15:30:49]

Description was changed from

==========
Refactor UniqueNameHelper to use an adapter pattern for code sharing.

This code will be used by ExplodedFrameState in //content/common and
RenderFrameImpl in //content/renderer. To ensure the various unique
name generation bits are always in sync, factor RenderFrameImpl
dependencies to be hidden behind an adapter.

This also allows some internal cleanup in UniqueNameHelper to more
cleanly handle the 'create new child frame' case.

BUG=626202,645123
==========

to

==========
Refactor UniqueNameHelper to use an adapter pattern for code sharing.

This currently lives in //content/renderer and uses blink::WebFrame
directly. However, ExplodedFrameState will also need to do unique
name generation in the future when migrating unique name schemes.
Since ExplodedFrameState is in //content/common and can't reference
non-POD Blink types, this CL changes UniqueNameHelper to factor the
blink::WebFrame* specific logic behind an adapter interface.

The trickier part of this is that the adapter interface can't
provide lower-level concepts like Parent(), FirstSibling(), etc to
walk the frame tree: the adapter interface is virtual, so values
returned by lower-level methods like this

Instead, the adapter provides higher-level concepts. This will come
in useful in followup patches where methods like GetFramePosition()
can be extracted from ExplodedFrameState's generated unique name
rather than walking ExplodedFrameState's and assuming the ordering
matches DOM insertion order, etc.

This also allows some internal cleanup in UniqueNameHelper to more
cleanly handle the 'create new child frame' case.

BUG=626202,645123
==========

[dcheng, 2017-05-25 15:30:49]

dcheng@chromium.org changed reviewers:
+ creis@chromium.org, lukasza@chromium.org

[dcheng, 2017-05-25 15:38:34]

Description was changed from

==========
Refactor UniqueNameHelper to use an adapter pattern for code sharing.

This currently lives in //content/renderer and uses blink::WebFrame
directly. However, ExplodedFrameState will also need to do unique
name generation in the future when migrating unique name schemes.
Since ExplodedFrameState is in //content/common and can't reference
non-POD Blink types, this CL changes UniqueNameHelper to factor the
blink::WebFrame* specific logic behind an adapter interface.

The trickier part of this is that the adapter interface can't
provide lower-level concepts like Parent(), FirstSibling(), etc to
walk the frame tree: the adapter interface is virtual, so values
returned by lower-level methods like this

Instead, the adapter provides higher-level concepts. This will come
in useful in followup patches where methods like GetFramePosition()
can be extracted from ExplodedFrameState's generated unique name
rather than walking ExplodedFrameState's and assuming the ordering
matches DOM insertion order, etc.

This also allows some internal cleanup in UniqueNameHelper to more
cleanly handle the 'create new child frame' case.

BUG=626202,645123
==========

to

==========
Refactor UniqueNameHelper to use an adapter pattern for code sharing.

This currently lives in //content/renderer and uses blink::WebFrame
directly. However, ExplodedFrameState will also need to do unique
name generation in the future when migrating unique name schemes.
Since ExplodedFrameState is in //content/common and can't reference
non-POD Blink types, this CL changes UniqueNameHelper to factor the
blink::WebFrame* specific logic behind an adapter interface.

The trickier part of this is that the adapter interface can't easily
provide lower-level concepts like Parent(), FirstSibling(), etc to
walk the frame tree: the adapter interface is virtual, so it can't
simply manufacture on-the-fly by-value wrappers for the return
values of these methods--the storage would need to be owned by each
RenderFrameImpl *and* RenderFrameProxy.

Instead, the adapter provides higher-level concepts. This will come
in useful in followup patches where methods like GetFramePosition()
can be extracted from ExplodedFrameState's generated unique name
rather than walking ExplodedFrameState's and assuming the ordering
matches DOM insertion order, etc.

This also allows some internal cleanup in UniqueNameHelper to more
cleanly handle the 'create new child frame' case.

BUG=626202,645123
==========

[dcheng, 2017-05-25 15:39:16]

This CL should be pretty straightforward; it's just pulling the logic that
depends on blink::WebFrame out of unique name helper, so we can move unique name
helper into common to be used by frame state serialization.

This should make the followup CLs fairly straightforward to review (and merge if
needed)

[Łukasz Anforowicz, 2017-05-25 16:48:50]

Just some minor comments below...

https://codereview.chromium.org/2902253003/diff/20001/content/common/unique_n...
File content/common/unique_name_helper.h (right):

https://codereview.chromium.org/2902253003/diff/20001/content/common/unique_n...
content/common/unique_name_helper.h:126: // egg problem, this method is designed
to be call on the *parent* frame of
nit: s/call/called/ ?

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:934: if (frame == GetWebFrame())
I wonder if this check means that we can avoid the call to
|unique_name_.clear();| in UniqueNameHelper::UpdateName.

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:978: std::reverse(result.begin(),
result.end());
Will the other implementation (for exploded page state) also have to reverse? 
If yes, then maybe reversing should be done inside unique_name_helper.cc.

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3243: DCHECK_EQ(frame_, parent);
Does that mean that the |parent| parameter can be removed?

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:1191: RenderFrameAdapter
render_frame_adapter_;
Optional: Have you considered hiding the RenderFrameAdapter class in an
anonymous namespace inside render_frame_impl.cc?  (and changing
RenderFrameAdapter type below into
std::unique_ptr<UniqueNameHelper::FrameAdapter>).

[dcheng, 2017-05-25 18:24:56]

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

[dcheng, 2017-05-25 18:25:03]

https://codereview.chromium.org/2902253003/diff/20001/content/common/unique_n...
File content/common/unique_name_helper.h (right):

https://codereview.chromium.org/2902253003/diff/20001/content/common/unique_n...
content/common/unique_name_helper.h:126: // egg problem, this method is designed
to be call on the *parent* frame of
On 2017/05/25 16:48:49, Łukasz A. wrote:
> nit: s/call/called/ ?

Done.

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:934: if (frame == GetWebFrame())
On 2017/05/25 16:48:50, Łukasz A. wrote:
> I wonder if this check means that we can avoid the call to
> |unique_name_.clear();| in UniqueNameHelper::UpdateName.

This doesn't work very well as it turns out: since the PendingChildFrameAdapter
delegates to the parent's ChildFrameAdapter, it'll ignore the wrong frame.

I could add an extra parameter, but just doing the clear() seems simpler in this
case.

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:978: std::reverse(result.begin(),
result.end());
On 2017/05/25 16:48:50, Łukasz A. wrote:
> Will the other implementation (for exploded page state) also have to reverse? 
> If yes, then maybe reversing should be done inside unique_name_helper.cc.

I was going to do this in the followup, but doing it now seems fine as well.

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3243: DCHECK_EQ(frame_, parent);
On 2017/05/25 16:48:50, Łukasz A. wrote:
> Does that mean that the |parent| parameter can be removed?

Likely. But out of scope of this patch.

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:1191: RenderFrameAdapter
render_frame_adapter_;
On 2017/05/25 16:48:50, Łukasz A. wrote:
> Optional: Have you considered hiding the RenderFrameAdapter class in an
> anonymous namespace inside render_frame_impl.cc?  (and changing
> RenderFrameAdapter type below into
> std::unique_ptr<UniqueNameHelper::FrameAdapter>).

I considered that. While it hides the subclass definition, it introduces an
unnecessary heap indirection. Internal Google style is to avoid using forward
declares just to avoid including a full definition.

One option would be to move it to a separate file, but then I really need to
find a better name =)

[commit-bot: I haz the power, 2017-05-25 18:25:29]

Dry run: CQ is trying da patch.

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

[Łukasz Anforowicz, 2017-05-25 18:33:18]

https://codereview.chromium.org/2902253003/diff/20001/content/common/unique_n...
File content/common/unique_name_helper.cc (right):

https://codereview.chromium.org/2902253003/diff/20001/content/common/unique_n...
content/common/unique_name_helper.cc:159: unique_name_.clear();
I thought you said (*) that we still need to clear the old name.  Is the
deletion of the 3 lines above intentional?

(*)
https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:934: if (frame == GetWebFrame())
On 2017/05/25 18:25:03, dcheng wrote:
> On 2017/05/25 16:48:50, Łukasz A. wrote:
> > I wonder if this check means that we can avoid the call to
> > |unique_name_.clear();| in UniqueNameHelper::UpdateName.
> 
> This doesn't work very well as it turns out: since the
PendingChildFrameAdapter
> delegates to the parent's ChildFrameAdapter, it'll ignore the wrong frame.
> 
> I could add an extra parameter, but just doing the clear() seems simpler in
this
> case.

Acknowledged.

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:1191: RenderFrameAdapter
render_frame_adapter_;
On 2017/05/25 18:25:03, dcheng wrote:
> On 2017/05/25 16:48:50, Łukasz A. wrote:
> > Optional: Have you considered hiding the RenderFrameAdapter class in an
> > anonymous namespace inside render_frame_impl.cc?  (and changing
> > RenderFrameAdapter type below into
> > std::unique_ptr<UniqueNameHelper::FrameAdapter>).
> 
> I considered that. While it hides the subclass definition, it introduces an
> unnecessary heap indirection. Internal Google style is to avoid using forward
> declares just to avoid including a full definition.
> 
> One option would be to move it to a separate file, but then I really need to
> find a better name =)

FWIW, the current name is not that bad IMO.  If you want to change it, then
maybe UniqueNameFrameAdapter (since the "Render" part is implied by having the
class nested within RenderFrame)?

[dcheng, 2017-05-25 18:40:41]

https://codereview.chromium.org/2902253003/diff/20001/content/common/unique_n...
File content/common/unique_name_helper.cc (right):

https://codereview.chromium.org/2902253003/diff/20001/content/common/unique_n...
content/common/unique_name_helper.cc:159: unique_name_.clear();
On 2017/05/25 18:33:18, Łukasz A. wrote:
> I thought you said (*) that we still need to clear the old name.  Is the
> deletion of the 3 lines above intentional?
> 
> (*)
>
https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...

Doh, forgot to revert the unstaged changes when I used git commit -p. Thanks for
catching this.

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2902253003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:1191: RenderFrameAdapter
render_frame_adapter_;
On 2017/05/25 18:33:18, Łukasz A. wrote:
> On 2017/05/25 18:25:03, dcheng wrote:
> > On 2017/05/25 16:48:50, Łukasz A. wrote:
> > > Optional: Have you considered hiding the RenderFrameAdapter class in an
> > > anonymous namespace inside render_frame_impl.cc?  (and changing
> > > RenderFrameAdapter type below into
> > > std::unique_ptr<UniqueNameHelper::FrameAdapter>).
> > 
> > I considered that. While it hides the subclass definition, it introduces an
> > unnecessary heap indirection. Internal Google style is to avoid using
forward
> > declares just to avoid including a full definition.
> > 
> > One option would be to move it to a separate file, but then I really need to
> > find a better name =)
> 
> FWIW, the current name is not that bad IMO.  If you want to change it, then
> maybe UniqueNameFrameAdapter (since the "Render" part is implied by having the
> class nested within RenderFrame)?

Done.

[Łukasz Anforowicz, 2017-05-25 18:50:33]

Thanks - lgtm.

[dcheng, 2017-05-25 18:53:06]

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

[commit-bot: I haz the power, 2017-05-25 18:54:00]

Dry run: CQ is trying da patch.

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

[Charlie Reis, 2017-05-25 20:34:25]

LGTM!

https://codereview.chromium.org/2902253003/diff/60001/content/common/unique_n...
File content/common/unique_name_helper.cc (right):

https://codereview.chromium.org/2902253003/diff/60001/content/common/unique_n...
content/common/unique_name_helper.cc:28: int GetSiblingCount() const override {
return parent_->GetChildCount(); }
Might be worth reiterating that this pending child hasn't been added to its
parent's child count yet, since that confused me at first.

https://codereview.chromium.org/2902253003/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2902253003/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.h:1190: UniqueNameFrameAdapter
render_frame_adapter_;
nit: unique_name_adapter_?  Or unique_name_frame_adapter_?

(Within RenderFrameImpl, render_frame_adapter_ sounds like it could be used for
something more general.)

[commit-bot: I haz the power, 2017-05-25 20:39:28]

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

[commit-bot: I haz the power, 2017-05-25 20:39:29]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-25 21:10:44]

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

[dcheng, 2017-05-25 21:11:16]

https://codereview.chromium.org/2902253003/diff/60001/content/common/unique_n...
File content/common/unique_name_helper.cc (right):

https://codereview.chromium.org/2902253003/diff/60001/content/common/unique_n...
content/common/unique_name_helper.cc:28: int GetSiblingCount() const override {
return parent_->GetChildCount(); }
On 2017/05/25 20:34:25, Charlie Reis (overloaded) wrote:
> Might be worth reiterating that this pending child hasn't been added to its
> parent's child count yet, since that confused me at first.

Done. I tweaked a few comments related to this to make it clearer overall (I
hope)

https://codereview.chromium.org/2902253003/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2902253003/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.h:1190: UniqueNameFrameAdapter
render_frame_adapter_;
On 2017/05/25 20:34:25, Charlie Reis (overloaded) wrote:
> nit: unique_name_adapter_?  Or unique_name_frame_adapter_?
> 
> (Within RenderFrameImpl, render_frame_adapter_ sounds like it could be used
for
> something more general.)

Done.

[commit-bot: I haz the power, 2017-05-25 21:11:38]

Dry run: CQ is trying da patch.

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

[dcheng, 2017-05-25 21:26:29]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2017-05-25 21:26:29]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-05-25 21:26:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2902253003/#ps100001
(title: "Fix off by one bug")

[commit-bot: I haz the power, 2017-05-25 21:26:45]

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-25 23:10:16]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495747589772380,
"parent_rev": "9e244b6ad9c1c3f073a93274f6f6f83adef322cb", "commit_rev":
"61b2c92224f1bb6db51b0d7b9a84a08dd451f658"}

[commit-bot: I haz the power, 2017-05-25 23:10:27]

Description was changed from

==========
Refactor UniqueNameHelper to use an adapter pattern for code sharing.

This currently lives in //content/renderer and uses blink::WebFrame
directly. However, ExplodedFrameState will also need to do unique
name generation in the future when migrating unique name schemes.
Since ExplodedFrameState is in //content/common and can't reference
non-POD Blink types, this CL changes UniqueNameHelper to factor the
blink::WebFrame* specific logic behind an adapter interface.

The trickier part of this is that the adapter interface can't easily
provide lower-level concepts like Parent(), FirstSibling(), etc to
walk the frame tree: the adapter interface is virtual, so it can't
simply manufacture on-the-fly by-value wrappers for the return
values of these methods--the storage would need to be owned by each
RenderFrameImpl *and* RenderFrameProxy.

Instead, the adapter provides higher-level concepts. This will come
in useful in followup patches where methods like GetFramePosition()
can be extracted from ExplodedFrameState's generated unique name
rather than walking ExplodedFrameState's and assuming the ordering
matches DOM insertion order, etc.

This also allows some internal cleanup in UniqueNameHelper to more
cleanly handle the 'create new child frame' case.

BUG=626202,645123
==========

to

==========
Refactor UniqueNameHelper to use an adapter pattern for code sharing.

This currently lives in //content/renderer and uses blink::WebFrame
directly. However, ExplodedFrameState will also need to do unique
name generation in the future when migrating unique name schemes.
Since ExplodedFrameState is in //content/common and can't reference
non-POD Blink types, this CL changes UniqueNameHelper to factor the
blink::WebFrame* specific logic behind an adapter interface.

The trickier part of this is that the adapter interface can't easily
provide lower-level concepts like Parent(), FirstSibling(), etc to
walk the frame tree: the adapter interface is virtual, so it can't
simply manufacture on-the-fly by-value wrappers for the return
values of these methods--the storage would need to be owned by each
RenderFrameImpl *and* RenderFrameProxy.

Instead, the adapter provides higher-level concepts. This will come
in useful in followup patches where methods like GetFramePosition()
can be extracted from ExplodedFrameState's generated unique name
rather than walking ExplodedFrameState's and assuming the ordering
matches DOM insertion order, etc.

This also allows some internal cleanup in UniqueNameHelper to more
cleanly handle the 'create new child frame' case.

BUG=626202,645123

Review-Url: https://codereview.chromium.org/2902253003
Cr-Commit-Position: refs/heads/master@{#474839}
Committed:
https://chromium.googlesource.com/chromium/src/+/61b2c92224f1bb6db51b0d7b9a84...
==========

[commit-bot: I haz the power, 2017-05-25 23:10:28]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/61b2c92224f1bb6db51b0d7b9a84...