Replicating WebFrame::uniqueName across renderers.

Replicating WebFrame::uniqueName across renderers.

When a frame is navigated, it might be moved to another renderer
process.  During such move, the other renderer process will create a
local frame object (and the old renderer process will have a remote
frame object).  It is important that the new local frame object has the
same unique name as the frame used to have in the old renderer object.
If the unique name is not correctly replicated to the other renderer
process, then Layout Tests (i.e.
http/tests/security/cross-origin-appcache-allowed.html) fail when run in
site isolation mode (i.e. with --additional-drt-flag=--site-per-process
flag;  note that site isolation mode won't work without extra CLs like
crrev.com/1589643003).

This CL adds a |unique_name| field to the content::FrameReplicationState
struct and plumbs this value from the renderer, through the browser
and back to the renderers.

BUG=576969
TBR=gene@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/464d869b9576471215c7e4a25137ac47061ade29
Cr-Commit-Position: refs/heads/master@{#376772}

[Łukasz Anforowicz, 2016-01-26 19:19:54]

Description was changed from

==========
Replicating WebFrame::uniqueName across renderers.

When a frame is navigated, it might be moved to another renderer
process.  During such move, the other renderer process will create a
local frame object (and the old renderer process will have a remote
frame object).  It is important that the new local frame object has the
same unique name as the frame used to have in the old renderer object.
If the unique name is not correctly replicated to the other renderer
process, then Layout Tests (i.e.
http/tests/security/cross-origin-appcache-allowed.html) fail when run in
site isolation mode (i.e. with --additional-drt-flag=--site-per-process
flag).

This CL adds a |unique_name| field to the content::FrameReplicationState
struct and plumbs this value from the renderer, through the browser
and back to the renderers.

BUG=477150
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Replicating WebFrame::uniqueName across renderers.

When a frame is navigated, it might be moved to another renderer
process.  During such move, the other renderer process will create a
local frame object (and the old renderer process will have a remote
frame object).  It is important that the new local frame object has the
same unique name as the frame used to have in the old renderer object.
If the unique name is not correctly replicated to the other renderer
process, then Layout Tests (i.e.
http/tests/security/cross-origin-appcache-allowed.html) fail when run in
site isolation mode (i.e. with --additional-drt-flag=--site-per-process
flag;  note that site isolation mode won't work without extra CLs like
crrev.com/1589643003).

This CL adds a |unique_name| field to the content::FrameReplicationState
struct and plumbs this value from the renderer, through the browser
and back to the renderers.

BUG=477150, 576969
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Łukasz Anforowicz, 2016-01-27 21:56:55]

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

[Łukasz Anforowicz, 2016-01-27 21:56:56]

Charlie, could you please take a look?

[Charlie Reis, 2016-01-27 22:38:07]

On 2016/01/27 21:56:56, Łukasz Anforowicz wrote:
> Charlie, could you please take a look?

Sure.  I want to spend some time understanding it, because I was under the
impression that the unique name calculation would be deterministic enough that
it would be the same in each process (given the frame name replication we
already have).  Obviously that's not proving true in practice, but I want to see
if that's because of a known issue or not.  (Daniel had some plans to avoid an
issue with the name changing while there's a pending cross-process navigation.)

[Łukasz Anforowicz, 2016-01-27 22:44:24]

On 2016/01/27 22:38:07, Charlie Reis wrote:
> On 2016/01/27 21:56:56, Łukasz Anforowicz wrote:
> > Charlie, could you please take a look?
> 
> Sure.  I want to spend some time understanding it, because I was under the
> impression that the unique name calculation would be deterministic enough that
> it would be the same in each process (given the frame name replication we
> already have).  Obviously that's not proving true in practice, but I want to
see
> if that's because of a known issue or not.  (Daniel had some plans to avoid an
> issue with the name changing while there's a pending cross-process
navigation.)

Let's consider the following frame tree:

mainFrame
+-child1
+-child2

When child1 is added, it is the only child, so childCount()-based [1] uniqueName
will end in 0.  When child1 is navigated to another renderer, the new renderer
will recalculate childCount()-based uniqueName, but this time there are already
2 children, so uniqueName will end in 1.  Callstacks showing how this is
actually hapenning are in https://crbug.com/576969#c2

[1]
https://chromium.googlesource.com/chromium/src/+/988c6dbd265450c00e476fdb7b12...

[Charlie Reis, 2016-01-27 22:54:36]

On 2016/01/27 22:44:24, Łukasz Anforowicz wrote:
> On 2016/01/27 22:38:07, Charlie Reis wrote:
> > On 2016/01/27 21:56:56, Łukasz Anforowicz wrote:
> > > Charlie, could you please take a look?
> > 
> > Sure.  I want to spend some time understanding it, because I was under the
> > impression that the unique name calculation would be deterministic enough
that
> > it would be the same in each process (given the frame name replication we
> > already have).  Obviously that's not proving true in practice, but I want to
> see
> > if that's because of a known issue or not.  (Daniel had some plans to avoid
an
> > issue with the name changing while there's a pending cross-process
> navigation.)
> 
> Let's consider the following frame tree:
> 
> mainFrame
> +-child1
> +-child2
> 
> When child1 is added, it is the only child, so childCount()-based [1]
uniqueName
> will end in 0.  When child1 is navigated to another renderer, the new renderer
> will recalculate childCount()-based uniqueName, but this time there are
already
> 2 children, so uniqueName will end in 1.  Callstacks showing how this is
> actually hapenning are in https://crbug.com/576969#c2
> 
> [1]
>
https://chromium.googlesource.com/chromium/src/+/988c6dbd265450c00e476fdb7b12...

Wow, that really is a broken way to compute the unique name.  Thanks for the
explanation-- I'll take a look at the rest!

[dcheng, 2016-01-27 23:05:55]

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

[dcheng, 2016-01-27 23:05:55]

Is there any other renderer code that needs uniqueName() other than dumping the
layout tree and other layout testing stuff?

If not, maybe we can just remove the plumbing in/out of Blink and just calculate
it in the browser, since you already landed the patch to make layout test dumps
work in OOPI mode.

[Łukasz Anforowicz, 2016-01-27 23:19:21]

On 2016/01/27 23:05:55, dcheng wrote:
> Is there any other renderer code that needs uniqueName() other than dumping
the
> layout tree and other layout testing stuff?
> 
> If not, maybe we can just remove the plumbing in/out of Blink and just
calculate
> it in the browser, since you already landed the patch to make layout test
dumps
> work in OOPI mode.

Example of methods that call blink::WebFrame::uniqueName:

content::HistoryEntry::GetHistoryNodeForFrame
content::RenderFrameImpl::OpenURL (but this is only to send the unique name to
the browser)

Example of methods that call blink::FrameTree::uniqueName:

blink::DocumentLoader::prepareSubframeArchiveLoadIfNeeded
blink::FrameLoader::setHistoryItemStateForCommit

