Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2849003002:
Linux native notifications: Add initial unit test (Closed)
|
Created:
3 years, 7 months ago by Tom (Use chromium acct) Modified:
3 years, 7 months ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux native notifications: Add initial unit test
BUG=676220
R=peter@chromium.org,thestig@chromium.org
Review-Url: https://codereview.chromium.org/2849003002
Cr-Commit-Position: refs/heads/master@{#468876}
Committed: https://chromium.googlesource.com/chromium/src/+/c5a3921e63a83c63cd256d9f69eaf837e445699a
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use TestBrowserThreadBundle #
Total comments: 11
Patch Set 3 : peter's comments #
Total comments: 7
Patch Set 4 : address thestig's comments #
Messages
Total messages: 23 (12 generated)
Patchset #1 (id:1) has been deleted
reviewers ptal
https://codereview.chromium.org/2849003002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2849003002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:46: NotificationPlatformBridgeLinux( This is for testing only, right? Add a comment? https://codereview.chromium.org/2849003002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc (right): https://codereview.chromium.org/2849003002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:20: class FakeTaskRunner : public base::SequencedTaskRunner { Do you want to use TestBrowserThreadBundle, so you can have a UI thread and such? Then you can drop some of the notification_platform_bridge_linux.cc changes. https://codereview.chromium.org/2849003002/diff/20001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2849003002/diff/20001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:3654: if (is_desktop_linux && enable_native_notifications) { Curiuos why the mac version isn't gated on similar logic.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/2849003002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2849003002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:46: NotificationPlatformBridgeLinux( On 2017/04/29 01:35:05, Lei Zhang (OOO May Day) wrote: > This is for testing only, right? Add a comment? Done. https://codereview.chromium.org/2849003002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc (right): https://codereview.chromium.org/2849003002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:20: class FakeTaskRunner : public base::SequencedTaskRunner { On 2017/04/29 01:35:05, Lei Zhang (OOO May Day) wrote: > Do you want to use TestBrowserThreadBundle, so you can have a UI thread and > such? Then you can drop some of the notification_platform_bridge_linux.cc > changes. Done. I should have just waited for you to reply on hangouts :) https://codereview.chromium.org/2849003002/diff/20001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2849003002/diff/20001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:3654: if (is_desktop_linux && enable_native_notifications) { On 2017/04/29 01:35:05, Lei Zhang (OOO May Day) wrote: > Curiuos why the mac version isn't gated on similar logic. The mac version doesn't need to be conditioned on enable_native_notifications because it doesn't have the additional use_dbus dependency the Linux version has. Also, ninja only includes files with _mac in the filename in the mac version, so it doesn't even need is_mac.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:207: cleanup_posted_ = true; Instead of introducing cleanup_posted_, can we just check whether bus_ is non-null? Posting the task will keep the instance alive until it runs, so it's a fairly good approximation. https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc (right): https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:24: arg3.Run("", "", true); It would be good to annotate what the parameters mean. arg3.Run("" /* interface_name */, "" /* signal_name */, true /* success */); https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:31: NotificationPlatformBridgeLinuxTest() {} nit: {} --> = default; Could just leave out this line too, the compiler will create it for you. https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:37: "/org/freedesktop/Notifications"; nit: usually we just put such constants in the anonymous namespace. https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:87: TEST_F(NotificationPlatformBridgeLinuxTest, SetUpAndTearDown) {} What sort of tests do you have in mind?
https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:207: cleanup_posted_ = true; On 2017/05/02 13:50:21, Peter Beverloo wrote: > Instead of introducing cleanup_posted_, can we just check whether bus_ is > non-null? Posting the task will keep the instance alive until it runs, so it's a > fairly good approximation. Done. FYI I added |cleanup_posted_| to catch the case where initialization failed and |bus_| was null, but we still want to be explicit about posting the cleanup task. But now that I think about it, if initialization failed I guess we don't need to cleanup anyway. Also, |bus_| is only supposed to be used on the task runner. CleanUp() must now unconditionally post CleanUpOnTaskRunner() to avoid a race condition where bus_ is null but Init() is in the task runner queue. I changed CleanUpOnTaskRunner() so that it is safe to call more than once. https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc (right): https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:24: arg3.Run("", "", true); On 2017/05/02 13:50:21, Peter Beverloo wrote: > It would be good to annotate what the parameters mean. > > arg3.Run("" /* interface_name */, "" /* signal_name */, true /* success */); Done. https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:31: NotificationPlatformBridgeLinuxTest() {} On 2017/05/02 13:50:21, Peter Beverloo wrote: > nit: {} --> = default; > > Could just leave out this line too, the compiler will create it for you. Done. https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:37: "/org/freedesktop/Notifications"; On 2017/05/02 13:50:21, Peter Beverloo wrote: > nit: usually we just put such constants in the anonymous namespace. Done. https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:87: TEST_F(NotificationPlatformBridgeLinuxTest, SetUpAndTearDown) {} On 2017/05/02 13:50:21, Peter Beverloo wrote: > What sort of tests do you have in mind? * Verify format of the "Notify" message * Verify format of the "Close" message * Make sure initialization fails on any of these: * Server doesn't have sufficient capabilities * Signal connection failure * /org/Freedesktop/Notifications does not exist * Add tests for GetDisplayedNotifications() * Server (user) closing notifications * Chrome closing notifications
lgtm https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc (right): https://codereview.chromium.org/2849003002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:87: TEST_F(NotificationPlatformBridgeLinuxTest, SetUpAndTearDown) {} On 2017/05/02 21:23:16, Tom Anderson wrote: > On 2017/05/02 13:50:21, Peter Beverloo wrote: > > What sort of tests do you have in mind? > > * Verify format of the "Notify" message > * Verify format of the "Close" message > * Make sure initialization fails on any of these: > * Server doesn't have sufficient capabilities > * Signal connection failure > * /org/Freedesktop/Notifications does not exist > * Add tests for GetDisplayedNotifications() > * Server (user) closing notifications > * Chrome closing notifications Perfect :) https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:144: // to 0 and the object would be prematurely destructed. Thank you for the detailed comment!
The CQ bit was checked by thomasanderson@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:145: void InitAfterConstructor() { This can be Init(), and Init() can be InitOnTaskRunner()? To be more consistent with Display/DisplayOnTaskRunner? https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:289: if (!bus_) { May be good to mention this happens under normal conditions, and we only skip this if a test provided |bus_| already. https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc (right): https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:57: notification_bridge_linux_.reset( Can you use base::MakeUnique here?
The CQ bit was unchecked by thomasanderson@google.com
https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:145: void InitAfterConstructor() { On 2017/05/03 01:10:45, Lei Zhang wrote: > This can be Init(), and Init() can be InitOnTaskRunner()? To be more consistent > with Display/DisplayOnTaskRunner? Done. https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:289: if (!bus_) { On 2017/05/03 01:10:46, Lei Zhang wrote: > May be good to mention this happens under normal conditions, and we only skip > this if a test provided |bus_| already. Done. https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc (right): https://codereview.chromium.org/2849003002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc:57: notification_bridge_linux_.reset( On 2017/05/03 01:10:46, Lei Zhang wrote: > Can you use base::MakeUnique here? No because that's a private constructor
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2849003002/#ps100001 (title: "address thestig's comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493774542283420,
"parent_rev": "83455aa4765586a1ad9719a461f4b771d6de9b84", "commit_rev":
"c5a3921e63a83c63cd256d9f69eaf837e445699a"}
Message was sent while issue was closed.
Description was changed from ========== Linux native notifications: Add initial unit test BUG=676220 R=peter@chromium.org,thestig@chromium.org ========== to ========== Linux native notifications: Add initial unit test BUG=676220 R=peter@chromium.org,thestig@chromium.org Review-Url: https://codereview.chromium.org/2849003002 Cr-Commit-Position: refs/heads/master@{#468876} Committed: https://chromium.googlesource.com/chromium/src/+/c5a3921e63a83c63cd256d9f69ea... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c5a3921e63a83c63cd256d9f69ea... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
