|
|
Created:
3 years, 8 months ago by Tom (Use chromium acct) Modified:
3 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux native notifications: Support closing and updating notifications
BUG=676220
R=thestig@chromium.org,peter@chromium.org
Review-Url: https://codereview.chromium.org/2803873003
Cr-Commit-Position: refs/heads/master@{#462751}
Committed: https://chromium.googlesource.com/chromium/src/+/02fc962eaa3cd415db363f2289f8e8963a51130c
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address thestig@'s comments #
Total comments: 23
Patch Set 3 : update include guards #Patch Set 4 : Address peter@'s comments #
Total comments: 9
Patch Set 5 : Address sky and thestig's comments #Patch Set 6 : Add comment in scoped_gobject.h #Patch Set 7 : glib_signals.h -> glib_signal.h to fix gn gen --check #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 32 (16 generated)
Description was changed from ========== Linux native notifications: Support closing and updating notifications ========== to ========== Linux native notifications: Support closing and updating notifications BUG=676220 R=thestig@chromium.org,peter@chromium.org ==========
thomasanderson@google.com changed reviewers: + peter@chromium.org, thestig@chromium.org
Patchset #1 (id:1) has been deleted
peter@ and/or thestig@ ptal You can skip the changes in gtk_util.h and glib_util.h as I'll be moving those into a separate CL
https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:46: g_cancellable_cancel(cancellable); Thought this would need to be cancellable.get(). https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:101: NotificationData* data = FindNotificationData(notification_id, profile_id); Is this expected to never be NULL? i.e. We can safely assume NativeNotificationDisplayService::Close() will do the right thing. https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:49: std::string notification_id; Make the members set via ctor const? https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:51: bool is_incognito; This is set but never read from right now. Is this part of the complete impl TODO? https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:70: // Callback used by GLib when the "Notify" message completes. ... the first time. https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:80: // A std::set<std::unique_ptr<T>> doesn't work well because You'd need something like the ListValue::Find() impl to make that work. https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtkui/gtk_util.h (right): https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/ui/libgt... chrome/browser/ui/libgtkui/gtk_util.h:119: typedef ScopedGObject<GtkStyleContext> ScopedStyleContext; While we are at it: using ScopedStyleContext = ScopedGObject<GtkStyleContext>; https://codereview.chromium.org/2803873003/diff/20001/ui/base/glib/glib_util.h File ui/base/glib/glib_util.h (right): https://codereview.chromium.org/2803873003/diff/20001/ui/base/glib/glib_util.... ui/base/glib/glib_util.h:5: #ifndef UI_BASE_GLIB_GLIB_UTIL_H_ How about scoped_gobject.h?
https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:46: g_cancellable_cancel(cancellable); On 2017/04/06 03:59:54, Lei Zhang wrote: > Thought this would need to be cancellable.get(). ScopedGObject has "operator T*() { return obj_; }" https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:101: NotificationData* data = FindNotificationData(notification_id, profile_id); On 2017/04/06 03:59:54, Lei Zhang wrote: > Is this expected to never be NULL? i.e. We can safely assume > NativeNotificationDisplayService::Close() will do the right thing. Probably not; added a nullptr check. https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:49: std::string notification_id; On 2017/04/06 03:59:54, Lei Zhang wrote: > Make the members set via ctor const? Done. https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:51: bool is_incognito; On 2017/04/06 03:59:54, Lei Zhang wrote: > This is set but never read from right now. Is this part of the complete impl > TODO? It will be used by GetDisplayed() https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:70: // Callback used by GLib when the "Notify" message completes. On 2017/04/06 03:59:54, Lei Zhang wrote: > ... the first time. Done. https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:80: // A std::set<std::unique_ptr<T>> doesn't work well because On 2017/04/06 03:59:54, Lei Zhang wrote: > You'd need something like the ListValue::Find() impl to make that work. Acknowledged. https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtkui/gtk_util.h (right): https://codereview.chromium.org/2803873003/diff/20001/chrome/browser/ui/libgt... chrome/browser/ui/libgtkui/gtk_util.h:119: typedef ScopedGObject<GtkStyleContext> ScopedStyleContext; On 2017/04/06 03:59:54, Lei Zhang wrote: > While we are at it: > > using ScopedStyleContext = ScopedGObject<GtkStyleContext>; Done. https://codereview.chromium.org/2803873003/diff/20001/ui/base/glib/glib_util.h File ui/base/glib/glib_util.h (right): https://codereview.chromium.org/2803873003/diff/20001/ui/base/glib/glib_util.... ui/base/glib/glib_util.h:5: #ifndef UI_BASE_GLIB_GLIB_UTIL_H_ On 2017/04/06 03:59:54, Lei Zhang wrote: > How about scoped_gobject.h? Done.
lgtm
thomasanderson@google.com changed reviewers: + sky@chromium.org
+sky for ui/base/glib/scoped_gobject.h
https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:49: void OnNotifyComplete(GObject* source_object, nit: maybe move to the anonymous namespace around line 17? OnNotifyComplete is the sort of generic name that could cause multiple symbol definitions when added to the global scope. https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:61: user_data); We're putting quite some trust here in the data that the libgio is giving back to us. Since we can get the platform bridge from the global scope, you could simplify this by calling: g_browser_process->notification_platform_bridge()->OnNotifyComplete(user_data, value) Then cast user_data to NotificationData* in there, and verify that it's actually part of the UnorderedUniqueSet map. That feels a lot safer and drops the pointer to the NPBridgeLinux from each of the NotificationData instances. WDYT? https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:85: data->update_data.reset(new Notification(notification)); nit (from base/memory/ptr_util.h): data->update_data = base::MakeUnique<Notification>(notification); https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:92: notifications_.emplace(data, std::unique_ptr<NotificationData>(data)); base::WrapUnique(data) https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:94: ::OnNotifyComplete, data); It's rather confusing that we have two OnNotifyComplete methods in this file. Maybe rename the global scope one to NotifyCompleteReceiver? https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:121: data->cancellable.reset(nullptr); nit: no need for `nullptr` https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:149: base::UTF16ToUTF8(notification.message()).c_str(), nullptr, nullptr, -1); I know that you're just moving this, but are these safe? The std::string temporary variable to hold the UTF8 version of notification.title() might end up being destroyed before the call is made, because we're not holding on to a reference and rather only use the c_str(). Maybe extract to locals to be safe? const std::string title = base::UTF16ToUTF8(notification.title()); --> title.c_str() (in the call) https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:172: }); nit: while I'm a fan of STL, we're tied to a lookup performance of O(n) here anyway, so consider simplifying as: ========================= for (const auto& pair : notifications_) { NotificationData* data = pair.first; if (data->notification_id == notification_id && data->profile_id == profile_id) { return data; } } return nullptr; ========================= https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:63: NotificationCommon::Type update_type; Maybe initialize this as NotificationCommon::TYPE_MAX? It's a scalar and we're currently leaving it as uninitialized. Alternatively base::Optional<> would work too. https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:68: }; (You could decide to only forward declare the struct in the header, and define it in the implementation, if you're a fan of keeping the header files clean.) https://codereview.chromium.org/2803873003/diff/40001/ui/base/glib/scoped_gob... File ui/base/glib/scoped_gobject.h (right): https://codereview.chromium.org/2803873003/diff/40001/ui/base/glib/scoped_gob... ui/base/glib/scoped_gobject.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. Should probably add this to //ui/base/BUILD.gn? (Even though it's not a compilation unit, there was a thread on chromium-dev@ about missing files about a week ago.)
https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:49: void OnNotifyComplete(GObject* source_object, On 2017/04/06 17:56:57, Peter Beverloo wrote: > nit: maybe move to the anonymous namespace around line 17? OnNotifyComplete is > the sort of generic name that could cause multiple symbol definitions when added > to the global scope. Done. https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:61: user_data); On 2017/04/06 17:56:57, Peter Beverloo wrote: > We're putting quite some trust here in the data that the libgio is giving back > to us. > > Since we can get the platform bridge from the global scope, you could simplify > this by calling: > > g_browser_process->notification_platform_bridge()->OnNotifyComplete(user_data, > value) > > Then cast user_data to NotificationData* in there, and verify that it's actually > part of the UnorderedUniqueSet map. > > That feels a lot safer and drops the pointer to the NPBridgeLinux from each of > the NotificationData instances. WDYT? Done. https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:85: data->update_data.reset(new Notification(notification)); On 2017/04/06 17:56:57, Peter Beverloo wrote: > nit (from base/memory/ptr_util.h): > > data->update_data = base::MakeUnique<Notification>(notification); Done. https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:92: notifications_.emplace(data, std::unique_ptr<NotificationData>(data)); On 2017/04/06 17:56:57, Peter Beverloo wrote: > base::WrapUnique(data) Done. https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:94: ::OnNotifyComplete, data); On 2017/04/06 17:56:57, Peter Beverloo wrote: > It's rather confusing that we have two OnNotifyComplete methods in this file. > Maybe rename the global scope one to NotifyCompleteReceiver? Done. https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:121: data->cancellable.reset(nullptr); On 2017/04/06 17:56:57, Peter Beverloo wrote: > nit: no need for `nullptr` Done. https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:149: base::UTF16ToUTF8(notification.message()).c_str(), nullptr, nullptr, -1); On 2017/04/06 17:56:57, Peter Beverloo wrote: > I know that you're just moving this, but are these safe? > > The std::string temporary variable to hold the UTF8 version of > notification.title() might end up being destroyed before the call is made, > because we're not holding on to a reference and rather only use the c_str(). > > Maybe extract to locals to be safe? > > const std::string title = base::UTF16ToUTF8(notification.title()); > --> title.c_str() (in the call) Done. https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:172: }); On 2017/04/06 17:56:57, Peter Beverloo wrote: > nit: while I'm a fan of STL, we're tied to a lookup performance of O(n) here > anyway, so consider simplifying as: > > ========================= > for (const auto& pair : notifications_) { > NotificationData* data = pair.first; > if (data->notification_id == notification_id && > data->profile_id == profile_id) { > return data; > } > } > > return nullptr; > ========================= Done. https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:63: NotificationCommon::Type update_type; On 2017/04/06 17:56:57, Peter Beverloo wrote: > Maybe initialize this as NotificationCommon::TYPE_MAX? It's a scalar and we're > currently leaving it as uninitialized. > > Alternatively base::Optional<> would work too. Done. https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.h:68: }; On 2017/04/06 17:56:57, Peter Beverloo wrote: > (You could decide to only forward declare the struct in the header, and define > it in the implementation, if you're a fan of keeping the header files clean.) Done. https://codereview.chromium.org/2803873003/diff/40001/ui/base/glib/scoped_gob... File ui/base/glib/scoped_gobject.h (right): https://codereview.chromium.org/2803873003/diff/40001/ui/base/glib/scoped_gob... ui/base/glib/scoped_gobject.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/04/06 17:56:57, Peter Beverloo wrote: > Should probably add this to //ui/base/BUILD.gn? > > (Even though it's not a compilation unit, there was a thread on chromium-dev@ > about missing files about a week ago.) Done.
Thanks! lgtm
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm++ https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2803873003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:121: data->cancellable.reset(nullptr); On 2017/04/06 17:56:57, Peter Beverloo wrote: > nit: no need for `nullptr` Well, only with the newest ScopedGObject. https://codereview.chromium.org/2803873003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2803873003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:145: if (notifications_.find(data) == notifications_.end()) !base::ContainsKey(notifications_, data) ?
https://codereview.chromium.org/2803873003/diff/80001/ui/base/glib/scoped_gob... File ui/base/glib/scoped_gobject.h (right): https://codereview.chromium.org/2803873003/diff/80001/ui/base/glib/scoped_gob... ui/base/glib/scoped_gobject.h:17: // Remove the floating reference from |obj_| if it has one. Should reset() get this logic too? If not, why? https://codereview.chromium.org/2803873003/diff/80001/ui/base/glib/scoped_gob... ui/base/glib/scoped_gobject.h:34: g_object_unref(obj_); reset() ? https://codereview.chromium.org/2803873003/diff/80001/ui/base/glib/scoped_gob... ui/base/glib/scoped_gobject.h:44: Unref(); It's mildly confusing that you have both reset and Unref. How about putting the implementation of Unref() here and removing Unref?
https://codereview.chromium.org/2803873003/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2803873003/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:145: if (notifications_.find(data) == notifications_.end()) On 2017/04/06 19:59:33, Lei Zhang wrote: > !base::ContainsKey(notifications_, data) ? Done. https://codereview.chromium.org/2803873003/diff/80001/ui/base/glib/scoped_gob... File ui/base/glib/scoped_gobject.h (right): https://codereview.chromium.org/2803873003/diff/80001/ui/base/glib/scoped_gob... ui/base/glib/scoped_gobject.h:17: // Remove the floating reference from |obj_| if it has one. On 2017/04/06 20:44:46, sky wrote: > Should reset() get this logic too? If not, why? Yes it should! Thanks for catching this! https://codereview.chromium.org/2803873003/diff/80001/ui/base/glib/scoped_gob... ui/base/glib/scoped_gobject.h:34: g_object_unref(obj_); On 2017/04/06 20:44:46, sky wrote: > reset() ? Done. https://codereview.chromium.org/2803873003/diff/80001/ui/base/glib/scoped_gob... ui/base/glib/scoped_gobject.h:44: Unref(); On 2017/04/06 20:44:46, sky wrote: > It's mildly confusing that you have both reset and Unref. How about putting the > implementation of Unref() here and removing Unref? Can't unfortunately. We require a template override for GtkStyleContext https://cs.chromium.org/chromium/src/chrome/browser/ui/libgtkui/gtk_util.h?rc...
LGTM https://codereview.chromium.org/2803873003/diff/80001/ui/base/glib/scoped_gob... File ui/base/glib/scoped_gobject.h (right): https://codereview.chromium.org/2803873003/diff/80001/ui/base/glib/scoped_gob... ui/base/glib/scoped_gobject.h:44: Unref(); On 2017/04/06 22:28:57, Tom Anderson wrote: > On 2017/04/06 20:44:46, sky wrote: > > It's mildly confusing that you have both reset and Unref. How about putting > the > > implementation of Unref() here and removing Unref? > > Can't unfortunately. We require a template override for GtkStyleContext > https://cs.chromium.org/chromium/src/chrome/browser/ui/libgtkui/gtk_util.h?rc... Subtle! Please add a comment about that as it's easy to miss.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, peter@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2803873003/#ps120001 (title: "Add comment in scoped_gobject.h")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, peter@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2803873003/#ps130001 (title: "glib_signals.h -> glib_signal.h to fix gn gen --check")
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": 130001, "attempt_start_ts": 1491533194335440, "parent_rev": "0ac6602eb8a9152a55bf395bac1157ceb06fde71", "commit_rev": "b8ef436cc839157f38ba650ef71532b01123b880"}
CQ is committing da patch. Bot data: {"patchset_id": 130001, "attempt_start_ts": 1491533194335440, "parent_rev": "5987c4f6a7563c248ba4f78a5e8fb1167bac9613", "commit_rev": "02fc962eaa3cd415db363f2289f8e8963a51130c"}
Message was sent while issue was closed.
Description was changed from ========== Linux native notifications: Support closing and updating notifications BUG=676220 R=thestig@chromium.org,peter@chromium.org ========== to ========== Linux native notifications: Support closing and updating notifications BUG=676220 R=thestig@chromium.org,peter@chromium.org Review-Url: https://codereview.chromium.org/2803873003 Cr-Commit-Position: refs/heads/master@{#462751} Committed: https://chromium.googlesource.com/chromium/src/+/02fc962eaa3cd415db363f2289f8... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:130001) as https://chromium.googlesource.com/chromium/src/+/02fc962eaa3cd415db363f2289f8... |