I am not sure at which point these methods are called.  I guess we already have
a synchronous IPC to get the routing id for a newly created frame - we could
pass the unique name back to the renderer at this point.  OTOH, we don't do
synchronous IPC when name is set/changed later (and current code would
recalculate the unique name at this point).


So:

- I would shy away from moving the unique name calculation to the browser in
this CL.

- If we do end up with unique name calculations in the browser, then adding it
to content::FrameReplicationState will be required (and so the current CL is a
step in the right direction / is compatible with moving unique name calculations
to the browser at a later point in time).

[Charlie Reis, 2016-01-27 23:25:06]

On 2016/01/27 23:19:21, Łukasz Anforowicz wrote:
> On 2016/01/27 23:05:55, dcheng wrote:
> > Is there any other renderer code that needs uniqueName() other than dumping
> the
> > layout tree and other layout testing stuff?
> > 
> > If not, maybe we can just remove the plumbing in/out of Blink and just
> calculate
> > it in the browser, since you already landed the patch to make layout test
> dumps
> > work in OOPI mode.
> 
> Example of methods that call blink::WebFrame::uniqueName:
> 
> content::HistoryEntry::GetHistoryNodeForFrame

This is the old session history path, which is still the default until we're
able to turn on UseSubframeNavigationEntries (hopefully this quarter).  I don't
think we can easily move the computation to the browser process until this is
gone.

> content::RenderFrameImpl::OpenURL (but this is only to send the unique name to
> the browser)

This is the new session history path, when UseSubframeNavigationEntries is on. 
We'll be able to compute in the browser process instead when we move to this
path.

> 
> Example of methods that call blink::FrameTree::uniqueName:
> 
> blink::DocumentLoader::prepareSubframeArchiveLoadIfNeeded
> blink::FrameLoader::setHistoryItemStateForCommit
> 
> I am not sure at which point these methods are called.  I guess we already
have
> a synchronous IPC to get the routing id for a newly created frame - we could
> pass the unique name back to the renderer at this point.  OTOH, we don't do
> synchronous IPC when name is set/changed later (and current code would
> recalculate the unique name at this point).
> 

We should avoid a sync IPC.


> 
> So:
> 
> - I would shy away from moving the unique name calculation to the browser in
> this CL.
> 
> - If we do end up with unique name calculations in the browser, then adding it
> to content::FrameReplicationState will be required (and so the current CL is a
> step in the right direction / is compatible with moving unique name
calculations
> to the browser at a later point in time).

+1.

[Charlie Reis, 2016-01-27 23:48:48]

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

[Charlie Reis, 2016-01-27 23:48:48]

A few thoughts on content/ below.  I'd also like to get Alex's eyes on this for
sanity checking the replication state aspect.

Can you add a content_browsertest for the 2 child example you mentioned?  That
would be very useful for documenting the specific need for this.

In general, it does make me a bit sad to have to replicate this, since it's a
lot of machinery.  The two alternatives in my mind each have downsides, though--
computing in the browser process is tricky before we switch to
UseSubframeNavigationEntries, and changing the computation to be deterministic
is not backward compatible with session data stored on disk.

It seems like we're going to have to tackle both of those alternatives at some
point, though.  We want to move the computation to the browser once
UseSubframeNavigationEntries is enabled, and we'll need to make a breaking
change to unique name computation to put a size limit at some point.

In that sense, is this just something to hold us over until we can compute the
name in the browser process?

https://codereview.chromium.org/1635873003/diff/60001/components/html_viewer/...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1635873003/diff/60001/components/html_viewer/...
components/html_viewer/html_frame.cc:136: // TODO(lukasza): Add
|frame_unique_name| to ReplicatedFrameState?
Seems likely (since that's what we're doing in content/), but I'll defer to
Mandoline folks.

https://codereview.chromium.org/1635873003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1635873003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:211:
render_manager_.OnDidUpdateName(name);
Don't we need to broadcast the new unique_name to the proxies?  Or is there a
reason that's not needed?

Also, is it possible for the name to stay the same and the unique name to
change?  If so, we might need to broadcast the change in those cases as well.

https://codereview.chromium.org/1635873003/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1635873003/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1390: // calculates
|unique_name| for the main frame.
I don't understand this TODO or how it relates to the code below.  Could you
clarify?

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
File content/common/frame_replication_state.h (left):

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
content/common/frame_replication_state.h:54: // The assigned name of the frame.
This name can be empty, unlike the unique
Why isn't this true anymore?

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
content/common/frame_replication_state.h:68: // https://crbug.com/576969: This
isn't really "unique" today.
I don't think it makes sense to document this here.  It'll likely just become a
stale comment once the bug is fixed.

[Łukasz Anforowicz, 2016-01-28 01:21:19]

On 2016/01/27 23:48:48, Charlie Reis wrote:
...
> 
> Can you add a content_browsertest for the 2 child example you mentioned?  That
> would be very useful for documenting the specific need for this.

Good idea.  I'll try to do this tomorrow.
> 
> In general, it does make me a bit sad to have to replicate this, since it's a
> lot of machinery.  The two alternatives in my mind each have downsides,
though--
> computing in the browser process is tricky before we switch to
> UseSubframeNavigationEntries, and changing the computation to be deterministic
> is not backward compatible with session data stored on disk.
> 
> It seems like we're going to have to tackle both of those alternatives at some
> point, though.  We want to move the computation to the browser once
> UseSubframeNavigationEntries is enabled, and we'll need to make a breaking
> change to unique name computation to put a size limit at some point.
> 
> In that sense, is this just something to hold us over until we can compute the
> name in the browser process?

RE: Computing in the browser process

It is tricky not only because UseSubframeNavigationEntries are not enabled yet,
but also because there is blink code that uses unique name.  I probably should
try to dig deeper, to understand what this blink code tries to do / why it needs
a frame unique name.

RE: Changing how we calculate unique name

Yes - this is something we should consider as an alternative approach to the
current CL.  I thought that the current CL might be less trouble than having to
rebaseline all recursive layout tests (which might be difficult without closing
the tree or something like this).  OTOH, this would avoid the complexity of
sending the unique name into the browser and back.

Are we confident that we know how we want unique name to look like in the
future?  For example:
- Does it need to include frame's name (if present)?
- Does it need to include frame's position and/or number of siblings and/or
overall frame tree shape?
- Can it / should it include main frame's uri?

Is there a section of a w3c spec that one could read to better understand how
session history feature forces us to use frame unique name today?

https://codereview.chromium.org/1635873003/diff/60001/components/html_viewer/...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1635873003/diff/60001/components/html_viewer/...
components/html_viewer/html_frame.cc:136: // TODO(lukasza): Add
|frame_unique_name| to ReplicatedFrameState?
On 2016/01/27 23:48:49, Charlie Reis wrote:
> Seems likely (since that's what we're doing in content/), but I'll defer to
> Mandoline folks.

