Fix setting the top-level FrameTreeNode name for named windows.

Fix setting the top-level FrameTreeNode name for named windows.

BUG=486889
Committed: https://crrev.com/c9f51dd496f8f869302338b5641a01836cb83dd4
Cr-Commit-Position: refs/heads/master@{#329543}

[nasko, 2015-05-11 23:52:29]

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

[nasko, 2015-05-11 23:52:30]

Hey Charlie and Alex,
Can you take a look at this CL for me? It ensures we set the FrameTreeNode name
on the top-level when creating named windows.
Thanks in advance!
Nasko

[alexmos, 2015-05-12 21:22:30]

Just one quick question below.

https://codereview.chromium.org/1141453003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1141453003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl_browsertest.cc:694:
NavigateToURL(shell(), url);
nit: EXPECT_TRUE around NavigateToURL?  (note to self: need to find some time to
finish 425335...)

https://codereview.chromium.org/1141453003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl_browsertest.cc:708:
->GetFrameTree()->root()->frame_name());
Does the name also make it over to the new window's renderer?  Would it make
sense to check "window.name" in the new_shell, and/or try this with a
window.open for a cross-site URL?

[nasko, 2015-05-12 21:58:22]

https://codereview.chromium.org/1141453003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1141453003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl_browsertest.cc:694:
NavigateToURL(shell(), url);
On 2015/05/12 21:22:30, alexmos wrote:
> nit: EXPECT_TRUE around NavigateToURL?  (note to self: need to find some time
to
> finish 425335...)

Done.

https://codereview.chromium.org/1141453003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl_browsertest.cc:708:
->GetFrameTree()->root()->frame_name());
On 2015/05/12 21:22:30, alexmos wrote:
> Does the name also make it over to the new window's renderer?  Would it make
> sense to check "window.name" in the new_shell, and/or try this with a
> window.open for a cross-site URL?

The window.open with cross-site URL should be already covered by all of our
cross-process postMessage tests, right? I've added the window.name test, since
it is a useful thing to check.

[alexmos, 2015-05-12 23:04:31]

LGTM

https://codereview.chromium.org/1141453003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1141453003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl_browsertest.cc:708:
->GetFrameTree()->root()->frame_name());
On 2015/05/12 21:58:21, nasko wrote:
> On 2015/05/12 21:22:30, alexmos wrote:
> > Does the name also make it over to the new window's renderer?  Would it make
> > sense to check "window.name" in the new_shell, and/or try this with a
> > window.open for a cross-site URL?
> 
> The window.open with cross-site URL should be already covered by all of our
> cross-process postMessage tests, right? I've added the window.name test, since
> it is a useful thing to check.

Right, makes sense, thanks!

[nasko, 2015-05-12 23:24:54]

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

[commit-bot: I haz the power, 2015-05-12 23:25:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1141453003/20001

[Charlie Reis, 2015-05-12 23:45:24]

LGTM with minor questions.

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

https://codereview.chromium.org/1141453003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1207:
frame_tree_.root()->SetFrameName(params.main_frame_name);
Could anything in RFHM::Init ever depend on the name?  Just wondering if we need
to move this above it (or if there's a reason to wait until here).

https://codereview.chromium.org/1141453003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1586:
create_params.main_frame_name = base::UTF16ToUTF8(params.frame_name);
Why do FrameTreeNode and FrameHostMsg_DidChangeName have a std::string when
ViewHostMsg_CreateWindow_Params has a string16?  Seems like these should have
the same type.

A quick skim shows that WebViewGuest is the one of two places that consume the
string16, and it converts it to a std::string. 
BackgroundContentsService::CreateBackgroundContents is the other, and that
(through some plumbing) passes it to DictionaryValue::SetString, which supports
both string16 and std::string.

Maybe in a separate cleanup CL we can standardize on std::string?

[nasko, 2015-05-13 00:07:28]

No code changes, just answers to Charlie's questions.

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

https://codereview.chromium.org/1141453003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1207:
frame_tree_.root()->SetFrameName(params.main_frame_name);
On 2015/05/12 23:45:24, Charlie Reis wrote:
> Could anything in RFHM::Init ever depend on the name?  Just wondering if we
need
> to move this above it (or if there's a reason to wait until here).

SetFrameName actually accesses render_manager_ and sends notifications to all
proxies in it. It sounds incorrect to access an object that hasn't been
initialized yet. Alex and I tried to reason through whether we need the name
during Init and it doesn't seem we do, so decided to leave it this way.

https://codereview.chromium.org/1141453003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1586:
create_params.main_frame_name = base::UTF16ToUTF8(params.frame_name);
On 2015/05/12 23:45:24, Charlie Reis wrote:
> Why do FrameTreeNode and FrameHostMsg_DidChangeName have a std::string when
> ViewHostMsg_CreateWindow_Params has a string16?  Seems like these should have
> the same type.
> 
> A quick skim shows that WebViewGuest is the one of two places that consume the
> string16, and it converts it to a std::string. 
> BackgroundContentsService::CreateBackgroundContents is the other, and that
> (through some plumbing) passes it to DictionaryValue::SetString, which
supports
> both string16 and std::string.
> 
> Maybe in a separate cleanup CL we can standardize on std::string?

Yeah, a separate CL will be fine. I tried doing it on this one, but it did add a
bunch of unrelated changes, so decided to not upload that version.

[nasko, 2015-05-13 00:07:40]

The CQ bit was unchecked by nasko@chromium.org

[nasko, 2015-05-13 00:07:47]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-05-13 00:09:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1141453003/20001

[commit-bot: I haz the power, 2015-05-13 00:39:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-13 00:40:39]

Patchset 2 (id:??) landed as
https://crrev.com/c9f51dd496f8f869302338b5641a01836cb83dd4
Cr-Commit-Position: refs/heads/master@{#329543}