Linux native notifications: Add notify and close tests

Linux native notifications: Add notify and close tests

BUG=676220
R=peter@chromium.org
CC=thestig@chromium.org

Review-Url: https://codereview.chromium.org/2857293002
Cr-Commit-Position: refs/heads/master@{#469462}
Committed: https://chromium.googlesource.com/chromium/src/+/14fb44e3336230da008526506b10246140b5bde0

[Tom (Use chromium acct), 2017-05-04 00:08:34]

Patchset #2 (id:20001) has been deleted

[Tom (Use chromium acct), 2017-05-04 00:08:39]

Patchset #1 (id:1) has been deleted

[Tom (Use chromium acct), 2017-05-04 00:13:47]

Description was changed from

==========
Linux native notifications: Add notify and close tests

BUG=676220
R=peter@chromium.org,thestig@chromium.org
==========

to

==========
Linux native notifications: Add notify and close tests

BUG=676220
R=peter@chromium.org
CC=thestig@chromium.org
==========

[Tom (Use chromium acct), 2017-05-04 00:13:48]

thomasanderson@google.com changed reviewers:
- thestig@chromium.org

[Peter Beverloo, 2017-05-04 14:43:06]

lgtm

https://codereview.chromium.org/2857293002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc
(right):

https://codereview.chromium.org/2857293002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:28:
using testing::StrictMock;
nit: move outside of the anonymous namespace

https://codereview.chromium.org/2857293002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:53:
EXPECT_TRUE(reader.PopString(&str));
nit: this method expects data in a very particular order, so it would be useful
to link to the documentation.

https://developer.gnome.org/notification-spec/#command-notify

https://codereview.chromium.org/2857293002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:114:
base::UTF8ToUTF16("Notifier's Name"), url, "id1",
Note that including "id1" (or any static value) means posting this notification
multiple times will cause it to replace the former one. If you do end up testing
notification replacement, enabling the caller to pass in the ID might be useful.

[Tom (Use chromium acct), 2017-05-04 18:15:36]

https://codereview.chromium.org/2857293002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc
(right):

https://codereview.chromium.org/2857293002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:28:
using testing::StrictMock;
On 2017/05/04 14:43:06, Peter Beverloo wrote:
> nit: move outside of the anonymous namespace

Done.

https://codereview.chromium.org/2857293002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:53:
EXPECT_TRUE(reader.PopString(&str));
On 2017/05/04 14:43:06, Peter Beverloo wrote:
> nit: this method expects data in a very particular order, so it would be
useful
> to link to the documentation.
> 
> https://developer.gnome.org/notification-spec/#command-notify

Done.

https://codereview.chromium.org/2857293002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:114:
base::UTF8ToUTF16("Notifier's Name"), url, "id1",
On 2017/05/04 14:43:06, Peter Beverloo wrote:
> Note that including "id1" (or any static value) means posting this
notification
> multiple times will cause it to replace the former one. If you do end up
testing
> notification replacement, enabling the caller to pass in the ID might be
useful.

Done.

[Tom (Use chromium acct), 2017-05-04 20:06:08]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2017-05-04 20:06:08]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2857293002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-04 20:06:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 20:54:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493928368398440,
"parent_rev": "f10490bd9e23a11a75977d6c53e2571248a61900", "commit_rev":
"14fb44e3336230da008526506b10246140b5bde0"}

[commit-bot: I haz the power, 2017-05-04 20:54:53]

Description was changed from

==========
Linux native notifications: Add notify and close tests

BUG=676220
R=peter@chromium.org
CC=thestig@chromium.org
==========

to

==========
Linux native notifications: Add notify and close tests

BUG=676220
R=peter@chromium.org
CC=thestig@chromium.org

Review-Url: https://codereview.chromium.org/2857293002
Cr-Commit-Position: refs/heads/master@{#469462}
Committed:
https://chromium.googlesource.com/chromium/src/+/14fb44e3336230da008526506b10...
==========

[commit-bot: I haz the power, 2017-05-04 20:54:57]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/14fb44e3336230da008526506b10...

[lijeffrey, 2017-05-08 22:05:06]

On 2017/05/04 20:54:57, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:80001) as
>
https://chromium.googlesource.com/chromium/src/+/14fb44e3336230da008526506b10...

Findit suggests this CL to have introduced a flaky test
"NotificationPlatformBridgeLinuxTest.NotifyAndCloseFormat" according to analysis
at
https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV...

Can someone please help verify?

Thanks,
Jeff