Move unique name generation and tracking into //content.

Move unique name generation and tracking into //content.

Blink doesn't really consume this anymore, so move the
concept out of Blink and simplify the plumbing. Future
CLs will move it wholly into the browser process.

BUG=626202, 645123
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[dcheng, 2017-03-01 09:58:47]

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

[commit-bot: I haz the power, 2017-03-01 09:59:03]

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-03-01 10:02:34]

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

[commit-bot: I haz the power, 2017-03-01 10:02:35]

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

[dcheng, 2017-03-01 23:44:32]

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

[commit-bot: I haz the power, 2017-03-01 23:45:24]

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

[dcheng, 2017-03-01 23:47:33]

Description was changed from

==========
Move unique name generation and tracking into //content.
==========

to

==========
Move unique name generation and tracking into //content.

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

[dcheng, 2017-03-01 23:47:33]

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

[dcheng, 2017-03-01 23:48:06]

Description was changed from

==========
Move unique name generation and tracking into //content.

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

to

==========
Move unique name generation and tracking into //content.

Blink doesn't really consume this anymore, so move the
concept out of Blink and simplify the plumbing. Future
CLs will move it wholly into the browser process.

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

[dcheng, 2017-03-01 23:57:26]

https://codereview.chromium.org/2714943004/diff/90001/content/renderer/BUILD.gn
File content/renderer/BUILD.gn (right):

https://codereview.chromium.org/2714943004/diff/90001/content/renderer/BUILD....
content/renderer/BUILD.gn:28: "//content/shell/test_runner",
Note: I'm pretty sure this is wrong, but this is something I'm working on
addressing while I see how the trybots feel about the change.

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

https://codereview.chromium.org/2714943004/diff/90001/content/renderer/render...
content/renderer/render_frame_impl.cc:3631:
blink::WebString::fromUTF8(unique_name_helper_.value()));
target should move out of WebHistoryItem, but doing so is non-trivial, as test
runner dumps |target|.

https://codereview.chromium.org/2714943004/diff/90001/content/shell/test_runn...
File content/shell/test_runner/BUILD.gn (right):

https://codereview.chromium.org/2714943004/diff/90001/content/shell/test_runn...
content/shell/test_runner/BUILD.gn:125: "//ui/accessibility:ax_gen",
This is likely due to trying to depend on renderer.

https://codereview.chromium.org/2714943004/diff/90001/content/shell/test_runn...
File content/shell/test_runner/DEPS (right):

https://codereview.chromium.org/2714943004/diff/90001/content/shell/test_runn...
content/shell/test_runner/DEPS:3: "+content",
content_shell only depends on the content public API, as it's a "canonical
sample embedder". However, it seems like it should be OK for test_runner to do
so, as test_runner itself is not meant to be a canonical sample like
content_shell itself.

https://codereview.chromium.org/2714943004/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (left):

https://codereview.chromium.org/2714943004/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:9050:
TEST_F(WebFrameSwapTest, UniqueNameAfterRemoteToLocalSwap) {
Working on a browser test for this, since we can't really easily test this in a
renderer unit test anymore, since both RenderFrameProxy and RenderFrameImpl are
involved.

https://codereview.chromium.org/2714943004/diff/90001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2714943004/diff/90001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:183: const WebString&
fallbackName,
Note: I'm 95% sure this isn't actually needed: when creating a child frame, we
already set name to the fallback name. But this is already a pretty large
change, so I'll pursue this in followups.

[commit-bot: I haz the power, 2017-03-02 00:19:44]

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

[commit-bot: I haz the power, 2017-03-02 00:19:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dcheng, 2017-03-02 02:02:14]

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

[commit-bot: I haz the power, 2017-03-02 02:03:52]

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-03-02 02:57:19]

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

[commit-bot: I haz the power, 2017-03-02 02:57:20]

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

[dcheng, 2017-03-02 06:56:17]

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

[commit-bot: I haz the power, 2017-03-02 06:56:55]

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

[dcheng, 2017-03-02 07:10:59]

Description was changed from

==========
Move unique name generation and tracking into //content.

Blink doesn't really consume this anymore, so move the
concept out of Blink and simplify the plumbing. Future
CLs will move it wholly into the browser process.

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

to

