Issue 2888303004: Minimize the delegate dependencies for non persistent notifications.

Miguel Garcia

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-18 17:39:57 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888303004/1

3 years, 7 months ago (2017-05-18 17:41:26 UTC) #2

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-18 18:01:25 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/371163) chromeos_amd64-generic_chromium_compile_only_ng on ...

3 years, 7 months ago (2017-05-18 18:01:26 UTC) #4

Miguel Garcia

Description was changed from ========== Minimize the delegate dependencies for native extension notifications. Upgrade NotificationEventDispatcher ...

3 years, 7 months ago (2017-05-22 10:57:34 UTC) #5

Miguel Garcia

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-23 13:23:00 UTC) #6

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888303004/20001

3 years, 7 months ago (2017-05-23 13:23:18 UTC) #7

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-23 13:39:01 UTC) #8

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/432502)

3 years, 7 months ago (2017-05-23 13:39:02 UTC) #9

Miguel Garcia

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-25 14:45:13 UTC) #10

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888303004/40001

3 years, 7 months ago (2017-05-25 14:45:31 UTC) #11

Miguel Garcia

miguelg@chromium.org changed reviewers: + peter@chromium.org

3 years, 7 months ago (2017-05-25 14:54:17 UTC) #12

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-25 16:14:53 UTC) #14

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-25 16:14:54 UTC) #15

Peter Beverloo

Thanks! It may be useful to list a few of the follow-up CLs in your ...

3 years, 6 months ago (2017-05-31 17:52:43 UTC) #16

Miguel Garcia

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-01 16:59:04 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888303004/60001

3 years, 6 months ago (2017-06-01 16:59:35 UTC) #18

Miguel Garcia

miguelg@chromium.org changed reviewers: + avi@chromium.org, dewittj@chromium.org

3 years, 6 months ago (2017-06-01 17:00:52 UTC) #19

Miguel Garcia

Thanks Peter! Adding the remaining OWNERS avi for the file in content/public dewittj for the ...

3 years, 6 months ago (2017-06-01 17:00:54 UTC) #20

Thanks Peter!

 Adding the remaining OWNERS

avi for the file in content/public
dewittj for the changes in extensions

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

https://codereview.chromium.org/2888303004/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/native_notification_display_service.cc:98: // an
event after the notificaiton has been displayed.
On 2017/05/31 17:52:42, Peter Beverloo wrote:
> notificaiton -> notification

Done.

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

https://codereview.chromium.org/2888303004/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/non_persistent_notification_handler.cc:23: void
NonPersistentNotificationHandler::OnClick(
On 2017/05/31 17:52:42, Peter Beverloo wrote:
> Should OnShow() be added to the handlers?

Not sure I get this, what would OnShow do?

https://codereview.chromium.org/2888303004/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/non_persistent_notification_handler.cc:31: // Non
persistent notifications don't allow buttons.
On 2017/05/31 17:52:42, Peter Beverloo wrote:
> nit: refer to https://notifications.spec.whatwg.org/#create-a-notification

Done.

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

https://codereview.chromium.org/2888303004/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:321: #if
BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
On 2017/05/31 17:52:42, Peter Beverloo wrote:
> Please add some justification about why we need this here. This will go away
> once the MCDS becomes a platform bridge right?

Even sooner, see https://codereview.chromium.org/2906883003/ two CLs ahead in
the chain :)
I will add the follow up patches to the CL description

https://codereview.chromium.org/2888303004/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:375: //
native notifications, use the NativeNotificationDelegate instead.
Now that I know for a fact this is the case I think I am just going to remove
the TODO.

On 2017/05/31 17:52:42, Peter Beverloo wrote:
> "... use the NND instead."
> 
> That should be done implicitly within the NPB for the MC right?

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

https://codereview.chromium.org/2888303004/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:149:
// with native notifications. crbug.com/714679
On 2017/05/31 17:52:42, Peter Beverloo wrote:
> TODO
> grammar
> bug number in TODO attribution

Done.

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:439: return;
On 2017/05/31 17:52:42, Peter Beverloo wrote:
> It may be good to describe when this case (and the cases on lines 410 and 425)
> can happen: when the user clicks on a notification for a renderer that has
since
> been closed.