FWIW, similar TODOs are still here for other like WebFrameOwnerProperties (from
https://chromium.googlesource.com/chromium/src/+/70605c32bd6a79317df00114ec55...).

Hopefully we can figure it out when doing an owners review for the changes under
//components/html_viewer

https://codereview.chromium.org/1635873003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1635873003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:211:
render_manager_.OnDidUpdateName(name);
On 2016/01/27 23:48:49, Charlie Reis wrote:
> Don't we need to broadcast the new unique_name to the proxies?  Or is there a
> reason that's not needed?

Ooops.  Good catch - I looked at RenderFrameHostImpl::OnDidChangeName, but
didn't notice that we broadcast the change above.  

Done.

> 
> Also, is it possible for the name to stay the same and the unique name to
> change?  If so, we might need to broadcast the change in those cases as well.

No - the unique name changes are tied to name changes (in blink::FrameTree).

https://codereview.chromium.org/1635873003/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1635873003/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1390: // calculates
|unique_name| for the main frame.
On 2016/01/27 23:48:49, Charlie Reis wrote:
> I don't understand this TODO or how it relates to the code below.  Could you
> clarify?

Both 1) the code below and 2) the code in FrameTree calculate unique name.  It
just turns out that calculating a unique name for a main frame means "just use
the name of the frame as the unique name", so hopefully duplicating this logic
below is ok.

I am not sure how to clarify the comment :-(

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
File content/common/frame_replication_state.h (left):

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
content/common/frame_replication_state.h:54: // The assigned name of the frame.
This name can be empty, unlike the unique
On 2016/01/27 23:48:49, Charlie Reis wrote:
> Why isn't this true anymore?

AFAICT it was never true.  If you look at FrameTree::setName, you see that for
main frame we simply assign |m_uniqueName = name|.  And the main frame initially
uses nullAtom as its name (i.e. see WebViewImpl::setMainFrame method).

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
content/common/frame_replication_state.h:68: // https://crbug.com/576969: This
isn't really "unique" today.
On 2016/01/27 23:48:49, Charlie Reis wrote:
> I don't think it makes sense to document this here.

Done.

> It'll likely just become a
> stale comment once the bug is fixed.

Hmmm... should there be a tool that complains when there are crbug comments in
Chromium's source code that refer to bugs that have been marked as fixed?

[alexmos, 2016-02-10 00:46:46]

Since Lukasz is going to dust off this CL, here are a few comments, mostly nits.
 The replication bits look good overall.

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
content/common/frame_replication_state.h:68: // https://crbug.com/576969: This
isn't really "unique" today.
> > It'll likely just become a
> > stale comment once the bug is fixed.
> 
> Hmmm... should there be a tool that complains when there are crbug comments in
> Chromium's source code that refer to bugs that have been marked as fixed?

I don't think that's always undesirable.  For example, when I'm writing a test,
I often mention which bug it's for, and I think it's fine to refer to a fixed
bug in that case.

https://codereview.chromium.org/1635873003/diff/100001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1635873003/diff/100001/components/html_viewer...
components/html_viewer/html_frame.cc:489: void
HTMLFrame::didChangeName(blink::WebLocalFrame* /* frame */,
Is this style ok?  I haven't really seen this (leaving arg names out) done
outside of Blink headers and IPC messages, and it doesn't seem like it's done
elsewhere in this file.

https://codereview.chromium.org/1635873003/diff/100001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1635873003/diff/100001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:210: if (name !=
replication_state_.name)
Maybe add a comment here explaining why there's no similar check for the
unique_name change?  I.e., that |unique_name| changes are tied to |name|
changes, so it's impossible for the name to stay the same and the unique name to
change.

https://codereview.chromium.org/1635873003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1635873003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:437: // window.name
property.
nit: explain why this now also takes unique_name in the comment.

https://codereview.chromium.org/1635873003/diff/100001/content/common/frame_r...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1635873003/diff/100001/content/common/frame_r...
content/common/frame_replication_state.h:67: // Unique name of the frame.
Maybe also mention what it's for, why it's replicated, and that its updates are
tied to |name| updates?

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

https://codereview.chromium.org/1635873003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.h:441: const blink::WebString& uniqueName,
nit: unique_name? (and maybe fix sandbox_flags while we're at it?)

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:61: m_uniqueName =
AtomicString(); // Remove our old frame name so it's not considered in
uniqueChildName.
Update function name in comment?

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:732:
child->initializeCoreFrame(frame()->host(), result.storedValue->value.get(),
name, nullAtom, nullAtom);
Should this be uniqueName?

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:787:
frame()->tree().setNameForReplacementFrame(name, uniqueName);
Hmm, the comment for that function says: "Should only be used when switching
between LocalFrames and RemoteFrames for the same logical frame so that the
unique name stays unique", but this isn't necessarily true here (the name can be
updated without swapping between frames).  Should we update that comment? 
Should this function be renamed to not be specific to replacement frames given
that we use it here?

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebRemoteFrame.h (right):

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebRemoteFrame.h:39: // Set frame name replicated
from another process.
nit: explain uniqueName in the comment.

[Łukasz Anforowicz, 2016-02-10 22:04:30]

Description was changed from

==========
Replicating WebFrame::uniqueName across renderers.

When a frame is navigated, it might be moved to another renderer
process.  During such move, the other renderer process will create a
local frame object (and the old renderer process will have a remote
frame object).  It is important that the new local frame object has the
same unique name as the frame used to have in the old renderer object.
If the unique name is not correctly replicated to the other renderer
process, then Layout Tests (i.e.
http/tests/security/cross-origin-appcache-allowed.html) fail when run in
site isolation mode (i.e. with --additional-drt-flag=--site-per-process
flag;  note that site isolation mode won't work without extra CLs like
crrev.com/1589643003).

This CL adds a |unique_name| field to the content::FrameReplicationState
struct and plumbs this value from the renderer, through the browser
and back to the renderers.

BUG=477150, 576969
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Replicating WebFrame::uniqueName across renderers.

When a frame is navigated, it might be moved to another renderer
process.  During such move, the other renderer process will create a
local frame object (and the old renderer process will have a remote
frame object).  It is important that the new local frame object has the
same unique name as the frame used to have in the old renderer object.
If the unique name is not correctly replicated to the other renderer
process, then Layout Tests (i.e.
http/tests/security/cross-origin-appcache-allowed.html) fail when run in
site isolation mode (i.e. with --additional-drt-flag=--site-per-process
flag;  note that site isolation mode won't work without extra CLs like
crrev.com/1589643003).

This CL adds a |unique_name| field to the content::FrameReplicationState
struct and plumbs this value from the renderer, through the browser
and back to the renderers.

BUG=576969
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Łukasz Anforowicz, 2016-02-10 22:10:51]

Charlie, can you take another look please?

