Issue 10831056: Port the render_view_host_manager_browsertest.cc to content_browsertests.

Issue 10831056: Port the render_view_host_manager_browsertest.cc to content_browsertests. (Closed)

Created:
8 years, 4 months ago by jam

Modified:
8 years, 4 months ago

Reviewers:
Charlie Reis

CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

More Reviews

Description

Port the render_view_host_manager_browsertest.cc to content_browsertests. BUG=90448 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148962

Patch Set 4 : #

Total comments: 6

Created: 8 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+351 lines, -494 lines)			Patch
M	chrome/browser_tests.isolate	View	1	3 chunks	+0 lines, -3 lines	0 comments	Download
M	chrome/chrome_tests.gypi	View		1 chunk	+0 lines, -1 line	0 comments	Download
D	chrome/test/data/click-noreferrer-links.html	View		1 chunk	+0 lines, -78 lines	0 comments	Download
D	chrome/test/data/navigate_opener.html	View		1 chunk	+0 lines, -12 lines	0 comments	Download
D	chrome/test/data/post_message.html	View		1 chunk	+0 lines, -29 lines	0 comments	Download
M	content/browser/renderer_host/render_view_host_manager_browsertest.cc	View	1 2 3	34 chunks	+271 lines, -374 lines	4 comments	Download
M	content/content_tests.gypi	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	content/shell/shell.h	View		4 chunks	+10 lines, -0 lines	0 comments	Download
M	content/shell/shell.cc	View		4 chunks	+28 lines, -3 lines	2 comments	Download
M	content/test/content_browser_test_utils.h	View	1 2	3 chunks	+22 lines, -0 lines	0 comments	Download
M	content/test/content_browser_test_utils.cc	View	1 2	1 chunk	+25 lines, -0 lines	0 comments	Download
A +	content/test/data/click-noreferrer-links.html	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A +	content/test/data/navigate_opener.html	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A +	content/test/data/post_message.html	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A +	content/test/data/title1.html	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A +	content/test/data/title2.html	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A +	content/test/data/title3.html	View		0 chunks	+-1 lines, --1 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Charlie Reis

LGTM. http://codereview.chromium.org/10831056/diff/5005/content/browser/renderer_host/render_view_host_manager_browsertest.cc File content/browser/renderer_host/render_view_host_manager_browsertest.cc (left): http://codereview.chromium.org/10831056/diff/5005/content/browser/renderer_host/render_view_host_manager_browsertest.cc#oldcode1087 content/browser/renderer_host/render_view_host_manager_browsertest.cc:1087: AddBlankTabAndShow(browser()); I guess this isn't needed? http://codereview.chromium.org/10831056/diff/5005/content/browser/renderer_host/render_view_host_manager_browsertest.cc File ...

8 years, 4 months ago (2012-07-30 16:40:01 UTC) #2

jam

8 years, 4 months ago (2012-07-30 17:03:50 UTC) #3

http://codereview.chromium.org/10831056/diff/5005/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(left):

http://codereview.chromium.org/10831056/diff/5005/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1087:
AddBlankTabAndShow(browser());
On 2012/07/30 16:40:01, creis wrote:
> I guess this isn't needed?

right. this was needed for chrome (which quits if the last Browser goes away),
but not content_shell

http://codereview.chromium.org/10831056/diff/5005/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

http://codereview.chromium.org/10831056/diff/5005/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:377: //
Should have swapped back and shown the new widnow again.
On 2012/07/30 16:40:01, creis wrote:
> nit: window

Done.

http://codereview.chromium.org/10831056/diff/5005/content/shell/shell.cc
File content/shell/shell.cc (right):

http://codereview.chromium.org/10831056/diff/5005/content/shell/shell.cc#newc...
content/shell/shell.cc:42: {
On 2012/07/30 16:40:01, creis wrote:
> This macro makes for an odd indent situation.  I'm ok either way, but should
the
> open brace be at zero indent in this case?

I'm not sure what's recommended in this case. when macros are used like this,
tabbing (and location of commas etc) usually doesn't/can't confirm to the style
guide.

Expand Messages | Collapse Messages