==========
Move unique name generation and tracking into //content.

Blink doesn't really consume this anymore, so move the
concept out of Blink and simplify the plumbing. Future
CLs will move it wholly into the browser process.

BUG=626202,645123
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[dcheng, 2017-03-02 07:11:10]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2017-03-02 07:11:17]

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

[commit-bot: I haz the power, 2017-03-02 07:11:53]

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-03-02 08:09:22]

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

[commit-bot: I haz the power, 2017-03-02 08:09:23]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[dcheng, 2017-03-02 08:53:52]

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

[commit-bot: I haz the power, 2017-03-02 08:54:07]

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-03-02 09:58:33]

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

[commit-bot: I haz the power, 2017-03-02 09:58:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-03-02 16:43:13]

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

[jam, 2017-03-02 16:43:14]

just to check: if this is only used by layout test code, does this have to be
inside content/renderer or content/browser? can it be wholly put inside the
test_runner code?

[dcheng, 2017-03-02 18:30:23]

On 2017/03/02 16:43:14, jam wrote:
> just to check: if this is only used by layout test code, does this have to be
> inside content/renderer or content/browser? can it be wholly put inside the
> test_runner code?

The main purpose of this is history tracking; the layout test code happens to
consume it 'because it exists'.

[dcheng, 2017-03-02 18:39:03]

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

[commit-bot: I haz the power, 2017-03-02 18:39:22]

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-03-02 20:16:14]

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

[commit-bot: I haz the power, 2017-03-02 20:16:15]

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-03-02 20:20:17]

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

[commit-bot: I haz the power, 2017-03-02 20:21:11]

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

[jam, 2017-03-02 20:31:16]

On 2017/03/02 18:30:23, dcheng wrote:
> On 2017/03/02 16:43:14, jam wrote:
> > just to check: if this is only used by layout test code, does this have to
be
> > inside content/renderer or content/browser? can it be wholly put inside the
> > test_runner code?
> 
> The main purpose of this is history tracking; the layout test code happens to
> consume it 'because it exists'.

ah, ok so it's not just layout tests. if so ignore my message

[commit-bot: I haz the power, 2017-03-02 21:45:22]

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

[commit-bot: I haz the power, 2017-03-02 21:45:23]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-03-02 23:59:17]

I'll defer to jam@ on all the build file changes, since I'm not sure about them.
 No objections from me there.