I think I've addressed all comments (from you and from Alex), except the
suggestion to add a browser test.  I am not sure if the new test is required
(since existing layout tests already expose lack of replication by failing with
--site-per-process).  If you feel strongly about the test, could you please
suggest how I can observe uniqueName differences from the test? (AFAIK unique
name was not exposed to the browser side until this CL;  additionally AFAIK
uniqueName is hidden in an internal state of the renderer and is not easily
observable via javascript or other mechanism available from browser tests).

In the latest patchset I've also removed |fallbackName| from
|WebFrameImplBase::initializeCoreFrame|, because it was only needed if
|uniqueName| hasn't been precalculated and right now |initializeCoreFrame|
method always gets a precalculated |uniqueName|.

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
content/common/frame_replication_state.h:68: // https://crbug.com/576969: This
isn't really "unique" today.
On 2016/02/10 00:46:45, alexmos (OOO until Feb 16) wrote:
> > > It'll likely just become a
> > > stale comment once the bug is fixed.
> > 
> > Hmmm... should there be a tool that complains when there are crbug comments
in
> > Chromium's source code that refer to bugs that have been marked as fixed?
> 
> I don't think that's always undesirable.  For example, when I'm writing a
test,
> I often mention which bug it's for, and I think it's fine to refer to a fixed
> bug in that case.

Acknowledged.  Maybe just pay attention to bugs on TODO lines?  I get 1081 bugs
with the cmdline below and some of them are stale (i.e. issue 8123):

$ git grep 'TODO.*crbug.com/[0-9]' | \
    sed -e 's:^.*crbug.com/\([0-9]\+\).*$:\1:g' | \
    sort | uniq

https://codereview.chromium.org/1635873003/diff/100001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1635873003/diff/100001/components/html_viewer...
components/html_viewer/html_frame.cc:489: void
HTMLFrame::didChangeName(blink::WebLocalFrame* /* frame */,
On 2016/02/10 00:46:46, alexmos (OOO until Feb 16) wrote:
> Is this style ok?  I haven't really seen this (leaving arg names out) done
> outside of Blink headers and IPC messages, and it doesn't seem like it's done
> elsewhere in this file.

I don't remember where I first saw this style, but it is supposed to convey that
the given parameter is not used in the function/method body (i.e. looking at the
parameters list one can quickly see which parameters matter and which are
ignored).  This shouldn't matter anymore for html_frame.cc (because unrelated CL
removed this file so I don't need to make the change above), but I am open for
discussing this style in context of other files (I might have used similar style
elsewhere).

Charlie?  WDYT?

https://codereview.chromium.org/1635873003/diff/100001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1635873003/diff/100001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:210: if (name !=
replication_state_.name)
On 2016/02/10 00:46:46, alexmos (OOO until Feb 16) wrote:
> Maybe add a comment here explaining why there's no similar check for the
> unique_name change?  I.e., that |unique_name| changes are tied to |name|
> changes, so it's impossible for the name to stay the same and the unique name
to
> change.

Done (via a combination of a comment + DCHECK + refactoring the if statement to
become an early return).

https://codereview.chromium.org/1635873003/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1635873003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:437: // window.name
property.
On 2016/02/10 00:46:46, alexmos (OOO until Feb 16) wrote:
> nit: explain why this now also takes unique_name in the comment.

Done (I rewrote the comment).

https://codereview.chromium.org/1635873003/diff/100001/content/common/frame_r...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1635873003/diff/100001/content/common/frame_r...
content/common/frame_replication_state.h:67: // Unique name of the frame.
On 2016/02/10 00:46:46, alexmos (OOO until Feb 16) wrote:
> Maybe also mention what it's for, why it's replicated, and that its updates
are
> tied to |name| updates?

I've expanded the comment:
1) added a reference to WebFrame::uniqueName
2) explained how unique name is used
3) explained why we need to replicate.

I didn't add explanation that updates of |unique_name| are tied to |name|
updates - this seems like an implementation detail that can change (right?) and
therefore I think we should avoid putting this into a comment (because it can
become stale and misleading).

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

