|
|
Created:
3 years, 8 months ago by Tom (Use chromium acct) Modified:
3 years, 6 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. |
DescriptionRefactor NotificationPlatformBridgeLinux
This CL:
* Removes the dependency on gdbus from NPBL and uses //src/dbus instead
* Handle notifications asynchronously on a dedicated task runner
* Modify NativeNotificationDisplayService to allow async initialization
of the NotificationPlatformBridge (only on Linux)
BUG=676220
R=thestig@chromium.org
Review-Url: https://codereview.chromium.org/2821533003
Cr-Commit-Position: refs/heads/master@{#467563}
Committed: https://chromium.googlesource.com/chromium/src/+/700d8c729c8d08f6aa42f15343f297ff5cfdf393
Patch Set 1 #Patch Set 2 : Refactor #
Total comments: 6
Patch Set 3 : Address hashimoto@'s comments #Patch Set 4 : Fix icon file getting prematurely deleted #
Total comments: 13
Patch Set 5 : thestig@'s comments #Patch Set 6 : Rebase #Patch Set 7 : Rebase #
Total comments: 28
Patch Set 8 : address latest round of comments #Patch Set 9 : Rebase #Patch Set 10 : Merge some things from https://codereview.chromium.org/2828503005 #Patch Set 11 : Refactor #Patch Set 12 : add additional comments #
Total comments: 28
Patch Set 13 : address latest comments from thestig #
Total comments: 18
Patch Set 14 : address latest comments from peter #
Total comments: 4
Patch Set 15 : address latest comments from thestig #Patch Set 16 : fix compile errors #
Total comments: 6
Patch Set 17 : address latest comments from peter #
Total comments: 18
Patch Set 18 : Remove unused includes #Patch Set 19 : final comments #
Total comments: 2
Messages
Total messages: 66 (29 generated)
thestig@ ptal This CL uses //src/dbus instead of gdbus. It has a much nicer api and supports real base::Callback()s. I wish I would have known about this sooner :( Next cl will be to replace the IO thread this CL adds with a Notification thread
Also, sorry you have to review all this. You wouldn't have to do this if I just got it right the first time :S
thomasanderson@google.com changed reviewers: + hashimoto@chromium.org, peter@chromium.org
+peter for review +hashimoto for DEPS change
Description was changed from ========== Linux native notifications: Remove dependency on gdbus BUG=676220 R=thestig@chromium.org ========== to ========== Refactor NotificationPlatformBridgeLinux This CL: * Removes the dependency on gdbus from NPBL and uses //src/dbus instead * Adds a dedicated notification thread BUG=676220 R=thestig@chromium.org ==========
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Ugh, surprised to see there is still someone using dbus-glib in chrome. Glad to see you switching to our D-Bus lib :) DEPS lgtm https://codereview.chromium.org/2821533003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:152: // us in GSignalReceiver. Odd-indexed ones contain the button text. GSignalReceiver is no longer used. https://codereview.chromium.org/2821533003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:215: notification_proxy_->CallMethod(&method_call, -1, Please use dbus::ObjectProxy::TIMEOUT_USE_DEFAULT instead of -1 to make it easier to understand. The same goes for other CallMethod and CallMethodAndBlock. https://codereview.chromium.org/2821533003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:316: bus_options.dbus_task_runner = task_runner(); You don't need to fill dbus_task_runner here. dbus::Bus::Options::dbus_task_runner is used when the thread where dbus::Bus is constructed is different from the thread where D-Bus tasks are handled. In this case, dbus::Bus is constructed on the thread where D-Bus tasks are handled, so there is no need to specify the task runner.
https://codereview.chromium.org/2821533003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:152: // us in GSignalReceiver. Odd-indexed ones contain the button text. On 2017/04/17 03:17:58, hashimoto wrote: > GSignalReceiver is no longer used. Changed to OnActionInvoked() https://codereview.chromium.org/2821533003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:215: notification_proxy_->CallMethod(&method_call, -1, On 2017/04/17 03:17:58, hashimoto wrote: > Please use dbus::ObjectProxy::TIMEOUT_USE_DEFAULT instead of -1 to make it > easier to understand. > The same goes for other CallMethod and CallMethodAndBlock. Done. https://codereview.chromium.org/2821533003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:316: bus_options.dbus_task_runner = task_runner(); On 2017/04/17 03:17:58, hashimoto wrote: > You don't need to fill dbus_task_runner here. > dbus::Bus::Options::dbus_task_runner is used when the thread where dbus::Bus is > constructed is different from the thread where D-Bus tasks are handled. > In this case, dbus::Bus is constructed on the thread where D-Bus tasks are > handled, so there is no need to specify the task runner. Done.
On 2017/04/14 02:33:53, Tom Anderson wrote: > Also, sorry you have to review all this. You wouldn't have to do this if I just > got it right the first time :S Looking. I thought you knew about the top level dbus dir. Well, now you do.
https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/DEPS#ne... chrome/browser/DEPS:17: "+dbus", Move this down to chrome/browser/notifications/DEPS https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:32: class NotificationThreadSafe : public Notification { Seems a bit weird, as Notification is probably not thread safe. Is this really a not thread safe Notification plus a thread safe |icon_data| ? https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:105: NotificationPlatformBridgeLinux* npbl = new NotificationPlatformBridgeLinux(); With unique_ptr: auto npbl = base::MakeUnique<NotificationPlatformBridgeLinux>(); if (!npbl->BlockUntilReady()) return nullptr; return npbl.release(); https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:106: if (npbl->BlockUntilReady()) Does this block the UI thread? Maybe if you used an existing task runner, you won't have to wait for a new thread to start up? https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:226: int32_t expire_timeout = -1; const? kExpireTimeoutNever maybe? https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:39: std::unique_ptr<NativeNotificationThread> thread_; Is there an easy way to reuse an existing thread? We want to to avoid creating too many threads (there are too many threads!) when possible. e.g. device/power_save_blocker/power_save_blocker_x11.cc accesses DBus and it uses content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::FILE) for its |blocking_task_runner_| most of the time.
https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/DEPS#ne... chrome/browser/DEPS:17: "+dbus", On 2017/04/17 23:32:13, Lei Zhang wrote: > Move this down to chrome/browser/notifications/DEPS Done. https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:32: class NotificationThreadSafe : public Notification { On 2017/04/17 23:32:13, Lei Zhang wrote: > Seems a bit weird, as Notification is probably not thread safe. Is this really a > not thread safe Notification plus a thread safe |icon_data| ? Yeah I guess it is. However now that I think about it, I'm convinced we don't even need this. Even though gfx::Image() is not thread safe, we don't actually use it on the UI thread after calling Display() https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:105: NotificationPlatformBridgeLinux* npbl = new NotificationPlatformBridgeLinux(); On 2017/04/17 23:32:13, Lei Zhang wrote: > With unique_ptr: > > auto npbl = base::MakeUnique<NotificationPlatformBridgeLinux>(); > if (!npbl->BlockUntilReady()) > return nullptr; > return npbl.release(); Done. https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:106: if (npbl->BlockUntilReady()) On 2017/04/17 23:32:13, Lei Zhang wrote: > Does this block the UI thread? Maybe if you used an existing task runner, you > won't have to wait for a new thread to start up? We would still have to wait for the bus connection to initialize and create the proxy to /org/freedesktop/Notifications Only after that can we know if the system supports notifications In my testing, Create() usually took ~400us https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:226: int32_t expire_timeout = -1; On 2017/04/17 23:32:13, Lei Zhang wrote: > const? kExpireTimeoutNever maybe? Done. https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:39: std::unique_ptr<NativeNotificationThread> thread_; On 2017/04/17 23:32:14, Lei Zhang wrote: > Is there an easy way to reuse an existing thread? We want to to avoid creating > too many threads (there are too many threads!) when possible. > > e.g. device/power_save_blocker/power_save_blocker_x11.cc accesses DBus and it > uses > content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::FILE) for > its |blocking_task_runner_| most of the time. Create() needs to block, so I'm reluctant to put this on the file thread, since there could already be a bunch of tasks queued up which the UI thread would then have to wait for. But there _is_ a way to do this w/o blocking at all. * NPBL could queue up Display()/Close()/GetDisplayed() while the Dbus connection initializes. * Then if we find out the system supports notifications, post all of the queued up tasks to the notification thread * If it turns out the system doesn't support notifications, create a MessageCenterDisplayService on that on the current (UI) thread * After this point, just post/run the callback and skip the queuing We could also post to the file thread if we do this. thestig@ wdyt?
https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:39: std::unique_ptr<NativeNotificationThread> thread_; On 2017/04/18 01:41:11, Tom Anderson wrote: > On 2017/04/17 23:32:14, Lei Zhang wrote: > > Is there an easy way to reuse an existing thread? We want to to avoid creating > > too many threads (there are too many threads!) when possible. > > > > e.g. device/power_save_blocker/power_save_blocker_x11.cc accesses DBus and it > > uses > > content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::FILE) > for > > its |blocking_task_runner_| most of the time. > > Create() needs to block, so I'm reluctant to put this on the file thread, since > there could already be a bunch of tasks queued up which the UI thread would then > have to wait for. > > But there _is_ a way to do this w/o blocking at all. > > * NPBL could queue up Display()/Close()/GetDisplayed() while the Dbus connection > initializes. > * Then if we find out the system supports notifications, post all of the queued > up tasks to the notification thread > * If it turns out the system doesn't support notifications, create a > MessageCenterDisplayService on that on the current (UI) thread > * After this point, just post/run the callback and skip the queuing > > We could also post to the file thread if we do this. thestig@ wdyt? Yes, that sounds like how many other DBus clients work. I didn't look hard enough and just noticed the OnConnectedCallback to ConnectToSignal() is just a DCHECK. :-|
On 2017/04/18 02:39:58, Lei Zhang wrote: > https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... > File chrome/browser/notifications/notification_platform_bridge_linux.h (right): > > https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notific... > chrome/browser/notifications/notification_platform_bridge_linux.h:39: > std::unique_ptr<NativeNotificationThread> thread_; > On 2017/04/18 01:41:11, Tom Anderson wrote: > > On 2017/04/17 23:32:14, Lei Zhang wrote: > > > Is there an easy way to reuse an existing thread? We want to to avoid > creating > > > too many threads (there are too many threads!) when possible. > > > > > > e.g. device/power_save_blocker/power_save_blocker_x11.cc accesses DBus and > it > > > uses > > > content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::FILE) > > for > > > its |blocking_task_runner_| most of the time. > > > > Create() needs to block, so I'm reluctant to put this on the file thread, > since > > there could already be a bunch of tasks queued up which the UI thread would > then > > have to wait for. > > > > But there _is_ a way to do this w/o blocking at all. > > > > * NPBL could queue up Display()/Close()/GetDisplayed() while the Dbus > connection > > initializes. > > * Then if we find out the system supports notifications, post all of the > queued > > up tasks to the notification thread > > * If it turns out the system doesn't support notifications, create a > > MessageCenterDisplayService on that on the current (UI) thread > > * After this point, just post/run the callback and skip the queuing > > > > We could also post to the file thread if we do this. thestig@ wdyt? > Never mind it would be more complicated than that. I thought MessageCenterDisplayService was also a NotificationPlatformBridge that we could forward events to, but it's actually a NotificationDisplayService. This would require something like a dedicated NotificationDisplayServiceProxy that does the queueing/switching. peter@ can tell us if this is possible/worth it > Yes, that sounds like how many other DBus clients work. I didn't look hard > enough and just noticed the OnConnectedCallback to ConnectToSignal() is just a > DCHECK. :-| Something wrong with |on_signal_connected|? :3
This looks like a great clean-up, thanks! :) A few comments and questions. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:95: if (!npbl->BlockUntilReady()) We pretty much have to assume that the NotificationDisplayService is created on startup, which then creates the NotificationPlatformBridge. That means that this call would block start-up performance. That's super unfortunate, but we need to know whether the object proxy is available synchronously :/. Without refactoring the rest of the notification system to be asynchronous too (which will be a *huge* amount of work) I don't see a way around this. Could we check whether a proxy could be provided with the g_dbus_proxy_new_for_bus_sync() API? Or does that also (synchronously) create a thread under the hood? https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:142: writer.AppendString(""); One thing to consider for testing is that it may make sense to have a DBusNotificationBuilder of sorts that wraps this machinery. (At some point we should start adding some for sure.) https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:163: std::string id = base::SizeTToString(data->action_end++); micro nit: line is untouched, but consider marking this as const too. Or remove const from line 164. The reader might now think that there's a difference between these lines. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:202: WriteDataToTmpFile(notification.icon().As1xPNGBytes()); Do we have to extract the underlying RefCountedMemory on the main thread? This seems like the exact issue Robert commented on in https://codereview.chromium.org/2806203003/. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:220: base::Bind([](dbus::Response*) {})); Is there any benefit in using async calls if we're on a worker thread already? It seems like a potential footgun - what happens if (for whatever reason) Close() has to become synchronous and is called while an asynchronous Display() call is in progress - does the synchronous call flush any pending async calls too? https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:266: bool BlockUntilReady() { nit: document that it returns whether the proxy is available https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:322: bus_ = new dbus::Bus(bus_options); nit: make_scoped_refptr(new dbus::Bus(bus_options)) That'd clarify the fact that it's a refptr at call-site. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:333: auto on_signal_connected = [](const std::string&, const std::string&, question: suppose that we hit this DCHECK, what would we do about it? It's a broken assumption in the freedesktop notification service that we're connected to, so from that point of view it works for me. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:349: notifications_.clear(); nit: consider setting |notification_proxy_| to a nullptr here, so make it more explicit that line 348 closed it. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:471: : thread_(new NativeNotificationThread()) {} style nit: base::MakeUnique<NativeNotificationThread>() https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:473: NotificationPlatformBridgeLinux::~NotificationPlatformBridgeLinux() {} style nit: {} -> "= default;" https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:504: incognito, callback)); Will we be able to support this eventually, or not at all? Right now it's making a thread-jump to the thread where we post back saying "no", which is a bit unnecessary.
https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:95: if (!npbl->BlockUntilReady()) On 2017/04/18 18:09:28, Peter Beverloo wrote: > We pretty much have to assume that the NotificationDisplayService is created on > startup, which then creates the NotificationPlatformBridge. That means that this > call would block start-up performance. Is it really created at start up, or at first use? Or is first use during startup? > That's super unfortunate, but we need to know whether the object proxy is > available synchronously :/. Without refactoring the rest of the notification > system to be asynchronous too (which will be a *huge* amount of work) I don't > see a way around this. So what about having a NotificationDisplayServiceProxy to queue pending notifications? Code Search says 3 methods with 8 callers. It's not that hard, right? ;-)
https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:95: if (!npbl->BlockUntilReady()) On 2017/04/18 22:31:05, Lei Zhang wrote: > On 2017/04/18 18:09:28, Peter Beverloo wrote: > > We pretty much have to assume that the NotificationDisplayService is created > on > > startup, which then creates the NotificationPlatformBridge. That means that > this > > call would block start-up performance. > > Is it really created at start up, or at first use? Or is first use during > startup? On start-up for Android, but we're moving in that direction for Mac too. It's a requirement when we want the notifications to be able to outlive the browser process. > > That's super unfortunate, but we need to know whether the object proxy is > > available synchronously :/. Without refactoring the rest of the notification > > system to be asynchronous too (which will be a *huge* amount of work) I don't > > see a way around this. > > So what about having a NotificationDisplayServiceProxy to queue pending > notifications? Code Search says 3 methods with 8 callers. It's not that hard, > right? ;-) Yeah, Tom mentioned this. We'd have to queue all actions until after initialisation and execute them in order, but that's doable indeed. What is unfortunate is that we'd have two ways of ending up in the MessageCenterDisplayService, and I'm wondering if we can avoid that duplication by baking this in the NotificationDisplayService instead, but I haven't yet had time to think this through properly. I don't want to block you though. In either solution we'd need a queueing mechanism to enable asynchronous initialisation, so it's a safe path to head into :).
https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:95: if (!npbl->BlockUntilReady()) On 2017/04/18 18:09:28, Peter Beverloo wrote: > We pretty much have to assume that the NotificationDisplayService is created on > startup, which then creates the NotificationPlatformBridge. That means that this > call would block start-up performance. > > That's super unfortunate, but we need to know whether the object proxy is > available synchronously :/. Without refactoring the rest of the notification > system to be asynchronous too (which will be a *huge* amount of work) I don't > see a way around this. > > Could we check whether a proxy could be provided with the > g_dbus_proxy_new_for_bus_sync() API? Or does that also (synchronously) create a > thread under the hood? It doesn't create a thread but it does to synchronous IO. fwiw creating a thread takes a fraction of the time of IO Also, this would open up the possibility of the thread creation or the 2nd dbus connection failing for whatever reason, leaving us with a broken NPBL. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:95: if (!npbl->BlockUntilReady()) On 2017/04/18 22:42:06, Peter Beverloo wrote: > On 2017/04/18 22:31:05, Lei Zhang wrote: > > On 2017/04/18 18:09:28, Peter Beverloo wrote: > > > We pretty much have to assume that the NotificationDisplayService is created > > on > > > startup, which then creates the NotificationPlatformBridge. That means that > > this > > > call would block start-up performance. > > > > Is it really created at start up, or at first use? Or is first use during > > startup? > > On start-up for Android, but we're moving in that direction for Mac too. It's a > requirement when we want the notifications to be able to outlive the browser > process. > > > > That's super unfortunate, but we need to know whether the object proxy is > > > available synchronously :/. Without refactoring the rest of the notification > > > system to be asynchronous too (which will be a *huge* amount of work) I > don't > > > see a way around this. > > > > So what about having a NotificationDisplayServiceProxy to queue pending > > notifications? Code Search says 3 methods with 8 callers. It's not that hard, > > right? ;-) > > Yeah, Tom mentioned this. We'd have to queue all actions until after > initialisation and execute them in order, but that's doable indeed. > > What is unfortunate is that we'd have two ways of ending up in the > MessageCenterDisplayService, and I'm wondering if we can avoid that duplication > by baking this in the NotificationDisplayService instead, but I haven't yet had > time to think this through properly. > > I don't want to block you though. In either solution we'd need a queueing > mechanism to enable asynchronous initialisation, so it's a safe path to head > into :). I'll try creating a followup patch that does this just to ground the discussion https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:142: writer.AppendString(""); On 2017/04/18 18:09:28, Peter Beverloo wrote: > One thing to consider for testing is that it may make sense to have a > DBusNotificationBuilder of sorts that wraps this machinery. (At some point we > should start adding some for sure.) Acknowledged. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:163: std::string id = base::SizeTToString(data->action_end++); On 2017/04/18 18:09:28, Peter Beverloo wrote: > micro nit: line is untouched, but consider marking this as const too. Or remove > const from line 164. The reader might now think that there's a difference > between these lines. Done. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:202: WriteDataToTmpFile(notification.icon().As1xPNGBytes()); On 2017/04/18 18:09:28, Peter Beverloo wrote: > Do we have to extract the underlying RefCountedMemory on the main thread? This > seems like the exact issue Robert commented on in > https://codereview.chromium.org/2806203003/. I thought we didn't have to (see my previous comment about this), but it couldn't hurt to do a deep copy as the images are pretty small. done https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:220: base::Bind([](dbus::Response*) {})); On 2017/04/18 18:09:28, Peter Beverloo wrote: > Is there any benefit in using async calls if we're on a worker thread already? > It seems like a potential footgun - what happens if (for whatever reason) > Close() has to become synchronous and is called while an asynchronous Display() > call is in progress - does the synchronous call flush any pending async calls > too? Didn't think about that, but it's better safe than sorry. done https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:266: bool BlockUntilReady() { On 2017/04/18 18:09:28, Peter Beverloo wrote: > nit: document that it returns whether the proxy is available Done. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:322: bus_ = new dbus::Bus(bus_options); On 2017/04/18 18:09:28, Peter Beverloo wrote: > nit: make_scoped_refptr(new dbus::Bus(bus_options)) > > That'd clarify the fact that it's a refptr at call-site. Done. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:333: auto on_signal_connected = [](const std::string&, const std::string&, On 2017/04/18 18:09:28, Peter Beverloo wrote: > question: suppose that we hit this DCHECK, what would we do about it? I think the only way this could happen is if the notification server goes down while Chrome is running. I suppose if we had the proxy we could switch to using Chrome notifications, but I don't think this is a common enough case to worry about. At least Chrome won't crash if this happens. > It's a > broken assumption in the freedesktop notification service that we're connected > to, so from that point of view it works for me. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:349: notifications_.clear(); On 2017/04/18 18:09:27, Peter Beverloo wrote: > nit: consider setting |notification_proxy_| to a nullptr here, so make it more > explicit that line 348 closed it. Done. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:471: : thread_(new NativeNotificationThread()) {} On 2017/04/18 18:09:28, Peter Beverloo wrote: > style nit: base::MakeUnique<NativeNotificationThread>() Done. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:473: NotificationPlatformBridgeLinux::~NotificationPlatformBridgeLinux() {} On 2017/04/18 18:09:27, Peter Beverloo wrote: > style nit: {} -> "= default;" Done. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:504: incognito, callback)); On 2017/04/18 18:09:28, Peter Beverloo wrote: > Will we be able to support this eventually, or not at all? Right now it's making > a thread-jump to the thread where we post back saying "no", which is a bit > unnecessary. I've been putting off implementing this for a while and I'm not sure I understand how it's meant to work. My first impl involved iterating over |notifications_| and filtering the ones that matched |profile_id| and |incognito|. But when I clicked on "Get displayed notifications" in your notification generator, it listed ones from previous runs as well.
https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:504: incognito, callback)); On 2017/04/18 23:07:43, Tom Anderson wrote: > On 2017/04/18 18:09:28, Peter Beverloo wrote: > > Will we be able to support this eventually, or not at all? Right now it's > making > > a thread-jump to the thread where we post back saying "no", which is a bit > > unnecessary. > > I've been putting off implementing this for a while and I'm not sure I > understand how it's meant to work. My first impl involved iterating over > |notifications_| and filtering the ones that matched |profile_id| and > |incognito|. But when I clicked on "Get displayed notifications" in your > notification generator, it listed ones from previous runs as well. Assuming the parameters of Display(), it should return a vector of the |notification_id| values displayed for the {profile_id, incognito} profile that are still being available to the user, either on-screen or in a notification center.
The latest patch set merges in the progress from https://codereview.chromium.org/2828503005/ Notable differences: * Delete NotificationDisplayServiceProxy and merge its responsibility into NativeNotificiationDisplayService * Use the FILE thread instead of creating a notification thread
Description was changed from ========== Refactor NotificationPlatformBridgeLinux This CL: * Removes the dependency on gdbus from NPBL and uses //src/dbus instead * Adds a dedicated notification thread BUG=676220 R=thestig@chromium.org ========== to ========== Refactor NotificationPlatformBridgeLinux This CL: * Removes the dependency on gdbus from NPBL and uses //src/dbus instead * Handle notifications asynchronously on the FILE thread * Modify NativeNotificationDisplayService to allow async initialization of the NotificationPlatformBridge (only on Linux) BUG=676220 R=thestig@chromium.org ==========
Description was changed from ========== Refactor NotificationPlatformBridgeLinux This CL: * Removes the dependency on gdbus from NPBL and uses //src/dbus instead * Handle notifications asynchronously on the FILE thread * Modify NativeNotificationDisplayService to allow async initialization of the NotificationPlatformBridge (only on Linux) BUG=676220 R=thestig@chromium.org ========== to ========== Refactor NotificationPlatformBridgeLinux This CL: * Removes the dependency on gdbus from NPBL and uses //src/dbus instead * Handle notifications asynchronously on the FILE thread * Modify NativeNotificationDisplayService to allow async initialization of the NotificationPlatformBridge (only on Linux) BUG=676220 R=thestig@chromium.org ==========
ping
https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.cc:110: base::Unretained(this), notification_type, Since you already have a WeakPtrFactory, just use that? https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... File chrome/browser/notifications/native_notification_display_service.h (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.h:89: std::queue<base::Callback<void(void)>> actions_; base::Callback<void(void)> -> base::Closure https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:156: base::Unretained(this))); Do we know for sure if |this| will still be valid when Init() get called? https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:160: CleanUp(); Since NotificationPlatformBridgeLinux hangs off of g_browser_process, does it have time to actually clean up on shutdown? https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:168: const Notification& notification) { Can we check we are on the UI thread here? Ditto below. Really, try to DCHECK_CURRENTLY_ON() in all the methods when a class lives on multiple threads. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:179: base::Unretained(this), notification_type, notification_id, Similar question re: base::Unretained() usage. Ditto below. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:207: content::BrowserThread::PostTask( If you PostTaskAndReply(WithResult) the you may be able to avoid locking. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:234: OnConnected(notification_proxy_); So if |notification_proxy_| is NULL, then we definitely want to call OnConnected(false), but is this the right place to call OnConnected(true), or should we wait until both ConnectToSignal() calls below succeeds as well? https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:521: CleanUp(); Can this be CleanUpInternal() since it's already on the FILE thread? https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:42: void IsConnected(base::OnceCallback<void(bool)> callback); Maybe rename to CheckConnection? IsConnected sounds like it ought to return a bool. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:45: struct ResourceFile; Leave as it was, AKA sorted. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:58: void CleanUpInternal(); CleanUpOnFileThread? https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:95: base::Lock connected_lock_; Does this only protect |connected_| or |on_connected_callbacks_| as well? Not clear from just reading what's here.
https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.cc:110: base::Unretained(this), notification_type, On 2017/04/21 22:45:17, Lei Zhang wrote: > Since you already have a WeakPtrFactory, just use that? Done. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... File chrome/browser/notifications/native_notification_display_service.h (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.h:89: std::queue<base::Callback<void(void)>> actions_; On 2017/04/21 22:45:17, Lei Zhang wrote: > base::Callback<void(void)> -> base::Closure Done. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:156: base::Unretained(this))); On 2017/04/21 22:45:18, Lei Zhang wrote: > Do we know for sure if |this| will still be valid when Init() get called? Made NPBLI ref counted, so we can be sure it's still alive when Init() (and more importantly CleanUp()) is called https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:160: CleanUp(); On 2017/04/21 22:45:17, Lei Zhang wrote: > Since NotificationPlatformBridgeLinux hangs off of g_browser_process, does it > have time to actually clean up on shutdown? No. Changed this to DCHECK instead https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:168: const Notification& notification) { On 2017/04/21 22:45:18, Lei Zhang wrote: > Can we check we are on the UI thread here? Ditto below. Really, try to > DCHECK_CURRENTLY_ON() in all the methods when a class lives on multiple threads. Done. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:179: base::Unretained(this), notification_type, notification_id, On 2017/04/21 22:45:17, Lei Zhang wrote: > Similar question re: base::Unretained() usage. Ditto below. Done. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:207: content::BrowserThread::PostTask( On 2017/04/21 22:45:17, Lei Zhang wrote: > If you PostTaskAndReply(WithResult) the you may be able to avoid locking. Not sure I understand. The lock protects connected_ which is used before the PostTask https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:234: OnConnected(notification_proxy_); On 2017/04/21 22:45:17, Lei Zhang wrote: > So if |notification_proxy_| is NULL, then we definitely want to call > OnConnected(false), but is this the right place to call OnConnected(true), or > should we wait until both ConnectToSignal() calls below succeeds as well? Done. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:521: CleanUp(); On 2017/04/21 22:45:17, Lei Zhang wrote: > Can this be CleanUpInternal() since it's already on the FILE thread? Done. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:42: void IsConnected(base::OnceCallback<void(bool)> callback); On 2017/04/21 22:45:18, Lei Zhang wrote: > Maybe rename to CheckConnection? IsConnected sounds like it ought to return a > bool. Done. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:45: struct ResourceFile; On 2017/04/21 22:45:18, Lei Zhang wrote: > Leave as it was, AKA sorted. Done. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:58: void CleanUpInternal(); On 2017/04/21 22:45:18, Lei Zhang wrote: > CleanUpOnFileThread? Done. https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:95: base::Lock connected_lock_; On 2017/04/21 22:45:18, Lei Zhang wrote: > Does this only protect |connected_| or |on_connected_callbacks_| as well? Not > clear from just reading what's here. It protects both. Added a comment to clarify
Sorry for the delay, this CL has a lot of things I didn't know about. What about tests? https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.cc:19: #include "chrome/common/features.h" Unused? https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.cc:61: notification_bridge_connected_ = false; We can avoid the compile-time ifdef by adding a new method to NotificationPlatformBridge (SetReadyCallback?). The non-Linux implementations would just callback.Run(), whereas the Linux callback would post once complete. https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... File chrome/browser/notifications/native_notification_display_service.h (right): https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.h:75: void GetDisplayedNow(const DisplayedNotificationsCallback& callback); These three methods don't exist in the .cc https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.h:89: std::queue<base::Closure> actions_; nit: base::OnceClosure https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:128: notification_copy->set_icon(DeepCopyImage(notification_copy->icon())); nit: add a comment about why we do this https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:135: base::Bind(&NotificationPlatformBridgeLinuxImpl::DisplayNow, this, optional: "Now" doesn't convey the same meaning that "OnTaskRunner" as a suffix would have https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:552: else if (++connected_signals == 2) base::BarrierClosure solves this in a very nice way :) https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:8: #include "chrome/browser/notifications/notification_platform_bridge.h" #include "base/callback_forward.h" #include "base/macros.h" #include "base/memory/ref_counted.h" https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:34: scoped_refptr<NotificationPlatformBridgeLinuxImpl> impl_; Please keep DISALLOW_COPY_AND_ASSIGN()
https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.cc:19: #include "chrome/common/features.h" On 2017/04/25 15:16:55, Peter Beverloo wrote: > Unused? Used for ENABLE_NATIVE_NOTIFICATIONS Edit: and removed now that we're no longer using that https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.cc:61: notification_bridge_connected_ = false; On 2017/04/25 15:16:55, Peter Beverloo wrote: > We can avoid the compile-time ifdef by adding a new method to > NotificationPlatformBridge (SetReadyCallback?). The non-Linux implementations > would just callback.Run(), whereas the Linux callback would post once complete. Done. https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... File chrome/browser/notifications/native_notification_display_service.h (right): https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.h:75: void GetDisplayedNow(const DisplayedNotificationsCallback& callback); On 2017/04/25 15:16:55, Peter Beverloo wrote: > These three methods don't exist in the .cc Oops, removed https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.h:89: std::queue<base::Closure> actions_; On 2017/04/25 15:16:55, Peter Beverloo wrote: > nit: base::OnceClosure Done. https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:128: notification_copy->set_icon(DeepCopyImage(notification_copy->icon())); On 2017/04/25 15:16:55, Peter Beverloo wrote: > nit: add a comment about why we do this Done. https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:135: base::Bind(&NotificationPlatformBridgeLinuxImpl::DisplayNow, this, On 2017/04/25 15:16:55, Peter Beverloo wrote: > optional: "Now" doesn't convey the same meaning that "OnTaskRunner" as a suffix > would have Done. https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:552: else if (++connected_signals == 2) On 2017/04/25 15:16:55, Peter Beverloo wrote: > base::BarrierClosure solves this in a very nice way :) Done. Cool trick! https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:8: #include "chrome/browser/notifications/notification_platform_bridge.h" On 2017/04/25 15:16:55, Peter Beverloo wrote: > #include "base/callback_forward.h" > #include "base/macros.h" > #include "base/memory/ref_counted.h" Done. https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.h:34: scoped_refptr<NotificationPlatformBridgeLinuxImpl> impl_; On 2017/04/25 15:16:56, Peter Beverloo wrote: > Please keep DISALLOW_COPY_AND_ASSIGN() Done.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
On 2017/04/25 15:16:56, Peter Beverloo wrote: > Sorry for the delay, this CL has a lot of things I didn't know about. What about > tests? > I'll add the initial tests in a followup cl
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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:207: content::BrowserThread::PostTask( On 2017/04/24 20:46:14, Tom Anderson wrote: > On 2017/04/21 22:45:17, Lei Zhang wrote: > > If you PostTaskAndReply(WithResult) the you may be able to avoid locking. > > Not sure I understand. The lock protects connected_ which is used before the > PostTask To be clear, I was thinking if you always accessed |connected_| on a single thread, then you won't need locks.
https://codereview.chromium.org/2821533003/diff/290001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/290001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:561: OnConnectionInitializationFinished(false); If both signals fail to connect, won't this run twice and end up calling CleanUpOnTaskRunnerThread() twice? https://codereview.chromium.org/2821533003/diff/290001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:576: scoped_refptr<dbus::Bus> bus_; You may want to group the member variables below into a section that are accessed only on the UI thread, vs a section that's accessed on |task_runner_|.
Patchset #15 (id:310001) has been deleted
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...
https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:207: content::BrowserThread::PostTask( On 2017/04/25 18:55:57, Lei Zhang wrote: > On 2017/04/24 20:46:14, Tom Anderson wrote: > > On 2017/04/21 22:45:17, Lei Zhang wrote: > > > If you PostTaskAndReply(WithResult) the you may be able to avoid locking. > > > > Not sure I understand. The lock protects connected_ which is used before the > > PostTask > > To be clear, I was thinking if you always accessed |connected_| on a single > thread, then you won't need locks. Oh I see what you mean now. Done. https://codereview.chromium.org/2821533003/diff/290001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/290001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:561: OnConnectionInitializationFinished(false); On 2017/04/25 19:05:50, Lei Zhang wrote: > If both signals fail to connect, won't this run twice and end up calling > CleanUpOnTaskRunnerThread() twice? Yes but it should have been harmless. Added |cleanup_posted_| anyway for this. https://codereview.chromium.org/2821533003/diff/290001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:576: scoped_refptr<dbus::Bus> bus_; On 2017/04/25 19:05:50, Lei Zhang wrote: > You may want to group the member variables below into a section that are > accessed only on the UI thread, vs a section that's accessed on |task_runner_|. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 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: This issue passed the CQ dry run.
lgtm, but please get Lei's approval for the DBus interaction. https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... File chrome/browser/notifications/displayed_notifications_dispatch_callback.h (right): https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... chrome/browser/notifications/displayed_notifications_dispatch_callback.h:18: using NotificationBridgeReadyCallback = micro nit: make this a public member of NotificationPlatformBridge. The other callback is defined in this file because it's used throughout the Mac platform bridge, which has several components and an XPC service in it. https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge.h (right): https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge.h:50: // failed. |callback| may be called directly or from a posted task. nit: no double spaces. I don't mind what you do in the _linux.* files, but let's stay consistent elsewhere. https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:167: PostTaskToUiThread( nit: you are on the UI thread. The other bridges now have the behaviour that they invoke the |callback| synchronously when the state is known, so you could do the same here.
https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... File chrome/browser/notifications/displayed_notifications_dispatch_callback.h (right): https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... chrome/browser/notifications/displayed_notifications_dispatch_callback.h:18: using NotificationBridgeReadyCallback = On 2017/04/26 13:53:36, Peter Beverloo wrote: > micro nit: make this a public member of NotificationPlatformBridge. The other > callback is defined in this file because it's used throughout the Mac platform > bridge, which has several components and an XPC service in it. Done. https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge.h (right): https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge.h:50: // failed. |callback| may be called directly or from a posted task. On 2017/04/26 13:53:36, Peter Beverloo wrote: > nit: no double spaces. I don't mind what you do in the _linux.* files, but let's > stay consistent elsewhere. Done. https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:167: PostTaskToUiThread( On 2017/04/26 13:53:36, Peter Beverloo wrote: > nit: you are on the UI thread. The other bridges now have the behaviour that > they invoke the |callback| synchronously when the state is known, so you could > do the same here. Done.
CL description update - no longer on the FILE thread. https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:11: #include "base/run_loop.h" No longer needed? https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:16: #include "base/synchronization/lock.h" No longer needed.
Description was changed from ========== Refactor NotificationPlatformBridgeLinux This CL: * Removes the dependency on gdbus from NPBL and uses //src/dbus instead * Handle notifications asynchronously on the FILE thread * Modify NativeNotificationDisplayService to allow async initialization of the NotificationPlatformBridge (only on Linux) BUG=676220 R=thestig@chromium.org ========== to ========== Refactor NotificationPlatformBridgeLinux This CL: * Removes the dependency on gdbus from NPBL and uses //src/dbus instead * Handle notifications asynchronously on a dedicated task runner * Modify NativeNotificationDisplayService to allow async initialization of the NotificationPlatformBridge (only on Linux) BUG=676220 R=thestig@chromium.org ==========
On 2017/04/26 21:07:54, Lei Zhang wrote: > CL description update - no longer on the FILE thread. Done. https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:11: #include "base/run_loop.h" On 2017/04/26 21:07:54, Lei Zhang wrote: > No longer needed? Done. https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:16: #include "base/synchronization/lock.h" On 2017/04/26 21:07:54, Lei Zhang wrote: > No longer needed. Done.
https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:334: const std::string title = base::UTF16ToUTF8(notification->title()); Just merge this with the next line? Same for |message|. Fairly obvious what these are without the variable. https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:356: actions.push_back("default"); Add constants for "default" and "settings". https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:361: actions.push_back("Settings"); For later: does this need to be localized? https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:477: [](NotificationCommon::Operation operation, The very end of https://google.github.io/styleguide/cppguide.html#Lambda_expressions says to keep them short. Move this to an anonymous function? https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:483: DCHECK(profile_manager); No need. https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:574: // Members used only on the UI thead. thread https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:592: bool cleanup_posted_ = false; Wrong section? You are using this on the UI thread.
https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:334: const std::string title = base::UTF16ToUTF8(notification->title()); On 2017/04/26 21:27:53, Lei Zhang wrote: > Just merge this with the next line? Same for |message|. Fairly obvious what > these are without the variable. Done. https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:356: actions.push_back("default"); On 2017/04/26 21:27:53, Lei Zhang wrote: > Add constants for "default" and "settings". Done. https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:361: actions.push_back("Settings"); On 2017/04/26 21:27:53, Lei Zhang wrote: > For later: does this need to be localized? Yes https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:477: [](NotificationCommon::Operation operation, On 2017/04/26 21:27:53, Lei Zhang wrote: > The very end of > https://google.github.io/styleguide/cppguide.html#Lambda_expressions says to > keep them short. Move this to an anonymous function? Done. https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:483: DCHECK(profile_manager); On 2017/04/26 21:27:53, Lei Zhang wrote: > No need. Done. https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:574: // Members used only on the UI thead. On 2017/04/26 21:27:53, Lei Zhang wrote: > thread Done. https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:592: bool cleanup_posted_ = false; On 2017/04/26 21:27:53, Lei Zhang wrote: > Wrong section? You are using this on the UI thread. oh yeah, wrong section
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: linux_chromium_chromeos_ozone_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 hashimoto@chromium.org, peter@chromium.org Link to the patchset: https://codereview.chromium.org/2821533003/#ps410001 (title: "final 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": 410001, "attempt_start_ts": 1493262559129220, "parent_rev": "b38b5bbfa9a59152bdb56a528c772715845b3e37", "commit_rev": "700d8c729c8d08f6aa42f15343f297ff5cfdf393"}
Message was sent while issue was closed.
Description was changed from ========== Refactor NotificationPlatformBridgeLinux This CL: * Removes the dependency on gdbus from NPBL and uses //src/dbus instead * Handle notifications asynchronously on a dedicated task runner * Modify NativeNotificationDisplayService to allow async initialization of the NotificationPlatformBridge (only on Linux) BUG=676220 R=thestig@chromium.org ========== to ========== Refactor NotificationPlatformBridgeLinux This CL: * Removes the dependency on gdbus from NPBL and uses //src/dbus instead * Handle notifications asynchronously on a dedicated task runner * Modify NativeNotificationDisplayService to allow async initialization of the NotificationPlatformBridge (only on Linux) BUG=676220 R=thestig@chromium.org Review-Url: https://codereview.chromium.org/2821533003 Cr-Commit-Position: refs/heads/master@{#467563} Committed: https://chromium.googlesource.com/chromium/src/+/700d8c729c8d08f6aa42f15343f2... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:410001) as https://chromium.googlesource.com/chromium/src/+/700d8c729c8d08f6aa42f15343f2...
Message was sent while issue was closed.
gab@chromium.org changed reviewers: + gab@chromium.org
Message was sent while issue was closed.
Drive-by on task_scheduler API usage decisions on this CL, thanks for using it and hoping to serve you even better with improved APIs and documentation :) https://codereview.chromium.org/2821533003/diff/410001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/410001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:131: : task_runner_(base::CreateSingleThreadTaskRunnerWithTraits( Surveying existing use cases of base::CreateSingleThreadTaskRunnerWithTraits() and just making sure I this use case is appropriate now after we've improved the API a bit: Is the work happening on |task_runner_| thread-affine? Or is it merely thread-unsafe? base::CreateSequencedTaskRunnerWithTraits() provides thread-safety and is prefered when possible as it provides better scheduling opportunities. If it's truly thread-affine, please add a comment as to why. If it's merely hitting some DCHECKs because some leaf dependencies are hitting thread-affinity checks, many of those are incorrect right now and being updated to sequence checks -- let me know which check you're hitting if that's the case and we should add a TODO here to make this run on a sequence when that situation gets better shortly. Lastly, if this is truly truly thread-affine, is it okay for it to share a thread with other MayBlock() + TaskPriority::USER_BLOCKING work or should it have a dedicated thread. The current call results in the former. If this was unintentional and a dedicated thread is needed you'll want to specific SingleThreadTaskRunnerThreadMode::DEDICATED. https://codereview.chromium.org/2821533003/diff/410001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:275: bus_options.dbus_task_runner = task_runner_; From the documentation on bus.h this doesn't seem appropriate: // The thread servicing the task runner should meet the following // requirements: // 1) Already running. // 2) Has a MessageLoopForIO. scoped_refptr<base::SequencedTaskRunner> dbus_task_runner; 1) this isn't a dedicated thread. 2) It doesn't have a MessageLoopForIO. 3) "Already running" is kind of an overkill statement IMO, it's fairly rare for a task runner to not be "running" apart from early startup and shutdown. But I suppose it's dbus.h's documentation that needs updating?
Message was sent while issue was closed.
Hi Gabriel. Sorry for not getting back to you sooner On 2017/05/25 01:09:36, gab wrote: > Drive-by on task_scheduler API usage decisions on this CL, thanks for using it > and hoping to serve you even better with improved APIs and documentation :) > > https://codereview.chromium.org/2821533003/diff/410001/chrome/browser/notific... > File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): > > https://codereview.chromium.org/2821533003/diff/410001/chrome/browser/notific... > chrome/browser/notifications/notification_platform_bridge_linux.cc:131: : > task_runner_(base::CreateSingleThreadTaskRunnerWithTraits( > Surveying existing use cases of base::CreateSingleThreadTaskRunnerWithTraits() > and just making sure I this use case is appropriate now after we've improved the > API a bit: > > Is the work happening on |task_runner_| thread-affine? Or is it merely > thread-unsafe? base::CreateSequencedTaskRunnerWithTraits() provides > thread-safety and is prefered when possible as it provides better scheduling > opportunities. > > If it's truly thread-affine, please add a comment as to why. If it's merely > hitting some DCHECKs because some leaf dependencies are hitting thread-affinity > checks, many of those are incorrect right now and being updated to sequence > checks -- let me know which check you're hitting if that's the case and we > should add a TODO here to make this run on a sequence when that situation gets > better shortly. > The only reason this is a single threaded task runner is because I was hitting some DCHECKs in ::dbus. The tasks merely need to run in sequence. hashimoto@ can the DCHECKs for the dbus origin thread be changed to sequence checks? > Lastly, if this is truly truly thread-affine, is it okay for it to share a > thread with other MayBlock() + TaskPriority::USER_BLOCKING work or should it > have a dedicated thread. The current call results in the former. If this was > unintentional and a dedicated thread is needed you'll want to specific > SingleThreadTaskRunnerThreadMode::DEDICATED. > > https://codereview.chromium.org/2821533003/diff/410001/chrome/browser/notific... > chrome/browser/notifications/notification_platform_bridge_linux.cc:275: > bus_options.dbus_task_runner = task_runner_; > From the documentation on bus.h this doesn't seem appropriate: > > // The thread servicing the task runner should meet the following > // requirements: > // 1) Already running. > // 2) Has a MessageLoopForIO. > scoped_refptr<base::SequencedTaskRunner> dbus_task_runner; > > 1) this isn't a dedicated thread. > 2) It doesn't have a MessageLoopForIO. > 3) "Already running" is kind of an overkill statement IMO, it's fairly rare for > a task runner to not be "running" apart from early startup and shutdown. > > But I suppose it's dbus.h's documentation that needs updating?
Message was sent while issue was closed.
On 2017/06/01 23:00:49, Tom Anderson wrote: > Hi Gabriel. Sorry for not getting back to you sooner > > On 2017/05/25 01:09:36, gab wrote: > > Drive-by on task_scheduler API usage decisions on this CL, thanks for using it > > and hoping to serve you even better with improved APIs and documentation :) > > > > > https://codereview.chromium.org/2821533003/diff/410001/chrome/browser/notific... > > File chrome/browser/notifications/notification_platform_bridge_linux.cc > (right): > > > > > https://codereview.chromium.org/2821533003/diff/410001/chrome/browser/notific... > > chrome/browser/notifications/notification_platform_bridge_linux.cc:131: : > > task_runner_(base::CreateSingleThreadTaskRunnerWithTraits( > > Surveying existing use cases of base::CreateSingleThreadTaskRunnerWithTraits() > > and just making sure I this use case is appropriate now after we've improved > the > > API a bit: > > > > Is the work happening on |task_runner_| thread-affine? Or is it merely > > thread-unsafe? base::CreateSequencedTaskRunnerWithTraits() provides > > thread-safety and is prefered when possible as it provides better scheduling > > opportunities. > > > > If it's truly thread-affine, please add a comment as to why. If it's merely > > hitting some DCHECKs because some leaf dependencies are hitting > thread-affinity > > checks, many of those are incorrect right now and being updated to sequence > > checks -- let me know which check you're hitting if that's the case and we > > should add a TODO here to make this run on a sequence when that situation gets > > better shortly. > > > > The only reason this is a single threaded task runner is because I was hitting > some DCHECKs in ::dbus. The tasks merely need to run in sequence. hashimoto@ > can the DCHECKs for the dbus origin thread be changed to sequence checks? Got it thanks, can you add a code comment to that effect above this use of CreateSingleThreadTaskRunnerWithTraits()? I'm trying to make sure it's rare and justified to use that API. > > > Lastly, if this is truly truly thread-affine, is it okay for it to share a > > thread with other MayBlock() + TaskPriority::USER_BLOCKING work or should it > > have a dedicated thread. The current call results in the former. If this was > > unintentional and a dedicated thread is needed you'll want to specific > > SingleThreadTaskRunnerThreadMode::DEDICATED. > > > > > https://codereview.chromium.org/2821533003/diff/410001/chrome/browser/notific... > > chrome/browser/notifications/notification_platform_bridge_linux.cc:275: > > bus_options.dbus_task_runner = task_runner_; > > From the documentation on bus.h this doesn't seem appropriate: > > > > // The thread servicing the task runner should meet the following > > // requirements: > > // 1) Already running. > > // 2) Has a MessageLoopForIO. > > scoped_refptr<base::SequencedTaskRunner> dbus_task_runner; > > > > 1) this isn't a dedicated thread. > > 2) It doesn't have a MessageLoopForIO. > > 3) "Already running" is kind of an overkill statement IMO, it's fairly rare > for > > a task runner to not be "running" apart from early startup and shutdown. > > > > But I suppose it's dbus.h's documentation that needs updating? |