Done.

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:449: for
(auto iter = notification_renderers_.begin();
On 2017/05/31 17:52:43, Peter Beverloo wrote:
> Assuming you go with the pair solution, include //base/stl_util.h and:
> 
>   base::EraseIf(non_persistent_notifications_,
>                 [renderer_id](const std::pair<int, int>& pair) {
>                     return pair.first == renderer_id;
>                 });

Acknowledged.

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
File content/browser/notifications/notification_event_dispatcher_impl.h (right):

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:25: //
TODO(miguelg) rename so it says Persistent.
On 2017/05/31 17:52:43, Peter Beverloo wrote:
> Out of interest, why? Wouldn't it make the API simpler if we'd just branch on
> NotificationIdGenerator::IsPersistentNotification()?
> 
> In either case, this should (a) be in the interface definition, not in the
> implementation, and (b) not have a blank line on l24.

You mean having a single Dispatch method with completely different
implementations depending on whether it's persistent or not? That could work but
it seems better to be explicit in a public API? I am happy to revisit this
anyway.
For now I will move the comment to the api and add "consider renaming" instead.

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:41: 
On 2017/05/31 17:52:43, Peter Beverloo wrote:
> nit: no line breaks in override blocks like this.

Done.

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:60: //
Notification real id -> non persistent id.
It just feels harder to read when both members of the pair are ints (also more
error prone). Would it be fine to keep both maps?

I changed the comment.

On 2017/05/31 17:52:43, Peter Beverlif you oo wrote:
> What is a "real" notification Id?
> 
> Since everything is keyed by the notification id, can we just have the
following
> struct?
> 
>   std::map<std::string, std::pair<int /* renderer_id */, int /*
> non_persistent_notification_id */>>

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_message_filter.cc:110: if
(platform_notification_shown_) {
On 2017/05/31 17:52:43, Peter Beverloo wrote:
> Just always call it? We'd bail out super early in std::remove() (in
> base::EraseIf) if the container is empty.

I'm fine with that, it's only that this is called a lot (on every navigation
etc.) so it seems the kind of thing we want to only do if needed in case the
logic ever grows for whatever reason.

https://codereview.chromium.org/2888303004/diff/40001/content/public/browser/...
File content/public/browser/notification_event_dispatcher.h (right):

https://codereview.chromium.org/2888303004/diff/40001/content/public/browser/...
content/public/browser/notification_event_dispatcher.h:64: int
non_persistent_id) = 0;
On 2017/05/31 17:52:43, Peter Beverloo wrote:
> We should only define methods in the //content/public/ directory that will be
> used by consumers outside of //content.
> 
> Both RegisterNonPersistentNotification and RendererGone are solely called by
the
> NotificationMessageFilter, so they can live in NotificationEventDispatcherImpl
> (which has a GetInstance() method of its own).

Done.

https://codereview.chromium.org/2888303004/diff/40001/content/public/browser/...
content/public/browser/notification_event_dispatcher.h:66: // Dispatch methods
for the different notification events.
On 2017/05/31 17:52:43, Peter Beverloo wrote:
> Please explain why these are different from the ones above. (I.e. what
> "non-persistent" means.)

Done.

https://codereview.chromium.org/2888303004/diff/40001/content/public/browser/...
content/public/browser/notification_event_dispatcher.h:76: virtual void
RendererGone(int renderer_id) = 0;
On 2017/05/31 17:52:43, Peter Beverloo wrote:
> nit: DidShutdownRenderer()
> 
> We're already super late in the process by the time we call this...

Sure, it cleans up state on the browser when the renderers are gone. Is it a
problem it's called very late in the renderer destruction process?

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-01 17:31:34 UTC) #21

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/467912)

3 years, 6 months ago (2017-06-01 17:31:35 UTC) #22

Peter Beverloo

lgtm % comments Thanks for pointing to the next few CLs so that it's clear ...

3 years, 6 months ago (2017-06-01 17:56:28 UTC) #23

lgtm % comments

Thanks for pointing to the next few CLs so that it's clear to see where this is
headed.

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