https://codereview.chromium.org/1635873003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.h:441: const blink::WebString& uniqueName,
On 2016/02/10 00:46:46, alexmos (OOO until Feb 16) wrote:
> nit: unique_name? (and maybe fix sandbox_flags while we're at it?)

Done.

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:61: m_uniqueName =
AtomicString(); // Remove our old frame name so it's not considered in
uniqueChildName.
On 2016/02/10 00:46:46, alexmos (OOO until Feb 16) wrote:
> Update function name in comment?

Ooops - thanks for catching this.  Done.

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:732:
child->initializeCoreFrame(frame()->host(), result.storedValue->value.get(),
name, nullAtom, nullAtom);
On 2016/02/10 00:46:46, alexmos (OOO until Feb 16) wrote:
> Should this be uniqueName?

Done.  Thanks for catching this.  Not sure how I managed to write this :-(

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:787:
frame()->tree().setNameForReplacementFrame(name, uniqueName);
On 2016/02/10 00:46:46, alexmos (OOO until Feb 16) wrote:
> Hmm, the comment for that function says: "Should only be used when switching
> between LocalFrames and RemoteFrames for the same logical frame so that the
> unique name stays unique", but this isn't necessarily true here (the name can
be
> updated without swapping between frames).  Should we update that comment? 
> Should this function be renamed to not be specific to replacement frames given
> that we use it here?  

Done.  Good point.  I did think about it when writing the CL, but then didn't
follow-up... :-(

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebRemoteFrame.h (right):

https://codereview.chromium.org/1635873003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebRemoteFrame.h:39: // Set frame name replicated
from another process.
On 2016/02/10 00:46:46, alexmos (OOO until Feb 16) wrote:
> nit: explain uniqueName in the comment.

Done.

[Charlie Reis, 2016-02-11 00:53:38]

On 2016/02/10 22:10:51, Łukasz Anforowicz wrote:
> Charlie, can you take another look please?

Great.  I'm part of the way through, and I'll finish up tomorrow.  Thanks!

[Łukasz Anforowicz, 2016-02-11 20:38:53]

On 2016/02/11 00:53:38, Charlie Reis wrote:
> On 2016/02/10 22:10:51, Łukasz Anforowicz wrote:
> > Charlie, can you take another look please?
> 
> Great.  I'm part of the way through, and I'll finish up tomorrow.  Thanks!

Thanks.  I've just wanted to give a heads-up that in patchset #12 I had to fix
some fallout from a new assert that I've added to
third_party/WebKit/Source/core/page/FrameTree.cpp in patchset #10.  Most of the
changes in patchset #12 are just making the test code provide a non-empty unique
name where applicable, but one change (sending a real, rather than default
FrameReplicationState in
content/browser/frame_host/render_frame_host_manager.cc) seems to be a separate
product bug caught by the assert - you probably want to double-check that what I
did makes sense in this file.

[Charlie Reis, 2016-02-11 22:02:14]

Great.  content/ LGTM with some thoughts below.

On 2016/02/10 22:10:51, Łukasz Anforowicz wrote:
> Charlie, can you take another look please?
> 
> I think I've addressed all comments (from you and from Alex), except the
> suggestion to add a browser test.  I am not sure if the new test is required
> (since existing layout tests already expose lack of replication by failing
with
> --site-per-process).  If you feel strongly about the test, could you please
> suggest how I can observe uniqueName differences from the test? (AFAIK unique
> name was not exposed to the browser side until this CL;  additionally AFAIK
> uniqueName is hidden in an internal state of the renderer and is not easily
> observable via javascript or other mechanism available from browser tests).
> 

I agree that the layout tests provide some coverage here, but it's a bit
indirect in my opinion.  I'm fine with adding a content_browsertest later but I
do think we should have one to point out this case.  It seems easy to test now
that we have it in the FrameReplicationState.  (The test wouldn't fail unless
the replication state were ignored in the renderer, but I think it would be
easier to understand if we ended up with a similar bug when refactoring unique
name calculation.)

https://codereview.chromium.org/1635873003/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1635873003/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1390: // calculates
|unique_name| for the main frame.
On 2016/01/28 01:21:19, Łukasz Anforowicz wrote:
> On 2016/01/27 23:48:49, Charlie Reis wrote:
> > I don't understand this TODO or how it relates to the code below.  Could you
> > clarify?
> 
> Both 1) the code below and 2) the code in FrameTree calculate unique name.  It
> just turns out that calculating a unique name for a main frame means "just use
> the name of the frame as the unique name", so hopefully duplicating this logic
> below is ok.
> 
> I am not sure how to clarify the comment :-(

Oh, I see.  The only calculation here is an assignment.  I'm ok with your new
comment.

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
File content/common/frame_replication_state.h (left):

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
content/common/frame_replication_state.h:54: // The assigned name of the frame.
This name can be empty, unlike the unique
On 2016/01/28 01:21:19, Łukasz Anforowicz wrote:
> On 2016/01/27 23:48:49, Charlie Reis wrote:
> > Why isn't this true anymore?
> 
> AFAICT it was never true.  If you look at FrameTree::setName, you see that for
> main frame we simply assign |m_uniqueName = name|.  And the main frame
initially
> uses nullAtom as its name (i.e. see WebViewImpl::setMainFrame method).

Acknowledged.

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
content/common/frame_replication_state.h:68: // https://crbug.com/576969: This
isn't really "unique" today.
On 2016/02/10 22:10:50, Łukasz Anforowicz wrote:
> On 2016/02/10 00:46:45, alexmos (OOO until Feb 16) wrote:
> > > > It'll likely just become a
> > > > stale comment once the bug is fixed.
> > > 
> > > Hmmm... should there be a tool that complains when there are crbug
comments
> in
> > > Chromium's source code that refer to bugs that have been marked as fixed?
> > 
> > I don't think that's always undesirable.  For example, when I'm writing a
> test,
> > I often mention which bug it's for, and I think it's fine to refer to a
fixed
> > bug in that case.
> 
> Acknowledged.  Maybe just pay attention to bugs on TODO lines?  I get 1081
bugs
> with the cmdline below and some of them are stale (i.e. issue 8123):
> 
> $ git grep 'TODO.*crbug.com/[0-9]' | \
>     sed -e 's:^.*crbug.com/\([0-9]\+\).*$:\1:g' | \
>     sort | uniq

Yeah, I've wondered this myself sometimes.  Finding TODOs with closed bug
numbers could be useful.  What would you do with that data?  It seems more like
something to process in the background and email the author of the TODO (and/or
the person who closed the bug).

https://codereview.chromium.org/1635873003/diff/100001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1635873003/diff/100001/components/html_viewer...
components/html_viewer/html_frame.cc:489: void
HTMLFrame::didChangeName(blink::WebLocalFrame* /* frame */,
On 2016/02/10 22:10:50, Łukasz Anforowicz wrote:
> On 2016/02/10 00:46:46, alexmos (OOO until Feb 16) wrote:
> > Is this style ok?  I haven't really seen this (leaving arg names out) done
> > outside of Blink headers and IPC messages, and it doesn't seem like it's
done
> > elsewhere in this file.
> 
> I don't remember where I first saw this style, but it is supposed to convey
that
> the given parameter is not used in the function/method body (i.e. looking at
the
> parameters list one can quickly see which parameters matter and which are
> ignored).  This shouldn't matter anymore for html_frame.cc (because unrelated
CL
> removed this file so I don't need to make the change above), but I am open for
> discussing this style in context of other files (I might have used similar
style
> elsewhere).
> 
> Charlie?  WDYT?

It's moot in this case, since this file is gone now, but it looks like it's used
in print_web_view_helper.cc as well.  In general, it's allowed by the style
guide but I thought it was discouraged in Chromium code.  I'd have to dig around
to find an old thread on it.

https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...
"Parameter names may be omitted only if the parameter is unused and its purpose
is obvious."

Personally, I'd prefer to have the actual names there rather than comments,
since it's more consistent with other code around it and doesn't provide a huge
benefit.

https://codereview.chromium.org/1635873003/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1635873003/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:819: // TODO(lukasza): Call
ReceivedBadMessage when |frame_unique_name| is empty?
Yep, that'll be useful down the road.  You can make this a statement rather than
a question.

https://codereview.chromium.org/1635873003/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1421:
frame_tree_node()->SetFrameName(name, unique_name);
Same TODO about BadMessageReceived?

https://codereview.chromium.org/1635873003/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1635873003/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1803:
render_frame_host->frame_tree_node()->current_replication_state()));
Yes, this looks reasonable to me.  I can't think of a reason to skip the
replication state for a GuestView embedder's proxy to its guest.

[Łukasz Anforowicz, 2016-02-11 23:23:56]

Thanks for reviewing.  Daniel, could you please take a look?

On 2016/02/11 22:02:14, Charlie Reis wrote:
> Great.  content/ LGTM with some thoughts below.
> 
> On 2016/02/10 22:10:51, Łukasz Anforowicz wrote:
> > Charlie, can you take another look please?
> > 
> > I think I've addressed all comments (from you and from Alex), except the
> > suggestion to add a browser test.  I am not sure if the new test is required
> > (since existing layout tests already expose lack of replication by failing
> with
> > --site-per-process).  If you feel strongly about the test, could you please
> > suggest how I can observe uniqueName differences from the test? (AFAIK
unique
> > name was not exposed to the browser side until this CL;  additionally AFAIK
> > uniqueName is hidden in an internal state of the renderer and is not easily
> > observable via javascript or other mechanism available from browser tests).
> 
> I agree that the layout tests provide some coverage here, but it's a bit
> indirect in my opinion.  I'm fine with adding a content_browsertest later but
I
> do think we should have one to point out this case.  It seems easy to test now
> that we have it in the FrameReplicationState.  (The test wouldn't fail unless
> the replication state were ignored in the renderer, but I think it would be
> easier to understand if we ended up with a similar bug when refactoring unique
> name calculation.)

