Parameterize PushMessagingBrowserTest to work with bubbles & infobars

Parameterize PushMessagingBrowserTest to work with bubbles & infobars

BUG=438758

[felt, 2015-06-02 05:59:14]

Patchset #1 (id:1) has been deleted

[felt, 2015-06-02 05:59:21]

Patchset #1 (id:20001) has been deleted

[felt, 2015-06-02 05:59:29]

Patchset #1 (id:40001) has been deleted

[felt, 2015-06-02 05:59:43]

Patchset #2 (id:80001) has been deleted

[felt, 2015-06-02 05:59:50]

Patchset #1 (id:60001) has been deleted

[felt, 2015-06-02 06:00:00]

Patchset #3 (id:140001) has been deleted

[felt, 2015-06-02 06:00:08]

Patchset #3 (id:160001) has been deleted

[felt, 2015-06-02 06:00:35]

felt@chromium.org changed reviewers:
+ johnme@chromium.org

[felt, 2015-06-02 06:00:48]

Hi John, PTAL?

[johnme, 2015-06-02 14:12:38]

Thanks, I left some comments :)

https://codereview.chromium.org/1153193013/diff/180001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1153193013/diff/180001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:82:
std::to_string(TestTimeouts::tiny_timeout().InMillisecondsF())),
Nit: base::IntToString seems more common.

https://codereview.chromium.org/1153193013/diff/180001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:94:
command_line->AppendSwitch(switches::kDisablePermissionsBubbles);
Since these tests only run on desktop (unfortunately, chrome_browsertests isn't
supported on Android), perhaps we don't need the parametrization, and could
always test with bubbles? Or do you think it's important to maintain test
coverage for infobars on desktop?

https://codereview.chromium.org/1153193013/diff/180001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:264:
bubble_manager->SetView(mock_bubble_view.get());
This is super-complicated, compared to 2 lines for infobars :-(

Can we simplify all this, by having a helper class akin to InfoBarResponder?

https://codereview.chromium.org/1153193013/diff/180001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:265:
mock_bubble_view->SetBrowserTest(true);
It seems kinda hacky that MockPermissionBubbleView::Show sometimes quits the
current message loop; to separate concerns, it would seem cleaner to pass a
RunLoop::QuitClosure as an opaque listener closure to MockPermissionBubbleView.

https://codereview.chromium.org/1153193013/diff/180001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:269:
content::RunMessageLoop();
Why are you running a message loop here? Note that RunScript -- when you pass an
out param so it uses content::ExecuteScriptAndExtractString --  already runs a
message loop until the page asynchronously replies using sendResultToTest or
sendErrorToTest.

https://codereview.chromium.org/1153193013/diff/180001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:275:
RunScript("requestNotificationPermission(" + tiny_timeout_ms_ + ");"));
requestNotificationPermission method doesn't take any parameters.

https://codereview.chromium.org/1153193013/diff/180001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:277:
EXPECT_TRUE(RunScript("getLastPermissionRequestResult();", &script_result));
getLastPermissionRequestResult takes a timeout parameter.

https://codereview.chromium.org/1153193013/diff/180001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:292: ASSERT_TRUE(
Last I checked, you couldn't use ASSERT_TRUE in helper methods, as the ASSERT
isn't able to terminate execution from a helper method. Should be fine to use
EXPECT_TRUE instead. Ditto above.

https://codereview.chromium.org/1153193013/diff/180001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:1176:
INSTANTIATE_TEST_CASE_P(PushMessagingBrowserTestWithParams,
Does this mean all the tests will run twice, even those which never request
permission? Seems like it'll make debugging test output more confusing.

https://codereview.chromium.org/1153193013/diff/180001/chrome/test/data/push_...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/1153193013/diff/180001/chrome/test/data/push_...
chrome/test/data/push_messaging/push_test.js:68: lastPermissionRequestResult =
permission;
What's the purpose of this change? Note that sendResultToTest is already an
asynchronous response mechanism.

Adding timeouts here seems to introduce unnecessary complexity and flakiness.
For example, I notice that 3 of the other patches in
https://code.google.com/p/chromium/issues/detail?id=438758 were reverted due to
flakiness :-o

[felt, 2015-06-03 01:56:18]

Closing CL in favor of https://codereview.chromium.org/1154943008/.