Issue 1549039: Enable support for notifications layout tests in test shell....

Issue 1549039: Enable support for notifications layout tests in test shell.... (Closed)

Created:
10 years, 8 months ago by John Gregg

Modified:
9 years ago

Reviewers:
Andrew T Wilson (Slow)

CC:
chromium-reviews, darin-cc_chromium.org, dpranke+watch_chromium.org, pam+watch_chromium.org

Base URL:
http://src.chromium.org/svn/trunk/src/

Visibility:
Public.

Description

Enable support for notifications layout tests in test shell. BUG=none TEST=notifications layout tests Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44372 Reverted. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44515

Patch Set 4 : '' #

Created: 10 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+158 lines, -1 line)			Patch
M	webkit/support/test_webkit_client.cc	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	webkit/tools/test_shell/layout_test_controller.h	View	1 2 3	1 chunk	+4 lines, -0 lines	0 comments	Download
M	webkit/tools/test_shell/layout_test_controller.cc	View	1 2 3	3 chunks	+13 lines, -0 lines	0 comments	Download
A	webkit/tools/test_shell/notification_presenter.h	View	1	1 chunk	+45 lines, -0 lines	0 comments	Download
A	webkit/tools/test_shell/notification_presenter.cc	View	1 2	1 chunk	+76 lines, -0 lines	0 comments	Download
M	webkit/tools/test_shell/test_shell.h	View	1 2 3	4 chunks	+5 lines, -1 line	0 comments	Download
M	webkit/tools/test_shell/test_shell.cc	View	1 2 3	3 chunks	+3 lines, -0 lines	0 comments	Download
M	webkit/tools/test_shell/test_shell.gypi	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	webkit/tools/test_shell/test_shell_webkit_init.h	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	webkit/tools/test_shell/test_webview_delegate.h	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	webkit/tools/test_shell/test_webview_delegate.cc	View	1 2 3	4 chunks	+7 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Andrew T Wilson (Slow)

LGTM http://codereview.chromium.org/1549039/diff/12001/13004 File webkit/tools/test_shell/notification_presenter.cc (right): http://codereview.chromium.org/1549039/diff/12001/13004#newcode22 webkit/tools/test_shell/notification_presenter.cc:22: // Make sure it's in the form of ...

10 years, 8 months ago (2010-04-13 18:18:29 UTC) #2

John Gregg

10 years, 8 months ago (2010-04-13 18:21:59 UTC) #3

http://codereview.chromium.org/1549039/diff/12001/13004
File webkit/tools/test_shell/notification_presenter.cc (right):

http://codereview.chromium.org/1549039/diff/12001/13004#newcode22
webkit/tools/test_shell/notification_presenter.cc:22: // Make sure it's in the
form of an origin
On 2010/04/13 18:18:29, Andrew T Wilson wrote:
> nit: period at end of comment.

Done.

http://codereview.chromium.org/1549039/diff/12001/13004#newcode23
webkit/tools/test_shell/notification_presenter.cc:23: GURL url(origin);
On 2010/04/13 18:18:29, Andrew T Wilson wrote:
> Do we care about doing something like DCHECK(url.is_valid())?

Not really, this is just test setup.  If the URL isn't valid it just won't help
the test, since it will only be checked for valid origins later through the
system.

http://codereview.chromium.org/1549039/diff/12001/13001
File webkit/tools/test_shell/test_shell.h (right):

http://codereview.chromium.org/1549039/diff/12001/13001#newcode395
webkit/tools/test_shell/test_shell.h:395: 
On 2010/04/13 18:18:29, Andrew T Wilson wrote:
> nit: why have a blank line here?

removed it

Expand Messages | Collapse Messages