Add browser test for notification action icons.

Add browser test for notification action icons.

Also exposes the --enable-notification-action-icons runtime flag.

BUG=581336
Committed: https://crrev.com/683791c3ed0d450fc0c311d8b99be2c40cd6f5ff
Cr-Commit-Position: refs/heads/master@{#375858}

[Michael van Ouwerkerk, 2016-02-11 16:39:48]

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

[Michael van Ouwerkerk, 2016-02-11 16:39:48]

John, could you take a look please?

[johnme, 2016-02-11 18:13:33]

Might have made sense as two CLs, but no matter. lgtm.

https://codereview.chromium.org/1693583002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/1693583002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_browsertest.cc:303:
EXPECT_EQ(1u, buttons.size());
Nit: Instead of changing this line, change the RunScript line to show a
buttonless notification, e.g.:

ASSERT_TRUE(RunScript("DisplayPersistentNotification('Some title', {})",
                                                     &script_result));

to make it easier to understand that this EXPECT_EQ is checking that the Site
Settings button is not listed in buttons.

https://codereview.chromium.org/1693583002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_browsertest.cc:476:
ASSERT_EQ(2u, notification.buttons().size());
Nit: this still doesn't check that the order of titles in buttons() matches the
order in JS. You may want to add that?

https://codereview.chromium.org/1693583002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1693583002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:70538: +  <int value="-1691281364"
label="enable-notification-action-icons"/>
This histogram is documented as "Chrome flags that lead to chrome restart on
ChromeOS". Is that accurate, and if so does this flag need to cause a restart?

[Michael van Ouwerkerk, 2016-02-12 11:04:43]

Thanks John!

https://codereview.chromium.org/1693583002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/1693583002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_browsertest.cc:303:
EXPECT_EQ(1u, buttons.size());
On 2016/02/11 18:13:33, johnme wrote:
> Nit: Instead of changing this line, change the RunScript line to show a
> buttonless notification, e.g.:
> 
> ASSERT_TRUE(RunScript("DisplayPersistentNotification('Some title', {})",
>                                                      &script_result));
> 
> to make it easier to understand that this EXPECT_EQ is checking that the Site
> Settings button is not listed in buttons.

Done.

https://codereview.chromium.org/1693583002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_browsertest.cc:476:
ASSERT_EQ(2u, notification.buttons().size());
On 2016/02/11 18:13:33, johnme wrote:
> Nit: this still doesn't check that the order of titles in buttons() matches
the
> order in JS. You may want to add that?

Done.

https://codereview.chromium.org/1693583002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1693583002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:70538: +  <int value="-1691281364"
label="enable-notification-action-icons"/>
On 2016/02/11 18:13:33, johnme wrote:
> This histogram is documented as "Chrome flags that lead to chrome restart on
> ChromeOS". Is that accurate, and if so does this flag need to cause a restart?

AboutFlagsHistogramTest.CheckHistograms requires this to be present. All flags
in about_flags.cc must have entries in LoginCustomFlags.

[Michael van Ouwerkerk, 2016-02-12 11:16:28]

mvanouwerkerk@chromium.org changed reviewers:
+ isherman@chromium.org, peter@chromium.org, pfeldman@chromium.org

[Michael van Ouwerkerk, 2016-02-12 11:16:28]

Peter, could you take a look please as owner of chrome/browser/notifications/ ?

Ilya, could you take a look please as owner of tools/metrics/histograms/ ?

Pavel, could you take a look please as owner of the various plumbing bits?

[Michael van Ouwerkerk, 2016-02-12 11:17:00]

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

[commit-bot: I haz the power, 2016-02-12 11:17:05]

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

[Peter Beverloo, 2016-02-12 11:47:24]

lgtm % description, thank you! :)

https://codereview.chromium.org/1693583002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1693583002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7980: +        Enable notification action
icons.
It would be good to clarify that this is in context of Web Notifications, since
we already support them for other notification types.

https://codereview.chromium.org/1693583002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/1693583002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_browsertest.cc:467:
RequestAndAcceptPermission();
ASSERT_NO_FATAL_FAILURE(GrantNotificationPermissionForTest());

There's no reason to exercise the infobar/bubble permission infrastructure for
this test.

[commit-bot: I haz the power, 2016-02-12 12:33:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-12 12:33:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Michael van Ouwerkerk, 2016-02-12 16:04:56]

Thanks Peter.

https://codereview.chromium.org/1693583002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1693583002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7980: +        Enable notification action
icons.
On 2016/02/12 11:47:24, Peter Beverloo wrote:
> It would be good to clarify that this is in context of Web Notifications,
since
> we already support them for other notification types.

Done.

https://codereview.chromium.org/1693583002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/1693583002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_browsertest.cc:467:
RequestAndAcceptPermission();
On 2016/02/12 11:47:24, Peter Beverloo wrote:
> ASSERT_NO_FATAL_FAILURE(GrantNotificationPermissionForTest());
> 
> There's no reason to exercise the infobar/bubble permission infrastructure for
> this test.

Done.

[Michael van Ouwerkerk, 2016-02-12 16:05:03]

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

[commit-bot: I haz the power, 2016-02-12 16:06:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693583002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693583002/40001

[commit-bot: I haz the power, 2016-02-12 18:19:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-12 18:19:09]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2016-02-12 22:59:48]

histograms.xml lgtm

[pfeldman, 2016-02-17 01:19:11]

public/web lgtm

[Michael van Ouwerkerk, 2016-02-17 10:30:18]

Thank you gentlemen.

[Michael van Ouwerkerk, 2016-02-17 10:30:30]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-02-17 10:30:30]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
peter@chromium.org, isherman@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1693583002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-02-17 10:30:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693583002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693583002/60001

[commit-bot: I haz the power, 2016-02-17 11:55:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-17 11:56:42]

Description was changed from

==========
Add browser test for notification action icons.

Also exposes the --enable-notification-action-icons runtime flag.

BUG=581336
==========

to

==========
Add browser test for notification action icons.

Also exposes the --enable-notification-action-icons runtime flag.

BUG=581336

Committed: https://crrev.com/683791c3ed0d450fc0c311d8b99be2c40cd6f5ff
Cr-Commit-Position: refs/heads/master@{#375858}
==========

[commit-bot: I haz the power, 2016-02-17 11:56:43]

Patchset 4 (id:??) landed as
https://crrev.com/683791c3ed0d450fc0c311d8b99be2c40cd6f5ff
Cr-Commit-Position: refs/heads/master@{#375858}