crbug.com/576969 covers 2 bugs really (this is probably bad / I should split the
bug somehow...):
1. uniqueName is sometimes not unique (2 frames end up with same unique name)
2. uniqueName of a frame differs across renderers

This CL fixes bug #2.  Bug #2 is only really impacting state *within* a renderer
- we might still have this bug if renderer tries to recalculate the unique name
on its own at some (maybe later) point (even if FrameReplicationState contained
the right unique name + even if renderer earlier correctly consumed the value
from FrameReplicationState).

I agree that a browser test might be useful to test #1 - to verify that each
frame has a truly unique name as visible via FrameReplicationState (although
here also we could argue that this is well covered by layout tests - for example
by the no-yet-landed one sketched in https://crbug.com/576969#c3).

At any rate - let's try to land the replication bits first :-)

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1635873003/diff/60001/content/common/frame_re...
content/common/frame_replication_state.h:68: // https://crbug.com/576969: This
isn't really "unique" today.
On 2016/02/11 22:02:14, Charlie Reis wrote:
> On 2016/02/10 22:10:50, Łukasz Anforowicz wrote:
> > On 2016/02/10 00:46:45, alexmos (OOO until Feb 16) wrote:
> > > > > It'll likely just become a
> > > > > stale comment once the bug is fixed.
> > > > 
> > > > Hmmm... should there be a tool that complains when there are crbug
> comments
> > in
> > > > Chromium's source code that refer to bugs that have been marked as
fixed?
> > > 
> > > I don't think that's always undesirable.  For example, when I'm writing a
> > test,
> > > I often mention which bug it's for, and I think it's fine to refer to a
> fixed
> > > bug in that case.
> > 
> > Acknowledged.  Maybe just pay attention to bugs on TODO lines?  I get 1081
> bugs
> > with the cmdline below and some of them are stale (i.e. issue 8123):
> > 
> > $ git grep 'TODO.*crbug.com/[0-9]' | \
> >     sed -e 's:^.*crbug.com/\([0-9]\+\).*$:\1:g' | \
> >     sort | uniq
> 
> Yeah, I've wondered this myself sometimes.  Finding TODOs with closed bug
> numbers could be useful.  What would you do with that data?  It seems more
like
> something to process in the background and email the author of the TODO
(and/or
> the person who closed the bug).

I thought that the bug could be marked with a label like "StaleTodo" and
commented with the location of the TODO.  The label should help avoid commenting
on the same bug twice.

This also sounds like something where a quick ad-hoc script would do a job more
or less fine, but where potentially a lot of effort is needed to do things the
right way (host the script somewhere, run it automatically, maintain
write-permissions for the script, make sure that this is actually useful and
that somebody follows-up on the marked bugs).  Maybe I should try to pitch the
idea on chromium-dev.

https://codereview.chromium.org/1635873003/diff/100001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1635873003/diff/100001/components/html_viewer...
components/html_viewer/html_frame.cc:489: void
HTMLFrame::didChangeName(blink::WebLocalFrame* /* frame */,
On 2016/02/11 22:02:14, Charlie Reis wrote:
> On 2016/02/10 22:10:50, Łukasz Anforowicz wrote:
> > On 2016/02/10 00:46:46, alexmos (OOO until Feb 16) wrote:
> > > Is this style ok?  I haven't really seen this (leaving arg names out) done
> > > outside of Blink headers and IPC messages, and it doesn't seem like it's
> done
> > > elsewhere in this file.
> > 
> > I don't remember where I first saw this style, but it is supposed to convey
> that
> > the given parameter is not used in the function/method body (i.e. looking at
> the
> > parameters list one can quickly see which parameters matter and which are
> > ignored).  This shouldn't matter anymore for html_frame.cc (because
unrelated
> CL
> > removed this file so I don't need to make the change above), but I am open
for
> > discussing this style in context of other files (I might have used similar
> style
> > elsewhere).
> > 
> > Charlie?  WDYT?
> 
> It's moot in this case, since this file is gone now, but it looks like it's
used
> in print_web_view_helper.cc as well.  In general, it's allowed by the style
> guide but I thought it was discouraged in Chromium code.  I'd have to dig
around
> to find an old thread on it.
> 
>
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...
> "Parameter names may be omitted only if the parameter is unused and its
purpose
> is obvious."
> 
Right, but 1) we are not "omitting" the parameter names here, we are only
commenting them out and 2) your link says a few lines below that "Unused
parameters that might not be obvious should comment out the variable name in the
function definition" and gives an example consistent with what I did above:

void Circle::Rotate(double /*radians*/) {}
> Personally, I'd prefer to have the actual names there rather than comments,
> since it's more consistent with other code around it and doesn't provide a
huge
> benefit.

Done.

https://codereview.chromium.org/1635873003/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1635873003/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:819: // TODO(lukasza): Call
ReceivedBadMessage when |frame_unique_name| is empty?
On 2016/02/11 22:02:14, Charlie Reis wrote:
> Yep, that'll be useful down the road.  You can make this a statement rather
than
> a question.

Done.

https://codereview.chromium.org/1635873003/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1421:
frame_tree_node()->SetFrameName(name, unique_name);
On 2016/02/11 22:02:14, Charlie Reis wrote:
> Same TODO about BadMessageReceived?

Hmmm... maybe I should add DCHECKs here to make it clearer what should be
checked (you are right that a similar TODO can be added here, but not for the
main frame [which can have an empty unique name]).

Done.

https://codereview.chromium.org/1635873003/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1635873003/diff/220001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1803:
render_frame_host->frame_tree_node()->current_replication_state()));
On 2016/02/11 22:02:14, Charlie Reis wrote:
> Yes, this looks reasonable to me.  I can't think of a reason to skip the
> replication state for a GuestView embedder's proxy to its guest.

Acknowledged.

[dcheng, 2016-02-16 22:17:30]

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:184:
name.appendNumber(childCount() - (existingChildFrame ? 1 : 0));
Is it possible to pull the FrameTree changes into a separate patch?

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1590:
frame()->tree().setReplicatedName(name, uniqueName);
This isn't really setting the replicated name though: the name was calculated in
process for the WebLocalFrame::createChildFrame case.

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1612: // FIXME: Using
subResourceAttributeName as fallback is not a perfect
Move this FIXME to line 1606?

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right):

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:327: WebLocalFrame*
localFrame = rootFrame->createLocalChild(WebTreeScopeType::Document, "name",
"uniqueName", WebSandboxFlags::None, &localFrameClient, nullptr, properties);
I would have expected name == uniqueName for most of these test cases.

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:159: virtual WebFrame*
createChildFrame(WebLocalFrame* parent, WebTreeScopeType, const WebString&
frameName, const WebString& frameUniqueName, WebSandboxFlags sandboxFlags, const
WebFrameOwnerProperties&) { return nullptr; }
const WebString& name, const WebString& uniqueName

[Łukasz Anforowicz, 2016-02-16 23:39:53]

