Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(105)

Issue 2821533003: Refactor NotificationPlatformBridgeLinux (Closed)

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.

Description

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/+/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
Unified diffs Side-by-side diffs Delta from patch set Stats (+658 lines, -493 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 2 chunks +7 lines, -8 lines 0 comments Download
A chrome/browser/notifications/DEPS View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/notifications/native_notification_display_service.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +22 lines, -0 lines 0 comments Download
M chrome/browser/notifications/native_notification_display_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +68 lines, -17 lines 0 comments Download
M chrome/browser/notifications/notification_platform_bridge.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/notifications/notification_platform_bridge_android.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/notifications/notification_platform_bridge_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/notifications/notification_platform_bridge_linux.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +8 lines, -76 lines 0 comments Download
M chrome/browser/notifications/notification_platform_bridge_linux.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +523 lines, -391 lines 2 comments Download
M chrome/browser/notifications/notification_platform_bridge_mac.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/notifications/notification_platform_bridge_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/notifications/stub_notification_platform_bridge.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/notifications/stub_notification_platform_bridge.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/common/features.gni View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 66 (29 generated)
Tom (Use chromium acct)
thestig@ ptal This CL uses //src/dbus instead of gdbus. It has a much nicer api ...
3 years, 8 months ago (2017-04-14 02:24:53 UTC) #1
Tom (Use chromium acct)
Also, sorry you have to review all this. You wouldn't have to do this if ...
3 years, 8 months ago (2017-04-14 02:33:53 UTC) #2
Tom (Use chromium acct)
+peter for review +hashimoto for DEPS change
3 years, 8 months ago (2017-04-14 18:26:13 UTC) #4
hashimoto
Ugh, surprised to see there is still someone using dbus-glib in chrome. Glad to see ...
3 years, 8 months ago (2017-04-17 03:17:58 UTC) #8
Tom (Use chromium acct)
https://codereview.chromium.org/2821533003/diff/60001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/60001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode152 chrome/browser/notifications/notification_platform_bridge_linux.cc:152: // us in GSignalReceiver. Odd-indexed ones contain the button ...
3 years, 8 months ago (2017-04-17 18:07:13 UTC) #9
Lei Zhang
On 2017/04/14 02:33:53, Tom Anderson wrote: > Also, sorry you have to review all this. ...
3 years, 8 months ago (2017-04-17 23:14:59 UTC) #10
Lei Zhang
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#newcode17 chrome/browser/DEPS:17: "+dbus", Move this down to chrome/browser/notifications/DEPS https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc ...
3 years, 8 months ago (2017-04-17 23:32:14 UTC) #11
Tom (Use chromium acct)
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#newcode17 chrome/browser/DEPS:17: "+dbus", On 2017/04/17 23:32:13, Lei Zhang wrote: > Move ...
3 years, 8 months ago (2017-04-18 01:41:11 UTC) #12
Lei Zhang
https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notifications/notification_platform_bridge_linux.h File chrome/browser/notifications/notification_platform_bridge_linux.h (right): https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notifications/notification_platform_bridge_linux.h#newcode39 chrome/browser/notifications/notification_platform_bridge_linux.h:39: std::unique_ptr<NativeNotificationThread> thread_; On 2017/04/18 01:41:11, Tom Anderson wrote: > ...
3 years, 8 months ago (2017-04-18 02:39:58 UTC) #13
Tom (Use chromium acct)
On 2017/04/18 02:39:58, Lei Zhang wrote: > https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notifications/notification_platform_bridge_linux.h > File chrome/browser/notifications/notification_platform_bridge_linux.h (right): > > https://codereview.chromium.org/2821533003/diff/100001/chrome/browser/notifications/notification_platform_bridge_linux.h#newcode39 ...
3 years, 8 months ago (2017-04-18 04:59:34 UTC) #14
Peter Beverloo
This looks like a great clean-up, thanks! :) A few comments and questions. https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notifications/notification_platform_bridge_linux.cc File ...
3 years, 8 months ago (2017-04-18 18:09:28 UTC) #15
Lei Zhang
https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode95 chrome/browser/notifications/notification_platform_bridge_linux.cc:95: if (!npbl->BlockUntilReady()) On 2017/04/18 18:09:28, Peter Beverloo wrote: > ...
3 years, 8 months ago (2017-04-18 22:31:05 UTC) #16
Peter Beverloo
https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode95 chrome/browser/notifications/notification_platform_bridge_linux.cc:95: if (!npbl->BlockUntilReady()) On 2017/04/18 22:31:05, Lei Zhang wrote: > ...
3 years, 8 months ago (2017-04-18 22:42:06 UTC) #17
Tom (Use chromium acct)
https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode95 chrome/browser/notifications/notification_platform_bridge_linux.cc:95: if (!npbl->BlockUntilReady()) On 2017/04/18 18:09:28, Peter Beverloo wrote: > ...
3 years, 8 months ago (2017-04-18 23:07:43 UTC) #18
Peter Beverloo
https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/150001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode504 chrome/browser/notifications/notification_platform_bridge_linux.cc:504: incognito, callback)); On 2017/04/18 23:07:43, Tom Anderson wrote: > ...
3 years, 8 months ago (2017-04-18 23:16:13 UTC) #19
Tom (Use chromium acct)
The latest patch set merges in the progress from https://codereview.chromium.org/2828503005/ Notable differences: * Delete NotificationDisplayServiceProxy ...
3 years, 8 months ago (2017-04-20 01:45:38 UTC) #20
Tom (Use chromium acct)
ping
3 years, 8 months ago (2017-04-21 17:13:38 UTC) #23
Lei Zhang
https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notifications/native_notification_display_service.cc File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notifications/native_notification_display_service.cc#newcode110 chrome/browser/notifications/native_notification_display_service.cc:110: base::Unretained(this), notification_type, Since you already have a WeakPtrFactory, just ...
3 years, 8 months ago (2017-04-21 22:45:18 UTC) #24
Tom (Use chromium acct)
https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notifications/native_notification_display_service.cc File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notifications/native_notification_display_service.cc#newcode110 chrome/browser/notifications/native_notification_display_service.cc:110: base::Unretained(this), notification_type, On 2017/04/21 22:45:17, Lei Zhang wrote: > ...
3 years, 8 months ago (2017-04-24 20:46:15 UTC) #25
Peter Beverloo
Sorry for the delay, this CL has a lot of things I didn't know about. ...
3 years, 8 months ago (2017-04-25 15:16:56 UTC) #26
Tom (Use chromium acct)
https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notifications/native_notification_display_service.cc File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2821533003/diff/270001/chrome/browser/notifications/native_notification_display_service.cc#newcode19 chrome/browser/notifications/native_notification_display_service.cc:19: #include "chrome/common/features.h" On 2017/04/25 15:16:55, Peter Beverloo wrote: > ...
3 years, 8 months ago (2017-04-25 18:35:21 UTC) #27
Tom (Use chromium acct)
On 2017/04/25 15:16:56, Peter Beverloo wrote: > Sorry for the delay, this CL has a ...
3 years, 8 months ago (2017-04-25 18:36:26 UTC) #29
Lei Zhang
https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode207 chrome/browser/notifications/notification_platform_bridge_linux.cc:207: content::BrowserThread::PostTask( On 2017/04/24 20:46:14, Tom Anderson wrote: > On ...
3 years, 8 months ago (2017-04-25 18:55:58 UTC) #33
Lei Zhang
https://codereview.chromium.org/2821533003/diff/290001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/290001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode561 chrome/browser/notifications/notification_platform_bridge_linux.cc:561: OnConnectionInitializationFinished(false); If both signals fail to connect, won't this ...
3 years, 8 months ago (2017-04-25 19:05:50 UTC) #34
Tom (Use chromium acct)
https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/250001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode207 chrome/browser/notifications/notification_platform_bridge_linux.cc:207: content::BrowserThread::PostTask( On 2017/04/25 18:55:57, Lei Zhang wrote: > On ...
3 years, 8 months ago (2017-04-26 03:12:28 UTC) #38
Peter Beverloo
lgtm, but please get Lei's approval for the DBus interaction. https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notifications/displayed_notifications_dispatch_callback.h File chrome/browser/notifications/displayed_notifications_dispatch_callback.h (right): https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notifications/displayed_notifications_dispatch_callback.h#newcode18 ...
3 years, 8 months ago (2017-04-26 13:53:36 UTC) #45
Tom (Use chromium acct)
https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notifications/displayed_notifications_dispatch_callback.h File chrome/browser/notifications/displayed_notifications_dispatch_callback.h (right): https://codereview.chromium.org/2821533003/diff/350001/chrome/browser/notifications/displayed_notifications_dispatch_callback.h#newcode18 chrome/browser/notifications/displayed_notifications_dispatch_callback.h:18: using NotificationBridgeReadyCallback = On 2017/04/26 13:53:36, Peter Beverloo wrote: ...
3 years, 8 months ago (2017-04-26 17:56:54 UTC) #46
Lei Zhang
CL description update - no longer on the FILE thread. https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode11 ...
3 years, 8 months ago (2017-04-26 21:07:54 UTC) #47
Tom (Use chromium acct)
On 2017/04/26 21:07:54, Lei Zhang wrote: > CL description update - no longer on the ...
3 years, 8 months ago (2017-04-26 21:13:30 UTC) #49
Lei Zhang
https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode334 chrome/browser/notifications/notification_platform_bridge_linux.cc:334: const std::string title = base::UTF16ToUTF8(notification->title()); Just merge this with ...
3 years, 8 months ago (2017-04-26 21:27:53 UTC) #50
Tom (Use chromium acct)
https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2821533003/diff/370001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode334 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 ...
3 years, 8 months ago (2017-04-26 21:56:18 UTC) #51
Lei Zhang
lgtm
3 years, 8 months ago (2017-04-26 22:03:01 UTC) #52
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2821533003/410001
3 years, 8 months ago (2017-04-27 03:10:02 UTC) #59
commit-bot: I haz the power
Committed patchset #19 (id:410001) as https://chromium.googlesource.com/chromium/src/+/700d8c729c8d08f6aa42f15343f297ff5cfdf393
3 years, 8 months ago (2017-04-27 03:17:16 UTC) #62
gab
Drive-by on task_scheduler API usage decisions on this CL, thanks for using it and hoping ...
3 years, 7 months ago (2017-05-25 01:09:36 UTC) #64
Tom Anderson
Hi Gabriel. Sorry for not getting back to you sooner On 2017/05/25 01:09:36, gab wrote: ...
3 years, 6 months ago (2017-06-01 23:00:49 UTC) #65
gab
3 years, 6 months ago (2017-06-02 19:52:30 UTC) #66
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?

Powered by Google App Engine
This is Rietveld 408576698