Added plumbing to transport the frame name between RenderViewHost and the Webkit layer.

Added plumbing to transport the frame name between RenderViewHost and the Webkit layer.

Extended ViewMsg_New and ViewHostMsg_CreateWindow to have a new frame_name
parameter. This allows the RVH to know the initial name of the frame associated 
with its RenderView, and also to set the name of the frame when creating a new
RenderView.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49622

[rafaelw, 2010-06-10 22:48:05]

Mostly looks good.

http://codereview.chromium.org/2775003/diff/2001/3007
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/2775003/diff/2001/3007#newcode96
chrome/browser/renderer_host/render_view_host_delegate.h:96: // The passed
frame_name parameter is the _name_ parameter that was passed
nit: The passed frame_name parameter -> |frame_name|; _name_ -> name

http://codereview.chromium.org/2775003/diff/2001/3007#newcode484
chrome/browser/renderer_host/render_view_host_delegate.h:484: virtual string16
GetRenderViewFrameName() const { return string16(); }
This feels kinda wrong. I think providing frame_name to RVH::CreateRenderView
would be more in keeping with the existing code. Is there a reason to prefer
doing it this way? If so, please also just check with Darin or Brettw (they
don't need to review this entire CL -- I'd just wanted to know that they're ok
with this addition to RVHD).

http://codereview.chromium.org/2775003/diff/2001/3013
File chrome/browser/tab_contents/background_contents.h (right):

http://codereview.chromium.org/2775003/diff/2001/3013#newcode128
chrome/browser/tab_contents/background_contents.h:128: // The name of the frame
associated with this BC's RenderView (used when
frame -> top-level frame

http://codereview.chromium.org/2775003/diff/2001/3017
File chrome/browser/tab_contents/tab_contents_view.cc (right):

http://codereview.chromium.org/2775003/diff/2001/3017#newcode34
chrome/browser/tab_contents/tab_contents_view.cc:34: route_id,
tab_contents_->profile(), tab_contents_->GetSiteInstance(),
stack args, one per line.

http://codereview.chromium.org/2775003/diff/2001/3021
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/2775003/diff/2001/3021#newcode628
chrome/renderer/render_thread.cc:628: params.session_storage_namespace_id,
params.frame_name);
This param line is getting nuts, can you stack these vertically and have one per
line.

http://codereview.chromium.org/2775003/diff/2001/3022
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2775003/diff/2001/3022#newcode1603
chrome/renderer/render_view.cc:1603: params, &routing_id,
&cloned_session_storage_namespace_id));
start params on previous line. indent to align on second line with open paren.

http://codereview.chromium.org/2775003/diff/2001/3024
File chrome/test/render_view_test.cc (right):

http://codereview.chromium.org/2775003/diff/2001/3024#newcode108
chrome/test/render_view_test.cc:108: view_ = RenderView::Create(&render_thread_,
0, kOpenerId,
stack args, one per line.

[Andrew T Wilson (Slow), 2010-06-11 04:24:00]

Addressed review comments. Please take another look.

http://codereview.chromium.org/2775003/diff/2001/3007
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/2775003/diff/2001/3007#newcode484
chrome/browser/renderer_host/render_view_host_delegate.h:484: virtual string16
GetRenderViewFrameName() const { return string16(); }
On 2010/06/10 22:48:05, rafaelw wrote:
> This feels kinda wrong. I think providing frame_name to RVH::CreateRenderView
> would be more in keeping with the existing code.
I don't have a strong motivation or preference either way (this was the issue I
emailed you about). Darin didn't have time to look at this right now, but his
brief comments seemed to indicate that he agreed with you, so I'm taking your
suggestion and changing this to be a param passed to CreateRenderView() instead.

[rafaelw, 2010-06-11 06:22:50]

lgtm w/ additional formatting changes.

also, it's helpful as a reviewer if you respond to each review comment with at
least 'done' so i can make sure each comment is addressed.

http://codereview.chromium.org/2775003/diff/8002/11001
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/2775003/diff/8002/11001#newcode206
chrome/browser/extensions/extension_host.cc:206: profile_->GetRequestContext(),
string16());
move first arg to previous line. indent second arg or align with open paren.

http://codereview.chromium.org/2775003/diff/8002/11006
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/2775003/diff/8002/11006#newcode109
chrome/browser/renderer_host/render_view_host.h:109: URLRequestContextGetter*
request_context, const string16& frame_name);
move first arg to previous line. align second with open paren

http://codereview.chromium.org/2775003/diff/8002/11013
File chrome/browser/renderer_host/test/test_render_view_host.h (right):

http://codereview.chromium.org/2775003/diff/8002/11013#newcode170
chrome/browser/renderer_host/test/test_render_view_host.h:170:
URLRequestContextGetter* request_context, const string16& frame_name);
put first param on previous line. align second with open paren

http://codereview.chromium.org/2775003/diff/8002/11015
File chrome/browser/tab_contents/background_contents.h (right):

http://codereview.chromium.org/2775003/diff/8002/11015#newcode47
chrome/browser/tab_contents/background_contents.h:47: 
remove extra newline

http://codereview.chromium.org/2775003/diff/8002/11017
File chrome/browser/tab_contents/render_view_host_delegate_helper.cc (right):

http://codereview.chromium.org/2775003/diff/8002/11017#newcode66
chrome/browser/tab_contents/render_view_host_delegate_helper.cc:66: new
BackgroundContents(site, route_id);
remove formatting change.

http://codereview.chromium.org/2775003/diff/8002/11022
File chrome/browser/tab_contents/test_tab_contents.cc (right):

http://codereview.chromium.org/2775003/diff/8002/11022#newcode53
chrome/browser/tab_contents/test_tab_contents.cc:53:
profile()->GetRequestContext(), string16());
put first arg on previous line. indent second

http://codereview.chromium.org/2775003/diff/8002/11028
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/2775003/diff/8002/11028#newcode314
chrome/renderer/render_view.h:314: virtual WebKit::WebView* createView(
There's another implementor of WebViewClient in the test_shell project. It's
probably worth implementing the old & new signatures in this cl as well
(otherwise the canary bots will break when your webkit patch lands)

[Andrew T Wilson (Slow), 2010-06-12 02:44:20]

On 2010/06/11 06:22:50, rafaelw wrote:
> lgtm w/ additional formatting changes.
> 
> also, it's helpful as a reviewer if you respond to each review comment with at
> least 'done' so i can make sure each comment is addressed.

OK, will do in the future. I won't bother this time since I'm not looking for
another review pass.


> 
> http://codereview.chromium.org/2775003/diff/8002/11028#newcode314
> chrome/renderer/render_view.h:314: virtual WebKit::WebView* createView(
> There's another implementor of WebViewClient in the test_shell project. It's
> probably worth implementing the old & new signatures in this cl as well
> (otherwise the canary bots will break when your webkit patch lands)
Thanks for pointing that out. Since there's a default implementation of
createView(), it's hard to make sure I catch all references. I'll temporarily
remove the default impl and do a build before landing the upstream patch to see
if I miss any others.