Daniel, can you look at my replies below?

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:184:
name.appendNumber(childCount() - (existingChildFrame ? 1 : 0));
On 2016/02/16 22:17:29, dcheng wrote:
> Is it possible to pull the FrameTree changes into a separate patch?

I think it is not possible.

1. I've added propagation of |uniqueName| wherever |name| was propagated - this
covers WebFrameClient::createChildFrame.

2. (1) requires that WebLocalFrameImpl::initializeCoreFrame knows the
|uniqueName| before calling WebFrameClient::createChildFrame - the |uniqueName|
is provided by a new FrameTree::calculateUniqueNameForNewChildFrame method.

3. The child frame has not yet been created and added to the parent frame at the
time when FrameTree::calculateUniqueNameForNewChildFrame method is called from
WebLocalFrameImpl::initializeCoreFrame.  This means that
FrameTree::calculateUniqueNameForChildFrame has to adjust childCount()-based
calculations depending on how it is called (for new vs for existing child
frame).

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1590:
frame()->tree().setReplicatedName(name, uniqueName);
On 2016/02/16 22:17:29, dcheng wrote:
> This isn't really setting the replicated name though: the name was calculated
in
> process for the WebLocalFrame::createChildFrame case.

Good point - I wasn't quite happy about setReplicatedName method name.  Does
setPrecalculatedName sound better?

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1612: // FIXME: Using
subResourceAttributeName as fallback is not a perfect
On 2016/02/16 22:17:29, dcheng wrote:
> Move this FIXME to line 1606?

Ooops.  Done.

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right):

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:327: WebLocalFrame*
localFrame = rootFrame->createLocalChild(WebTreeScopeType::Document, "name",
"uniqueName", WebSandboxFlags::None, &localFrameClient, nullptr, properties);
On 2016/02/16 22:17:29, dcheng wrote:
> I would have expected name == uniqueName for most of these test cases.

In product code, we have name == uniqueName for the main frame, but for all
child frames this equality doesn't hold.

Additionally, there are new asserts that will complain when uniqueName.isEmpty
for a child frame (because of this we cannot have name == uniqueName in at least
some cases, like the first modified tests in
content/browser/frame_host/frame_tree_unittest.cc).

That said, I guess it is ok to have name == uniqueName in
FrameThrottlingTest.cpp and WebFrameTest.cpp.  I started making this change, but
I got hesitant - when I see/read |"name", "uniqueName"| I can reasonably guess
what both of these arguments are, but it is not so obvious when reading |"name",
"name"|.

WDYT?

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:159: virtual WebFrame*
createChildFrame(WebLocalFrame* parent, WebTreeScopeType, const WebString&
frameName, const WebString& frameUniqueName, WebSandboxFlags sandboxFlags, const
WebFrameOwnerProperties&) { return nullptr; }
On 2016/02/16 22:17:30, dcheng wrote:
> const WebString& name, const WebString& uniqueName

Done.

[dcheng, 2016-02-19 18:14:57]

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right):

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:327: WebLocalFrame*
localFrame = rootFrame->createLocalChild(WebTreeScopeType::Document, "name",
"uniqueName", WebSandboxFlags::None, &localFrameClient, nullptr, properties);
On 2016/02/16 at 23:39:53, Łukasz Anforowicz wrote:
> On 2016/02/16 22:17:29, dcheng wrote:
> > I would have expected name == uniqueName for most of these test cases.
> 
> In product code, we have name == uniqueName for the main frame, but for all
child frames this equality doesn't hold.
> 
> Additionally, there are new asserts that will complain when uniqueName.isEmpty
for a child frame (because of this we cannot have name == uniqueName in at least
some cases, like the first modified tests in
content/browser/frame_host/frame_tree_unittest.cc).
> 
> That said, I guess it is ok to have name == uniqueName in
FrameThrottlingTest.cpp and WebFrameTest.cpp.  I started making this change, but
I got hesitant - when I see/read |"name", "uniqueName"| I can reasonably guess
what both of these arguments are, but it is not so obvious when reading |"name",
"name"|.
> 
> WDYT?

I don't know, it just seems weird to explicitly hardcode something in tests
that's different from what actually happens. Maybe we need a test helper for
createLocalChild / createRemoteChild: it'd probably be reasonable to add this to
FrameTestHelpers. createLocalChild() has a lot of arguments that are almost
always use default values in tests.

[Łukasz Anforowicz, 2016-02-19 19:56:59]

Daniel, can you take another look please?

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right):

https://codereview.chromium.org/1635873003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:327: WebLocalFrame*
localFrame = rootFrame->createLocalChild(WebTreeScopeType::Document, "name",
"uniqueName", WebSandboxFlags::None, &localFrameClient, nullptr, properties);
On 2016/02/19 18:14:57, dcheng wrote:
> On 2016/02/16 at 23:39:53, Łukasz Anforowicz wrote:
> > On 2016/02/16 22:17:29, dcheng wrote:
> > > I would have expected name == uniqueName for most of these test cases.
> > 
> > In product code, we have name == uniqueName for the main frame, but for all
> child frames this equality doesn't hold.
> > 
> > Additionally, there are new asserts that will complain when
uniqueName.isEmpty
> for a child frame (because of this we cannot have name == uniqueName in at
least
> some cases, like the first modified tests in
> content/browser/frame_host/frame_tree_unittest.cc).
> > 
> > That said, I guess it is ok to have name == uniqueName in
> FrameThrottlingTest.cpp and WebFrameTest.cpp.  I started making this change,
but
> I got hesitant - when I see/read |"name", "uniqueName"| I can reasonably guess
> what both of these arguments are, but it is not so obvious when reading
|"name",
> "name"|.
> > 
> > WDYT?
> 
> I don't know, it just seems weird to explicitly hardcode something in tests
> that's different from what actually happens. Maybe we need a test helper for
> createLocalChild / createRemoteChild: it'd probably be reasonable to add this
to
> FrameTestHelpers. createLocalChild() has a lot of arguments that are almost
> always use default values in tests.

Good idea for introducing a helper - this reduces a lot of
boilerplate/noise/duplication.  OTOH, this still won't do exactly what the
product code would do, because it is not possible to calculate a |uniqueName|
via FrameTree without having a local parent.  Still - cheating in one place with
a comment is probably better than having this spread across multiple tests.

Done... sort-of...

[dcheng, 2016-02-19 21:36:27]

https://codereview.chromium.org/1635873003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1635873003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:144: const AtomicString&
originalRequestedName,
Nit: this doesn't match the name in the declaration. I'd just call this name (to
avoid the conflict with requestedName below)

https://codereview.chromium.org/1635873003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/1635873003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp:239: // |uniqueName| is
normally calculated in a somewhat complicated way by
I think we should make a nominal effort at uniqueness at least =)

How about this:

static int counter = 0;

StringBuilder uniqueName;
uniqueName.append(name);
uniqueName.appendNumber(++counter);

return parent->createLocalChild(..., uniqueName.toString(), ...);

[dcheng, 2016-02-19 21:37:02]