https://codereview.chromium.org/2888303004/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/non_persistent_notification_handler.cc:23: void
NonPersistentNotificationHandler::OnClick(
On 2017/06/01 17:00:53, Miguel Garcia wrote:
> On 2017/05/31 17:52:42, Peter Beverloo wrote:
> > Should OnShow() be added to the handlers?
> 
> Not sure I get this, what would OnShow do?

You're now special casing non-persistent notifications in
native_notification_display_service.cc so that we can dispatch the Show event.
That's a hack - we should just fire NotificationHandler::OnShow() and only
provide an implementation for the non-persistent notification handler.

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
File content/browser/notifications/notification_event_dispatcher_impl.h (right):

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:25: //
TODO(miguelg) rename so it says Persistent.
On 2017/06/01 17:00:53, Miguel Garcia wrote:
> On 2017/05/31 17:52:43, Peter Beverloo wrote:
> > Out of interest, why? Wouldn't it make the API simpler if we'd just branch
on
> > NotificationIdGenerator::IsPersistentNotification()?
> > 
> > In either case, this should (a) be in the interface definition, not in the
> > implementation, and (b) not have a blank line on l24.
> 
> You mean having a single Dispatch method with completely different
> implementations depending on whether it's persistent or not? That could work
but
> it seems better to be explicit in a public API? I am happy to revisit this
> anyway.
> For now I will move the comment to the api and add "consider renaming"
instead.

Ideally the embedder (i.e. //chrome) wouldn't have to care about whether it's a
persistent or a non-persistent notification, it'd just be "a notification".
Long-term.

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:60: //
Notification real id -> non persistent id.
On 2017/06/01 17:00:53, Miguel Garcia wrote:
> It just feels harder to read when both members of the pair are ints (also more
> error prone). Would it be fine to keep both maps?
> 
> I changed the comment.
> 
> On 2017/05/31 17:52:43, Peter Beverlif you&nbsp;oo wrote:
> > What is a "real" notification Id?
> > 
> > Since everything is keyed by the notification id, can we just have the
> following
> > struct?
> > 
> >   std::map<std::string, std::pair<int /* renderer_id */, int /*
> > non_persistent_notification_id */>>

"Peter Beverlif you oo wrote:"

lol how did you do that?

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:60: //
Notification real id -> non persistent id.
On 2017/06/01 17:00:53, Miguel Garcia wrote:
> It just feels harder to read when both members of the pair are ints (also more
> error prone). Would it be fine to keep both maps?
> 
> I changed the comment.

But much simpler code since we'd only have to care about a single map and can
drop the manual iteration. I have a reasonable preference but won't block you on
it :).

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_message_filter.cc:110: if
(platform_notification_shown_) {
On 2017/06/01 17:00:53, Miguel Garcia wrote:
> On 2017/05/31 17:52:43, Peter Beverloo wrote:
> > Just always call it? We'd bail out super early in std::remove() (in
> > base::EraseIf) if the container is empty.
> 
> I'm fine with that, it's only that this is called a lot (on every navigation
> etc.) so it seems the kind of thing we want to only do if needed in case the
> logic ever grows for whatever reason.

We have one NMF per renderer process, so it's much less frequent than once per
navigation. The actual overhead would pretty much come down to a CALL, post
compiler optimizations. You can drop the DCHECK anyway.

https://codereview.chromium.org/2888303004/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/native_notification_display_service.cc
(right):

https://codereview.chromium.org/2888303004/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/native_notification_display_service.cc:99: if
(notification_type == NotificationCommon::NON_PERSISTENT) {
This is where OnShow() could be fired. (Or maybe in the NDS since this class
doesn't know about it.)

https://codereview.chromium.org/2888303004/diff/60001/content/browser/notific...
File content/browser/notifications/notification_event_dispatcher_impl.h (right):

https://codereview.chromium.org/2888303004/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:49: int
non_persistent_id);
nit: please document these methods

https://codereview.chromium.org/2888303004/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:58: //
Notification Id -> non persistent Id.
non-persistent notification id. Probably should name these members
notification_renderer_id_ and notification_non_persistent_id_ as well if you do
decide to stick with them because it's super ambiguous.

https://codereview.chromium.org/2888303004/diff/60001/content/public/browser/...
File content/public/browser/notification_event_dispatcher.h (right):

https://codereview.chromium.org/2888303004/diff/60001/content/public/browser/...
content/public/browser/notification_event_dispatcher.h:63: // notification
events.
Just call these "non-persistent", please don't diverge terminology even further.

dewittj

chrome/browser/extensions/api/notifications lgtm https://codereview.chromium.org/2888303004/diff/60001/chrome/browser/extensions/api/notifications/notifications_api.cc File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/2888303004/diff/60001/chrome/browser/extensions/api/notifications/notifications_api.cc#newcode503 chrome/browser/extensions/api/notifications/notifications_api.cc:503: g_browser_process->notification_platform_bridge()) { Probably fine but it feels ...

3 years, 6 months ago (2017-06-01 18:44:45 UTC) #25

Miguel Garcia

Description was changed from ========== Minimize the delegate dependencies for non persistent notifications. Upgrade NotificationEventDispatcher ...

3 years, 6 months ago (2017-06-02 12:06:17 UTC) #26

Miguel Garcia

Thanks all! https://codereview.chromium.org/2888303004/diff/40001/content/browser/notifications/notification_event_dispatcher_impl.h File content/browser/notifications/notification_event_dispatcher_impl.h (right): https://codereview.chromium.org/2888303004/diff/40001/content/browser/notifications/notification_event_dispatcher_impl.h#newcode25 content/browser/notifications/notification_event_dispatcher_impl.h:25: // TODO(miguelg) rename so it says Persistent. ...

3 years, 6 months ago (2017-06-02 12:37:13 UTC) #27

Thanks all!

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
File content/browser/notifications/notification_event_dispatcher_impl.h (right):

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:25: //
TODO(miguelg) rename so it says Persistent.
On 2017/06/01 17:56:27, Peter Beverloo wrote:
> On 2017/06/01 17:00:53, Miguel Garcia wrote:
> > On 2017/05/31 17:52:43, Peter Beverloo wrote:
> > > Out of interest, why? Wouldn't it make the API simpler if we'd just branch
> on
> > > NotificationIdGenerator::IsPersistentNotification()?
> > > 
> > > In either case, this should (a) be in the interface definition, not in the
> > > implementation, and (b) not have a blank line on l24.
> > 
> > You mean having a single Dispatch method with completely different
> > implementations depending on whether it's persistent or not? That could work
> but
> > it seems better to be explicit in a public API? I am happy to revisit this
> > anyway.
> > For now I will move the comment to the api and add "consider renaming"
> instead.
> 
> Ideally the embedder (i.e. //chrome) wouldn't have to care about whether it's
a
> persistent or a non-persistent notification, it'd just be "a notification".
> Long-term.

Acknowledged.

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:60: //
Notification real id -> non persistent id.
On 2017/06/01 17:56:27, Peter Beverloo wrote:
> On 2017/06/01 17:00:53, Miguel Garcia wrote:
> > It just feels harder to read when both members of the pair are ints (also
more
> > error prone). Would it be fine to keep both maps?
> > 
> > I changed the comment.
> > 
> > On 2017/05/31 17:52:43, Peter Beverlif you&nbsp;oo wrote:
> > > What is a "real" notification Id?
> > > 
> > > Since everything is keyed by the notification id, can we just have the
> > following
> > > struct?
> > > 
> > >   std::map<std::string, std::pair<int /* renderer_id */, int /*
> > > non_persistent_notification_id */>>
> 
> "Peter Beverlif you oo wrote:"
> 
> lol how did you do that?
> 

I have no idea...

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/2888303004/diff/40001/content/browser/notific...
content/browser/notifications/notification_message_filter.cc:110: if
(platform_notification_shown_) {


As discussed offline I am keeping it with a more common name.

On 2017/06/01 17:56:27, Peter Beverloo wrote:
> On 2017/06/01 17:00:53, Miguel Garcia wrote:
> > On 2017/05/31 17:52:43, Peter Beverloo wrote:
> > > Just always call it? We'd bail out super early in std::remove() (in
> > > base::EraseIf) if the container is empty.
> > 
> > I'm fine with that, it's only that this is called a lot (on every navigation
> > etc.) so it seems the kind of thing we want to only do if needed in case the
> > logic ever grows for whatever reason.
> 
> We have one NMF per renderer process, so it's much less frequent than once per
> navigation. The actual overhead would pretty much come down to a CALL, post
> compiler optimizations. You can drop the DCHECK anyway.

https://codereview.chromium.org/2888303004/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/2888303004/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_api.cc:503:
g_browser_process->notification_platform_bridge()) {
On 2017/06/01 18:44:45, dewittj wrote:
> Probably fine but it feels strange to ask the browser process about the bridge
> and then never use it.

You are right, this is an artifact of how to decide to use native notifications
(which is needed for linux) we are trying to generalize it but I will be using
one single delegate before that so this particular check will go away.

https://codereview.chromium.org/2888303004/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/native_notification_display_service.cc
(right):

https://codereview.chromium.org/2888303004/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/native_notification_display_service.cc:99: if
(notification_type == NotificationCommon::NON_PERSISTENT) {
On 2017/06/01 17:56:27, Peter Beverloo wrote:
> This is where OnShow() could be fired. (Or maybe in the NDS since this class
> doesn't know about it.)

Yeah makes sense, I will add it to the next patch.

https://codereview.chromium.org/2888303004/diff/60001/content/browser/notific...
File content/browser/notifications/notification_event_dispatcher_impl.h (right):

https://codereview.chromium.org/2888303004/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:49: int
non_persistent_id);
On 2017/06/01 17:56:27, Peter Beverloo wrote:
> nit: please document these methods

Done.

https://codereview.chromium.org/2888303004/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:58: //
Notification Id -> non persistent Id.
On 2017/06/01 17:56:27, Peter Beverloo wrote:
> non-persistent notification id. Probably should name these members
> notification_renderer_id_ and notification_non_persistent_id_ as well if you
do
> decide to stick with them because it's super ambiguous.

Done.

https://codereview.chromium.org/2888303004/diff/60001/content/public/browser/...
File content/public/browser/notification_event_dispatcher.h (right):

https://codereview.chromium.org/2888303004/diff/60001/content/public/browser/...
content/public/browser/notification_event_dispatcher.h:63: // notification
events.
On 2017/06/01 17:56:27, Peter Beverloo wrote:
> Just call these "non-persistent", please don't diverge terminology even
further.

Done.

Miguel Garcia

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, avi@chromium.org, dewittj@chromium.org Link ...

3 years, 6 months ago (2017-06-02 12:37:19 UTC) #29

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/2888303004/80001

3 years, 6 months ago (2017-06-02 12:37:31 UTC) #30

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496407038707440, "parent_rev": "cc1d4221ecff47ed19ce3106987ad0e7afab788a", "commit_rev": "26f0196082493c3d5109f196828a8c5baf16005a"}

3 years, 6 months ago (2017-06-02 14:51:16 UTC) #31

commit-bot: I haz the power

Description was changed from ========== Minimize the delegate dependencies for non persistent notifications. Upgrade NotificationEventDispatcher ...

3 years, 6 months ago (2017-06-02 14:51:24 UTC) #32

commit-bot: I haz the power

Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/26f0196082493c3d5109f196828a8c5baf16005a

3 years, 6 months ago (2017-06-02 14:51:26 UTC) #33

michaelbai

A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2916383004/ by michaelbai@chromium.org. ...

3 years, 6 months ago (2017-06-02 23:25:59 UTC) #34

michaelbai

3 years, 6 months ago (2017-06-02 23:48:52 UTC) #35

Message was sent while issue was closed.

On 2017/06/02 23:25:59, michaelbai wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/2916383004/ by mailto:michaelbai@chromium.org.
> 
> The reason for reverting is: This broke the unit test 
> 
> PlatformNotificationServiceTest.DisplayPageDisplayedEvent
> 
> see 
>
https://uberchromegw.corp.google.com/i/chromium.android/builders/Android%20N5....

Revert failed

I will disable the test.

Issue 2888303004: Minimize the delegate dependencies for non persistent notifications. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix tests #

Patch Set 3 : more fixes #

Patch Set 4 : format #

Patch Set 5 : review #

Messages