In general this looks good, but I'm not sure about the equivalence of the new
computation code without having time to look more closely, since it's not purely
mechanical.  Maybe there were reasons for the changes?  (Do we have good enough
test coverage here that I shouldn't worry?)

https://codereview.chromium.org/2714943004/diff/90001/content/shell/test_runn...
File content/shell/test_runner/DEPS (right):

https://codereview.chromium.org/2714943004/diff/90001/content/shell/test_runn...
content/shell/test_runner/DEPS:3: "+content",
On 2017/03/01 23:57:26, dcheng wrote:
> content_shell only depends on the content public API, as it's a "canonical
> sample embedder". However, it seems like it should be OK for test_runner to do
> so, as test_runner itself is not meant to be a canonical sample like
> content_shell itself.

I would defer to jam@ here.  Seems ok to me.

https://codereview.chromium.org/2714943004/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (left):

https://codereview.chromium.org/2714943004/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:9050:
TEST_F(WebFrameSwapTest, UniqueNameAfterRemoteToLocalSwap) {
On 2017/03/01 23:57:26, dcheng wrote:
> Working on a browser test for this, since we can't really easily test this in
a
> renderer unit test anymore, since both RenderFrameProxy and RenderFrameImpl
are
> involved.

Yes, I think a browser test for this is worthwhile.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3628: // subframe FrameNavigationEntry
modes).
Oops-- while you're here, can you kill this parenthetical phrase?  I should have
removed it when turning on subframe FNEs by default.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3696: SendDidCommitProvisionalLoad(frame,
commit_type, current_history_item_);
I suppose this is ok.  It's a bit risky if anyone else adds a use of |item|
without realizing they should use |current_history_item_| instead, but I don't
see a great alternative to suggest.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
content/renderer/render_frame_impl.h:262: const std::string& unique_name() const
{ return unique_name_helper_.value(); }
Worth a comment.  Something like:

Returns a name for the frame unique within the page, used for matching up
session history items with recreated frames (and in layout test results).

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
content/renderer/render_frame_proxy.h:11: #include
"content/renderer/unique_name_helper.h"
Doesn't look like this is needed.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
File content/renderer/unique_name_helper.cc (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:55: if
(name.starts_with(kFramePathPrefix) && name.ends_with("-->") &&
This doesn't match FrameTreeUniqueNameCandidate, which doesn't have things like
the --> check.  Is that intentional?  (I was expecting a mostly mechanical move,
so I'm puzzled if I should be looking more carefully at the algorithm.)

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:92: while (parent) {
Same.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
File content/renderer/unique_name_helper.h (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
content/renderer/unique_name_helper.h:68: class UniqueNameHelper {
Let's add a class level comment.  Something like:

Manages the complexity in generating a unique name for a frame, which is used to
match up session history items with corresponding frames.

(I would also suggest moving the large explanation comment from above to be just
below this description.)

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
content/renderer/unique_name_helper.h:81: static std::string
GenerateNameForNewChildFrame(blink::WebFrame* parent,
Why does this need to be static?  Looks like it's only called from
createChildFrame.  (Mainly surprising, not necessarily a problem.  If it's
useful to do so, let's say why in the comment.)

https://codereview.chromium.org/2714943004/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (left):

https://codereview.chromium.org/2714943004/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:60: // Do not recalculate
m_uniqueName if there is no real change of m_name.
Is there an equivalent of this in the new code?  (Or any of this method?  I
can't find corresponding checks in unique_name_helper.)

https://codereview.chromium.org/2714943004/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:106: //    non-unique
|uniqueName| - see https://crbug.com/558680#c14.
Same here.  I'm wondering if it's ok to be losing checks and knowledge like
this?

https://codereview.chromium.org/2714943004/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (left):

https://codereview.chromium.org/2714943004/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11254: TEST_F(WebFrameTest,
UniqueNames) {
Is there an equivalent to this test somewhere?  (Or will you add a browser test
for it like above?)

[dcheng, 2017-03-03 10:22:08]

https://codereview.chromium.org/2714943004/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (left):

https://codereview.chromium.org/2714943004/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:9050:
TEST_F(WebFrameSwapTest, UniqueNameAfterRemoteToLocalSwap) {
On 2017/03/02 23:59:17, Charlie Reis (OOO til Mar 6) wrote:
> On 2017/03/01 23:57:26, dcheng wrote:
> > Working on a browser test for this, since we can't really easily test this
in
> a
> > renderer unit test anymore, since both RenderFrameProxy and RenderFrameImpl
> are
> > involved.
> 
> Yes, I think a browser test for this is worthwhile.

I'm going to put this in a separate CL (that will be landed first).

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3628: // subframe FrameNavigationEntry
modes).
On 2017/03/02 23:59:17, Charlie Reis (OOO til Mar 6) wrote:
> Oops-- while you're here, can you kill this parenthetical phrase?  I should
have
> removed it when turning on subframe FNEs by default.

Done.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3696: SendDidCommitProvisionalLoad(frame,
commit_type, current_history_item_);
On 2017/03/02 23:59:17, Charlie Reis (OOO til Mar 6) wrote:
> I suppose this is ok.  It's a bit risky if anyone else adds a use of |item|
> without realizing they should use |current_history_item_| instead, but I don't
> see a great alternative to suggest.

I considered removing it as a function param and just having
SendDidCommitProvisionalLoad() use the member directly. WDYT?

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
content/renderer/render_frame_impl.h:262: const std::string& unique_name() const
{ return unique_name_helper_.value(); }
On 2017/03/02 23:59:17, Charlie Reis (OOO til Mar 6) wrote:
> Worth a comment.  Something like:
> 
> Returns a name for the frame unique within the page, used for matching up
> session history items with recreated frames (and in layout test results).

Done.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
content/renderer/render_frame_proxy.h:11: #include
"content/renderer/unique_name_helper.h"
On 2017/03/02 23:59:18, Charlie Reis (OOO til Mar 6) wrote:
> Doesn't look like this is needed.

Done.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
File content/renderer/unique_name_helper.cc (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:55: if
(name.starts_with(kFramePathPrefix) && name.ends_with("-->") &&
On 2017/03/02 23:59:18, Charlie Reis (OOO til Mar 6) wrote:
> This doesn't match FrameTreeUniqueNameCandidate, which doesn't have things
like
> the --> check.  Is that intentional?  (I was expecting a mostly mechanical
move,
> so I'm puzzled if I should be looking more carefully at the algorithm.)

There was actually a bug in the original code: if a unique name is just
'<!--framePath ', this would incorrectly match. In addition, the index to the
substring would underflow. This happens to be OK, because std::string guarantees
that substr clamps to the end of the string, but it seemed pretty dicey.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:92: while (parent) {
On 2017/03/02 23:59:18, Charlie Reis (OOO til Mar 6) wrote:
> Same.

This code differs slightly, but the net result is the same. The original code
had the calculation functions as member functions on FrameTree. This poses a
problem for new child frames, which don't yet exist. That made the Blink
implementation subtle, because that means code needs to carefully distinguish
between "I'm the parent FrameTree doing this calculation on behalf of a child"
versus "I'm a FrameTree node that's a child doing the calculation on behalf of
myself."

I've normalized it now so parent always means parent, so we can get rid of the
special case.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
File content/renderer/unique_name_helper.h (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
content/renderer/unique_name_helper.h:68: class UniqueNameHelper {
On 2017/03/02 23:59:18, Charlie Reis (OOO til Mar 6) wrote:
> Let's add a class level comment.  Something like:
> 
> Manages the complexity in generating a unique name for a frame, which is used
to
> match up session history items with corresponding frames.
> 
> (I would also suggest moving the large explanation comment from above to be
just
> below this description.)

Done: I merged the comment documenting value() into here, to avoid too much
redundancy.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
content/renderer/unique_name_helper.h:81: static std::string
GenerateNameForNewChildFrame(blink::WebFrame* parent,
On 2017/03/02 23:59:18, Charlie Reis (OOO til Mar 6) wrote:
> Why does this need to be static?  Looks like it's only called from
> createChildFrame.  (Mainly surprising, not necessarily a problem.  If it's
> useful to do so, let's say why in the comment.)

There's a chicken and egg problem here. We need to calculate the unique name for
a child frame that does not yet exist. To avoid having a UniqueNameHelper
instance have to deal with two subtly different codepaths, I've merged them into
one by plumbing in the correct parent to be using.

https://codereview.chromium.org/2714943004/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (left):

https://codereview.chromium.org/2714943004/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:60: // Do not recalculate
m_uniqueName if there is no real change of m_name.
On 2017/03/02 23:59:18, Charlie Reis (OOO til Mar 6) wrote:
> Is there an equivalent of this in the new code?  (Or any of this method?  I
> can't find corresponding checks in unique_name_helper.)

This is already checked at a higher level, in LocalDOMWindow::setName.

https://codereview.chromium.org/2714943004/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:106: //    non-unique
|uniqueName| - see https://crbug.com/558680#c14.
On 2017/03/02 23:59:18, Charlie Reis (OOO til Mar 6) wrote:
> Same here.  I'm wondering if it's ok to be losing checks and knowledge like
> this?

I've migrated this comment to set_propagated_name

https://codereview.chromium.org/2714943004/diff/190001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (left):

https://codereview.chromium.org/2714943004/diff/190001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11254: TEST_F(WebFrameTest,
UniqueNames) {
On 2017/03/02 23:59:18, Charlie Reis (OOO til Mar 6) wrote:
> Is there an equivalent to this test somewhere?  (Or will you add a browser
test
> for it like above?)

This will get ported to a layout test (also landed separately)

[dcheng, 2017-03-03 10:27:35]

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

[commit-bot: I haz the power, 2017-03-03 10:27:48]

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-03-03 12:21:17]

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

[commit-bot: I haz the power, 2017-03-03 12:21:18]

Dry run: This issue passed the CQ dry run.

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

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

[commit-bot: I haz the power, 2017-03-03 20:25:49]

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-03-03 22:12:20]

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

[commit-bot: I haz the power, 2017-03-03 22:12:21]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-03-05 22:32:54]

PTAL, I've updated the tests so that this CL doesn't lose any test coverage.

https://codereview.chromium.org/2714943004/diff/250001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2714943004/diff/250001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:9593:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
Here's the test that verifies unique names are preserved across process swaps.

https://codereview.chromium.org/2714943004/diff/250001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:9602:
EXPECT_TRUE(NavigateIframeToURL(shell()->web_contents(), "child-0", url));
Not sure if I should bother comparing the expected frame tree here, I feel like
we should be able to assume that it works.

[dcheng, 2017-03-13 00:09:19]

ping =)

[Charlie Reis, 2017-03-20 20:11:39]

Thanks, and very sorry for the long delay!  The changes to the algorithm (and
bug fixes) made it harder to match things up, but I think I'm good with it now.

LGTM with some requests for additional comments, and one possible concern about
cross-process navigations.  (Not sure if that's a new bug, one we have today, or
not a problem for some other reason.)

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3696: SendDidCommitProvisionalLoad(frame,
commit_type, current_history_item_);
On 2017/03/03 10:22:08, dcheng wrote:
> On 2017/03/02 23:59:17, Charlie Reis (OOO til Mar 6) wrote:
> > I suppose this is ok.  It's a bit risky if anyone else adds a use of |item|
> > without realizing they should use |current_history_item_| instead, but I
don't
> > see a great alternative to suggest.
> 
> I considered removing it as a function param and just having
> SendDidCommitProvisionalLoad() use the member directly. WDYT?

Yes, I definitely support that.

It's a bit orthogonal to my concern (i.e., that someone might use |item| in this
method without realizing it has diverged from current_history_item_, but that's
not something I'm too concerned about.  At most, I suppose we could put a "//
Don't use |item| after this point" comment when we assign it to
current_history_item_.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
File content/renderer/unique_name_helper.cc (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:55: if
(name.starts_with(kFramePathPrefix) && name.ends_with("-->") &&
On 2017/03/03 10:22:08, dcheng wrote:
> On 2017/03/02 23:59:18, Charlie Reis (OOO til Mar 6) wrote:
> > This doesn't match FrameTreeUniqueNameCandidate, which doesn't have things
> like
> > the --> check.  Is that intentional?  (I was expecting a mostly mechanical
> move,
> > so I'm puzzled if I should be looking more carefully at the algorithm.)
> 
> There was actually a bug in the original code: if a unique name is just
> '<!--framePath ', this would incorrectly match. In addition, the index to the
> substring would underflow. This happens to be OK, because std::string
guarantees
> that substr clamps to the end of the string, but it seemed pretty dicey.

Thanks for fixing it.  I suppose a test wouldn't have caught it because of
substr (and it's not clear to me if you can get there in practice since I don't
think kFramePathPrefix is a legal frame name), so we should be ok without an
extra test for it.

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:92: while (parent) {
On 2017/03/03 10:22:08, dcheng wrote:
> On 2017/03/02 23:59:18, Charlie Reis (OOO til Mar 6) wrote:
> > Same.
> 
> This code differs slightly, but the net result is the same. The original code
> had the calculation functions as member functions on FrameTree. This poses a
> problem for new child frames, which don't yet exist. That made the Blink
> implementation subtle, because that means code needs to carefully distinguish
> between "I'm the parent FrameTree doing this calculation on behalf of a child"
> versus "I'm a FrameTree node that's a child doing the calculation on behalf of
> myself."
> 
> I've normalized it now so parent always means parent, so we can get rid of the
> special case.

Acknowledged.

https://codereview.chromium.org/2714943004/diff/250001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2714943004/diff/250001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:9593:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
On 2017/03/05 22:32:54, dcheng wrote:
> Here's the test that verifies unique names are preserved across process swaps.

Thanks!

https://codereview.chromium.org/2714943004/diff/250001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:9602:
EXPECT_TRUE(NavigateIframeToURL(shell()->web_contents(), "child-0", url));
On 2017/03/05 22:32:54, dcheng wrote:
> Not sure if I should bother comparing the expected frame tree here, I feel
like
> we should be able to assume that it works.

Yes, we have other tests for that in navigation_controller_impl_browsertest.cc.

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3175:
unique_name_helper_.UpdateName(name.utf8());
This needs a comment saying something about how frames aren't allowed to change
their unique name after a navigation has committed.  Some additional questions:

1) It's a bit subtle that this code checks current_history_item_, but the code a
few lines below checks committed_first_load_.  If there's a reason for it, let's
explain it in the comment.  (Or if those members are going away, maybe we just
need a TODO below to clarify it?)

2) Does this do the right thing for a cross-process navigation, where a frame
may already have a unique name from the previous process but the new renderer
process might not know that there have already been commits in this frame? 
(There's an is_initial_navigation computed from the length of the history in
decidePolicyForNavigation, for example.)  Conversely, though, we would also have
to be careful not to prevent unique name assignment in the case of restoring a
tab, when the tab's history is non-empty but nothing has committed yet.

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/rende...
content/renderer/render_frame_impl.h:262: const std::string& unique_name() const
{ return unique_name_helper_.value(); }
This is still missing the comment.

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
File content/renderer/unique_name_helper.cc (right):

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:71: for (blink::WebFrame* frame =
parent->firstChild(); frame != nullptr;
minor nit: Why use frame != nullptr here and just frame in the loop above?

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:118: const std::string&
likely_unique_suffix) {
Maybe we should keep the DCHECK from the start of FrameTree::appendUniqueSuffix?

   // Verify that we are not doing unnecessary work.	
   DCHECK(uniqueNameExists(prefix));

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:141: std::string
CalculateNewName(blink::WebFrame* parent,
Let's include a comment here about |name| being the assigned name of the frame,
or if that's empty, the fallback name (as in the GenerateNameForNewChildFrame
declaration).

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
File content/renderer/unique_name_helper.h (right):

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
content/renderer/unique_name_helper.h:41: //                    real commit -
afterwards unique name stays immutable).
minor nit: Can you fix the indent here?  I'm not picky on what you use (since
the groups don't seem to agree on whether to align with each other or their
respective ::=='s), but lines 40-41 should at least agree with each other.  :)

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
content/renderer/unique_name_helper.h:95: const std::string& name);
Please also document what we're expecting about |name|.  If I understand
correctly, it's supposed to be the assigned name of the frame, or if that's
empty, then the fallback name.  Is that right?  (That's what
FrameTree::calculateUniqueNameForChildFrame used to do internally, which we
don't do here.  Looks like it's done at the call site in
RenderFrameImpl::createChildFrame instead.)

https://codereview.chromium.org/2714943004/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (left):

https://codereview.chromium.org/2714943004/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:63: // change (i.e. code in
content::FrameTreeNode::SetFrameName).
Can you move a version of this comment to LocalDOMWindow::setName, if we're
still relying on it to be true?

[dcheng, 2017-03-21 22:03:39]

dcheng@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, thestig@chromium.org

[dcheng, 2017-03-21 22:03:42]

+devlin for extensions
+thestig for components/printing

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2714943004/diff/190001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3696: SendDidCommitProvisionalLoad(frame,
commit_type, current_history_item_);
On 2017/03/20 20:11:38, Charlie Reis (slow) wrote:
> On 2017/03/03 10:22:08, dcheng wrote:
> > On 2017/03/02 23:59:17, Charlie Reis (OOO til Mar 6) wrote:
> > > I suppose this is ok.  It's a bit risky if anyone else adds a use of
|item|
> > > without realizing they should use |current_history_item_| instead, but I
> don't
> > > see a great alternative to suggest.
> > 
> > I considered removing it as a function param and just having
> > SendDidCommitProvisionalLoad() use the member directly. WDYT?
> 
> Yes, I definitely support that.
> 
> It's a bit orthogonal to my concern (i.e., that someone might use |item| in
this
> method without realizing it has diverged from current_history_item_, but
that's
> not something I'm too concerned about.  At most, I suppose we could put a "//
> Don't use |item| after this point" comment when we assign it to
> current_history_item_.

Done.

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3175:
unique_name_helper_.UpdateName(name.utf8());
On 2017/03/20 20:11:39, Charlie Reis (slow) wrote:
> This needs a comment saying something about how frames aren't allowed to
change
> their unique name after a navigation has committed.  Some additional
questions:
> 
> 1) It's a bit subtle that this code checks current_history_item_, but the code
a
> few lines below checks committed_first_load_.  If there's a reason for it,
let's
> explain it in the comment.  (Or if those members are going away, maybe we just
> need a TODO below to clarify it?)

Done.
(In retrospect, I believe the committed_first_load_ bool is actually wrong, but
changing that is out of scope of this CL)

> 
> 2) Does this do the right thing for a cross-process navigation, where a frame
> may already have a unique name from the previous process but the new renderer
> process might not know that there have already been commits in this frame? 
> (There's an is_initial_navigation computed from the length of the history in
> decidePolicyForNavigation, for example.)  Conversely, though, we would also
have
> to be careful not to prevent unique name assignment in the case of restoring a
> tab, when the tab's history is non-empty but nothing has committed yet.

It should do the right thing: JS shouldn't be running in a provisional local
frame, and once we've swapped the provisional local frame in, that means it's
committed (so the current history item won't be null)

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/rende...
content/renderer/render_frame_impl.h:262: const std::string& unique_name() const
{ return unique_name_helper_.value(); }
On 2017/03/20 20:11:39, Charlie Reis (slow) wrote:
> This is still missing the comment.

Done.

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
File content/renderer/unique_name_helper.cc (right):

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:71: for (blink::WebFrame* frame =
parent->firstChild(); frame != nullptr;
On 2017/03/20 20:11:39, Charlie Reis (slow) wrote:
> minor nit: Why use frame != nullptr here and just frame in the loop above?

Done.

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:118: const std::string&
likely_unique_suffix) {
On 2017/03/20 20:11:39, Charlie Reis (slow) wrote:
> Maybe we should keep the DCHECK from the start of
FrameTree::appendUniqueSuffix?
> 
>    // Verify that we are not doing unnecessary work.	
>    DCHECK(uniqueNameExists(prefix));

Done.

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
content/renderer/unique_name_helper.cc:141: std::string
CalculateNewName(blink::WebFrame* parent,
On 2017/03/20 20:11:39, Charlie Reis (slow) wrote:
> Let's include a comment here about |name| being the assigned name of the
frame,
> or if that's empty, the fallback name (as in the GenerateNameForNewChildFrame
> declaration).

See previous comment (I'm planning on changing Blink to not expose this detail,
which is pretty messy)

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
File content/renderer/unique_name_helper.h (right):

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
content/renderer/unique_name_helper.h:41: //                    real commit -
afterwards unique name stays immutable).
On 2017/03/20 20:11:39, Charlie Reis (slow) wrote:
> minor nit: Can you fix the indent here?  I'm not picky on what you use (since
> the groups don't seem to agree on whether to align with each other or their
> respective ::=='s), but lines 40-41 should at least agree with each other.  :)

Done.

https://codereview.chromium.org/2714943004/diff/270001/content/renderer/uniqu...
content/renderer/unique_name_helper.h:95: const std::string& name);
On 2017/03/20 20:11:39, Charlie Reis (slow) wrote:
> Please also document what we're expecting about |name|.  If I understand
> correctly, it's supposed to be the assigned name of the frame, or if that's
> empty, then the fallback name.  Is that right?  (That's what
> FrameTree::calculateUniqueNameForChildFrame used to do internally, which we
> don't do here.  Looks like it's done at the call site in
> RenderFrameImpl::createChildFrame instead.)

I've documented this centrally in RenderFrameImpl. UniqueNameHelper itself
doesn't really care what the inputs are, and this documents the subtlety where
it happens.

https://codereview.chromium.org/2714943004/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (left):

https://codereview.chromium.org/2714943004/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:63: // change (i.e. code in
content::FrameTreeNode::SetFrameName).
On 2017/03/20 20:11:39, Charlie Reis (slow) wrote:
> Can you move a version of this comment to LocalDOMWindow::setName, if we're
> still relying on it to be true?

I've put a modified version of this comment in LocalDOMWindow::setName(). Though
I think the DCHECK in content::FrameTreeNode may be at least partly wrong: since
the name is calculated renderer-side, and my recollection is frame swap no
longer requires that the previous frame is synchronously unloaded, we could end
up in a situation where we get two DidChangeName IPCs requesting to change to
the same name, but with different unique names (for reasons)

[Devlin, 2017-03-21 22:19:47]

extensions lgtm

[Lei Zhang, 2017-03-21 23:08:25]

On 2017/03/21 22:03:42, dcheng wrote:
> +thestig for components/printing

lgtm

[dcheng, 2017-03-21 23:44:05]

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

[commit-bot: I haz the power, 2017-03-21 23:44:32]

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-03-21 23:48:17]

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

[commit-bot: I haz the power, 2017-03-21 23:48:18]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[dcheng, 2017-03-22 00:29:58]

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

[commit-bot: I haz the power, 2017-03-22 00:30:57]

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-03-22 02:33:42]

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

[commit-bot: I haz the power, 2017-03-22 02:33:43]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[dcheng, 2017-03-22 02:34:35]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-03-22 02:34:36]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
thestig@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2714943004/#ps330001
(title: "Rebase again.")

[commit-bot: I haz the power, 2017-03-22 02:35:14]

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-03-22 04:36:21]

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

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

Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[dcheng, 2017-03-22 04:53:12]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-03-22 04:53: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-03-22 05:21:08]

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

[commit-bot: I haz the power, 2017-03-22 05:21:15]

Failed to apply patch for
third_party/WebKit/LayoutTests/fast/history/frameset-repeated-name-expected.txt:
While running git apply --index -p1;
  <stdin>:11: trailing whitespace.
  Tests that unique names for frames with identical name attributes don't
collide. This test requires content_shell --run-layout-test to dump the back
forward list.  
  error:
third_party/WebKit/LayoutTests/http/tests/history/frameset-repeated-name-expected.txt:
does not exist in index

Patch:   NR 
third_party/WebKit/LayoutTests/http/tests/history/frameset-repeated-name-expected.txt->third_party/WebKit/LayoutTests/fast/history/frameset-repeated-name-expected.txt
Index:
third_party/WebKit/LayoutTests/fast/history/frameset-repeated-name-expected.txt
diff --git
a/third_party/WebKit/LayoutTests/http/tests/history/frameset-repeated-name-expected.txt
b/third_party/WebKit/LayoutTests/fast/history/frameset-repeated-name-expected.txt
similarity index 58%
rename from
third_party/WebKit/LayoutTests/http/tests/history/frameset-repeated-name-expected.txt
rename to
third_party/WebKit/LayoutTests/fast/history/frameset-repeated-name-expected.txt
index
9a62d764b36b3f8e951b817bc969d8c9177e8b38..13097dd1d31dbc26aeb72d7b78652c6146b4bedd
100644
---
a/third_party/WebKit/LayoutTests/http/tests/history/frameset-repeated-name-expected.txt
+++
b/third_party/WebKit/LayoutTests/fast/history/frameset-repeated-name-expected.txt
@@ -1,13 +1,13 @@
-
+Tests that unique names for frames with identical name attributes don't
collide. This test requires content_shell --run-layout-test to dump the back
forward list.  
 
 ============== Back Forward List ==============
-curr->  http://127.0.0.1:8000/history/frameset-repeated-name.html
-            http://127.0.0.1:8000/history/resources/frameset-dest.html (in
frame "<!--framePath //<!--frame0-->-->")
+curr->  (file test):fast/history/frameset-repeated-name.html
+            (file test):fast/history/resources/frameset-dest.html (in frame
"<!--framePath //<!--frame0-->-->")
                 about:blank (in frame "f1")
                 about:blank (in frame "f2")
                 about:blank (in frame "f3")
                 about:blank (in frame "f4")
-            http://127.0.0.1:8000/history/resources/frameset-dest.html (in
frame "<!--framePath //<!--frame1-->-->")
+            (file test):fast/history/resources/frameset-dest.html (in frame
"<!--framePath //<!--frame1-->-->")
                 about:blank (in frame "<!--framePath
//<!--frame1-->/<!--frame0-->-->")
                 about:blank (in frame "<!--framePath
//<!--frame1-->/<!--frame1-->-->")
                 about:blank (in frame "<!--framePath
//<!--frame1-->/<!--frame2-->-->")

[dcheng, 2017-03-22 23:03:09]

This landed in
https://chromium.googlesource.com/chromium/src.git/+/999698bd4862a5ef634b7f8a...,
so closing.