https://codereview.chromium.org/1635873003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/1635873003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp:239: // |uniqueName| is
normally calculated in a somewhat complicated way by
Also: nit

Change "way by" to "way by the"

[Łukasz Anforowicz, 2016-02-19 22:10:09]

Daniel, can you take another look please?

https://codereview.chromium.org/1635873003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/1635873003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FrameTree.cpp:144: const AtomicString&
originalRequestedName,
On 2016/02/19 21:36:27, dcheng wrote:
> Nit: this doesn't match the name in the declaration. I'd just call this name
(to
> avoid the conflict with requestedName below)

Done (+ renamed the other |name| below to |uniqueName|).

https://codereview.chromium.org/1635873003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/1635873003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp:239: // |uniqueName| is
normally calculated in a somewhat complicated way by
On 2016/02/19 21:37:02, dcheng wrote:
> Also: nit
> 
> Change "way by" to "way by the"

Done.

https://codereview.chromium.org/1635873003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp:239: // |uniqueName| is
normally calculated in a somewhat complicated way by
On 2016/02/19 21:36:27, dcheng wrote:
> I think we should make a nominal effort at uniqueness at least =)
> 
> How about this:
> 
> static int counter = 0;
> 
> StringBuilder uniqueName;
> uniqueName.append(name);
> uniqueName.appendNumber(++counter);
> 
> return parent->createLocalChild(..., uniqueName.toString(), ...);

Done.

[dcheng, 2016-02-19 22:14:08]

lgtm

[Łukasz Anforowicz, 2016-02-22 14:59:35]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-02-22 14:59:35]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1635873003/#ps360001
(title: "Rebasing...")

[commit-bot: I haz the power, 2016-02-22 15:03:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1635873003/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1635873003/360001

[commit-bot: I haz the power, 2016-02-22 15:15:53]

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

[commit-bot: I haz the power, 2016-02-22 15:15:54]

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

[Łukasz Anforowicz, 2016-02-22 15:22:32]

Description was changed from

==========
Replicating WebFrame::uniqueName across renderers.

When a frame is navigated, it might be moved to another renderer
process.  During such move, the other renderer process will create a
local frame object (and the old renderer process will have a remote
frame object).  It is important that the new local frame object has the
same unique name as the frame used to have in the old renderer object.
If the unique name is not correctly replicated to the other renderer
process, then Layout Tests (i.e.
http/tests/security/cross-origin-appcache-allowed.html) fail when run in
site isolation mode (i.e. with --additional-drt-flag=--site-per-process
flag;  note that site isolation mode won't work without extra CLs like
crrev.com/1589643003).

This CL adds a |unique_name| field to the content::FrameReplicationState
struct and plumbs this value from the renderer, through the browser
and back to the renderers.

BUG=576969
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Replicating WebFrame::uniqueName across renderers.

When a frame is navigated, it might be moved to another renderer
process.  During such move, the other renderer process will create a
local frame object (and the old renderer process will have a remote
frame object).  It is important that the new local frame object has the
same unique name as the frame used to have in the old renderer object.
If the unique name is not correctly replicated to the other renderer
process, then Layout Tests (i.e.
http/tests/security/cross-origin-appcache-allowed.html) fail when run in
site isolation mode (i.e. with --additional-drt-flag=--site-per-process
flag;  note that site isolation mode won't work without extra CLs like
crrev.com/1589643003).

This CL adds a |unique_name| field to the content::FrameReplicationState
struct and plumbs this value from the renderer, through the browser
and back to the renderers.

BUG=576969
TBR=gene@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Łukasz Anforowicz, 2016-02-22 15:24:59]

lukasza@chromium.org changed reviewers:
+ gene@chromium.org

[Łukasz Anforowicz, 2016-02-22 15:25:00]

Gene, could you please take a look?  I am sorry for not realizing earlier that
components/printing/renderer/print_web_view_helper.cc needs a separate OWNERS
review, but I think that it might be ok to TBR this file, given that the changes
are mostly a mechanical follow-up to the addition of the uniqueName parameter.

[Łukasz Anforowicz, 2016-02-22 15:25:51]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-02-22 15:26:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1635873003/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1635873003/360001

[commit-bot: I haz the power, 2016-02-22 15:32:27]

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

[Łukasz Anforowicz, 2016-02-22 16:44:51]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-02-22 16:44:52]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1635873003/#ps380001
(title: "Fixing Android compile failure related to WebString initialization.")

[commit-bot: I haz the power, 2016-02-22 16:45:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1635873003/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1635873003/380001

[commit-bot: I haz the power, 2016-02-22 16:49:10]

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

[commit-bot: I haz the power, 2016-02-22 16:49:12]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2016-02-22 17:44:36]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-02-22 17:44:37]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1635873003/#ps400001
(title: "Rebasing...")

[commit-bot: I haz the power, 2016-02-22 17:45:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1635873003/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1635873003/400001

[commit-bot: I haz the power, 2016-02-22 19:26:41]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2016-02-22 19:28:23]

Description was changed from

==========
Replicating WebFrame::uniqueName across renderers.

When a frame is navigated, it might be moved to another renderer
process.  During such move, the other renderer process will create a
local frame object (and the old renderer process will have a remote
frame object).  It is important that the new local frame object has the
same unique name as the frame used to have in the old renderer object.
If the unique name is not correctly replicated to the other renderer
process, then Layout Tests (i.e.
http/tests/security/cross-origin-appcache-allowed.html) fail when run in
site isolation mode (i.e. with --additional-drt-flag=--site-per-process
flag;  note that site isolation mode won't work without extra CLs like
crrev.com/1589643003).

This CL adds a |unique_name| field to the content::FrameReplicationState
struct and plumbs this value from the renderer, through the browser
and back to the renderers.

BUG=576969
TBR=gene@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Replicating WebFrame::uniqueName across renderers.

When a frame is navigated, it might be moved to another renderer
process.  During such move, the other renderer process will create a
local frame object (and the old renderer process will have a remote
frame object).  It is important that the new local frame object has the
same unique name as the frame used to have in the old renderer object.
If the unique name is not correctly replicated to the other renderer
process, then Layout Tests (i.e.
http/tests/security/cross-origin-appcache-allowed.html) fail when run in
site isolation mode (i.e. with --additional-drt-flag=--site-per-process
flag;  note that site isolation mode won't work without extra CLs like
crrev.com/1589643003).

This CL adds a |unique_name| field to the content::FrameReplicationState
struct and plumbs this value from the renderer, through the browser
and back to the renderers.

BUG=576969
TBR=gene@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/464d869b9576471215c7e4a25137ac47061ade29
Cr-Commit-Position: refs/heads/master@{#376772}
==========

[commit-bot: I haz the power, 2016-02-22 19:28:25]

Patchset 21 (id:??) landed as
https://crrev.com/464d869b9576471215c7e4a25137ac47061ade29
Cr-Commit-Position: refs/heads/master@{#376772}