Use notification display service to collect persistent notifications.

Use notification display service to collect persistent notifications.

- Filter out non persistent notifications on PlatformNotificationService
- Fiter our notifications present in the database but not displayed

In order to keep the model simple there is no syncing of the database with
the new information, we stil rely on chrome restarts for that.

This CL also integrates https://codereview.chromium.org/2565323004/ authored by Peter Beverloo.

BUG=571056
Committed: https://crrev.com/fea8f2f0eba8a4b9e98281dd52f64a1241a96d9c
Cr-Commit-Position: refs/heads/master@{#438251}

[Miguel Garcia, 2016-11-24 16:47:41]

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

[commit-bot: I haz the power, 2016-11-24 16:48:05]

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

[Miguel Garcia, 2016-11-24 16:49:49]

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

[commit-bot: I haz the power, 2016-11-24 16:49:55]

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

[Miguel Garcia, 2016-11-24 16:51:38]

Patchset #1 (id:1) has been deleted

[Miguel Garcia, 2016-11-24 16:55:18]

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

[Miguel Garcia, 2016-11-24 16:55:18]

A few notes:

 The unit tests needs https://codereview.chromium.org/2528013002 We also have a
ui test so we might want to consider removing it so it's not blocking.

  I am not entirely sure about the new content/public file (whose methods are
implemented in content/browser/notifications). Let me know what you think about
that ok?

[commit-bot: I haz the power, 2016-11-24 17:32:45]

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

[commit-bot: I haz the power, 2016-11-24 17:32:45]

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_...)

[Miguel Garcia, 2016-11-30 13:26:16]

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

[commit-bot: I haz the power, 2016-11-30 13:26:27]

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

[Miguel Garcia, 2016-11-30 13:26:57]

I have reverted the unit test for now (since sky didn't like my idea for the
message service).

On 2016/11/24 16:55:18, Miguel Garcia wrote:
> A few notes:
> 
>  The unit tests needs https://codereview.chromium.org/2528013002 We also have
a
> ui test so we might want to consider removing it so it's not blocking.
> 
>   I am not entirely sure about the new content/public file (whose methods are
> implemented in content/browser/notifications). Let me know what you think
about
> that ok?

[commit-bot: I haz the power, 2016-11-30 14:36:17]

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

[commit-bot: I haz the power, 2016-11-30 14:36:18]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2016-12-02 13:43:34]

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_test_util.h (right):

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_test_util.h:55: // from native
notification centers.
s/This is...notification/centers/ -->

This may happen when native notification centers don't inform us about closed
notifications, for example as a result of a system reboot.

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_test_util.h:56: bool
ClearNotificationById(const std::string& delegate_id,
nit: what about "SilentDismissById" to further distinguish from CancelById? I'd
love the name to reflect how it's different from regular cancellation.

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

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:387: return
false;
I think we should dismiss the previous TODO and just continue to return
everything. That way we don't have to expose NotificationIdVerifier in the
//content API.

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

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:602:
std::vector<std::string> notifications = base::SplitString(
Consider using (from <algorithm>):

  ASSERT_EQ(1, std::count(script_message.begin(), script_message.end(), ','));

You don't use the values in |notifications| at any point, so we can remove the
allocations and additional work by simply counting the separators.

dito on line 619

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:611:
ASSERT_TRUE(notification_id.starts_with("p:"));
  ASSERT_TRUE(
      base::StartsWith(notification.id(), "p:", base::CompareCase::SENSITIVE));

(You might want to annotate in a comment what "p:" means.)

https://codereview.chromium.org/2534443002/diff/60001/chrome/test/data/notifi...
File chrome/test/data/notifications/platform_notification_service.html (right):

https://codereview.chromium.org/2534443002/diff/60001/chrome/test/data/notifi...
chrome/test/data/notifications/platform_notification_service.html:33: }
There's no reason for the Notification constructor to throw here, so we can drop
the try/catch. Also, we'd want to acknowledge showing the notification when it's
actually been shown. What about:

function DisplayNonPersistentNotification(title, options) {
  const notification = new Notification(title, options || {});
  notification.addEventListener('show', e =>
      domAutomationController.send('ok'));
  notification.addEventListener('error', e =>
      domAutomationController.send('could not show notification'));
}

https://codereview.chromium.org/2534443002/diff/60001/chrome/test/data/notifi...
chrome/test/data/notifications/platform_notification_service.html:163: });
Please stick to Chromium's two-space indentation rules for this entire method...

https://codereview.chromium.org/2534443002/diff/60001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/2534443002/diff/60001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:212: service
&&
Why would |service| be NULL?

https://codereview.chromium.org/2534443002/diff/60001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:234: }
We'll want to synchronize the database if there are any inconsistencies. It's
fine to do that as a follow-up, but please add a TODO.

[Miguel Garcia, 2016-12-05 10:52:18]

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

[commit-bot: I haz the power, 2016-12-05 10:52:29]

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

[Miguel Garcia, 2016-12-05 11:01:23]

Patchset #4 (id:80001) has been deleted

[Miguel Garcia, 2016-12-05 11:05:11]

Patchset #5 (id:120001) has been deleted

[Miguel Garcia, 2016-12-05 11:35:18]

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

[commit-bot: I haz the power, 2016-12-05 11:35:30]

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

[commit-bot: I haz the power, 2016-12-05 11:47:56]

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

[commit-bot: I haz the power, 2016-12-05 11:47:57]

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

[Miguel Garcia, 2016-12-05 12:04:27]

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

[commit-bot: I haz the power, 2016-12-05 12:04:39]

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

[commit-bot: I haz the power, 2016-12-05 12:37:47]

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

[commit-bot: I haz the power, 2016-12-05 12:37:48]

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_linu...)

[Miguel Garcia, 2016-12-05 13:11:27]

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

[commit-bot: I haz the power, 2016-12-05 13:11:38]

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

[Miguel Garcia, 2016-12-05 13:14:44]

Patchset #5 (id:140001) has been deleted

[Miguel Garcia, 2016-12-05 13:14:54]

Patchset #5 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-12-05 14:55:45]

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

[commit-bot: I haz the power, 2016-12-05 14:55:46]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Miguel Garcia, 2016-12-06 10:30:34]

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

[commit-bot: I haz the power, 2016-12-06 10:30:52]

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

[Miguel Garcia, 2016-12-06 12:01:19]

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_test_util.h (right):

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_test_util.h:55: // from native
notification centers.
On 2016/12/02 13:43:34, Peter Beverloo wrote:
> s/This is...notification/centers/ -->
> 
> This may happen when native notification centers don't inform us about closed
> notifications, for example as a result of a system reboot.

Done.

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_test_util.h:56: bool
ClearNotificationById(const std::string& delegate_id,
On 2016/12/02 13:43:34, Peter Beverloo wrote:
> nit: what about "SilentDismissById" to further distinguish from CancelById?
I'd
> love the name to reflect how it's different from regular cancellation.

Done.

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

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:387: return
false;
On 2016/12/02 13:43:34, Peter Beverloo wrote:
> I think we should dismiss the previous TODO and just continue to return
> everything. That way we don't have to expose NotificationIdVerifier in the
> //content API.

That's a good plan, I renamed the method in the process to make it clear what it
does.

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

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:602:
std::vector<std::string> notifications = base::SplitString(
I rather count the elements, if I count the separator I need to add extra code
when there are none to see if there are 0 or 1 elements on the list which kinds
of defeats the purpose.

On 2016/12/02 13:43:34, Peter Beverloo wrote:
> Consider using (from <algorithm>):
> 
>   ASSERT_EQ(1, std::count(script_message.begin(), script_message.end(), ','));
> 
> You don't use the values in |notifications| at any point, so we can remove the
> allocations and additional work by simply counting the separators.
> 
> dito on line 619

https://codereview.chromium.org/2534443002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:611:
ASSERT_TRUE(notification_id.starts_with("p:"));
On 2016/12/02 13:43:34, Peter Beverloo wrote:
>   ASSERT_TRUE(
>       base::StartsWith(notification.id(), "p:",
base::CompareCase::SENSITIVE));
> 
> (You might want to annotate in a comment what "p:" means.)

Done.

https://codereview.chromium.org/2534443002/diff/60001/chrome/test/data/notifi...
File chrome/test/data/notifications/platform_notification_service.html (right):

https://codereview.chromium.org/2534443002/diff/60001/chrome/test/data/notifi...
chrome/test/data/notifications/platform_notification_service.html:33: }
It will throw on Android. Then again these tests are not run on Android so I
guess it's fine

On 2016/12/02 13:43:34, Peter Beverloo wrote:
> There's no reason for the Notification constructor to throw here, so we can
drop
> the try/catch. Also, we'd want to acknowledge showing the notification when
it's
> actually been shown. What about:
> 
> function DisplayNonPersistentNotification(title, options) {
>   const notification = new Notification(title, options || {});
>   notification.addEventListener('show', e =>
>       domAutomationController.send('ok'));
>   notification.addEventListener('error', e =>
>       domAutomationController.send('could not show notification'));
> }

https://codereview.chromium.org/2534443002/diff/60001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/2534443002/diff/60001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:212: service
&&
On 2016/12/02 13:43:34, Peter Beverloo wrote:
> Why would |service| be NULL?

It's null in some context_unittests like
PlatformNotificationContextTest.ReadAllServiceWorkerDataEmpty

https://codereview.chromium.org/2534443002/diff/60001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:234: }
On 2016/12/02 13:43:34, Peter Beverloo wrote:
> We'll want to synchronize the database if there are any inconsistencies. It's
> fine to do that as a follow-up, but please add a TODO.

Done.

https://codereview.chromium.org/2534443002/diff/200001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(left):

https://codereview.chromium.org/2534443002/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_notification_manager.cc:89:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
Note that I have removed this restriction since we now actually call this method
from a SW thread.

[commit-bot: I haz the power, 2016-12-06 12:11:57]

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

[commit-bot: I haz the power, 2016-12-06 12:11:57]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2016-12-06 13:04:02]

Thanks! lg % threading

https://codereview.chromium.org/2534443002/diff/200001/chrome/test/data/notif...
File chrome/test/data/notifications/platform_notification_service.html (right):

https://codereview.chromium.org/2534443002/diff/200001/chrome/test/data/notif...
chrome/test/data/notifications/platform_notification_service.html:153:
messageStack.push(notificationResult);
micro nit: -2 space indent for lines 146-153

https://codereview.chromium.org/2534443002/diff/200001/content/browser/notifi...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/2534443002/diff/200001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:214:
&displayed_notifications);
We're not only calling GetDisplayedNotifications() on the UI thread anymore,
we're now calling it from any arbitrary thread - this method will be executed on
a sequenced task runner created through a blocking thread pool.

That violates a whole series of assumptions - ContentBrowserClient that's
assumed to live on the UI thread, access to the message center (through the
NotificationUIManager), access to the native notification centers. That could be
a potential source of bugs - who knows what operating systems do for concurrent
calls to the system APIs?

I'm not sure what to suggest. If at all possible, I'd like to limit the method
to the UI thread. That would imply that we get a jump like this:

PNCI::ReadAllNotificationDataForServiceWorkerRegistration() (IO thread)
--> Moves the LazyInitialize() call on line 190 to a base::Callback passed to...

PNCI::GetDisplayedNotificationsOnUiThread() (UI thread)
--> Gets the notifications (async if needed - something you'll likely need
anyway)
--> Posts the passed base::Callback to the IO thread.

PNCI::LazyInitialize() and beyond (IO thread) -- the existing execution path

You'd need to do something silly like allocating the std::set<> on the heap,
passing ownership to the base::Callback to
PNCI::DoReadAllNotificationDataForServiceWorkerRegistration() and passing it
weakly to the PNCI::GetDisplayedNotificationsOnUiThread() function, so that both
can access the same memory...

Let's chat? :)

[Miguel Garcia, 2016-12-07 16:29:55]

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

[commit-bot: I haz the power, 2016-12-07 16:30:18]

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

[Miguel Garcia, 2016-12-07 16:33:48]

Patchset #7 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-12-07 16:34:48]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Miguel Garcia, 2016-12-07 17:19:28]

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

[commit-bot: I haz the power, 2016-12-07 17:20:05]

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

[Miguel Garcia, 2016-12-07 17:20:50]

Thanks for the review. I now always called GetNotifications from the UI thread
(added a DCHECK too). As agreed it's not clear how own the vector properly.
Right now I free it at the very end, once we have filtered out which
notifications from the database are still valid. Ideas welcome.

https://codereview.chromium.org/2534443002/diff/200001/chrome/test/data/notif...
File chrome/test/data/notifications/platform_notification_service.html (right):

https://codereview.chromium.org/2534443002/diff/200001/chrome/test/data/notif...
chrome/test/data/notifications/platform_notification_service.html:153:
messageStack.push(notificationResult);
On 2016/12/06 13:04:02, Peter Beverloo wrote:
> micro nit: -2 space indent for lines 146-153

Done.

[commit-bot: I haz the power, 2016-12-07 19:49:53]

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

[commit-bot: I haz the power, 2016-12-07 19:49:53]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2016-12-13 11:41:12]

Sorry for the delay! I failed to send comments last week :(

I uploaded a proposal for cleaning up the ownership semantics of this CL here:

    https://codereview.chromium.org/2565323004/

https://codereview.chromium.org/2534443002/diff/240001/chrome/browser/notific...
File
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
(right):

https://codereview.chromium.org/2534443002/diff/240001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:609:
// GetNotifications should report the correct number.
nit: 609 is a stray comment line? what does it relate to?

https://codereview.chromium.org/2534443002/diff/240001/content/browser/notifi...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/2534443002/diff/240001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:263:
displayed_ids->count(it->notification_id)) {
Mm. std::set<> makes this O(n log n). Could be O(n) if we'd use an
std::unordered_set<>.

Doesn't matter a huge amount anyway - in pretty much all cases |n| will be very
small.

https://codereview.chromium.org/2534443002/diff/240001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:266: it =
notification_datas.erase(it);
nit: drop this. Maybe DCHECK on
NotificationIdGenerator::IsPersistentNotification(). The database is *only* used
for persistent notifications.

[Miguel Garcia, 2016-12-13 16:42:06]

Description was changed from

==========
Use notification display service to collect persistent notifications.

- Filter out non persistent notifications on PlatformNotificationService
- Fiter our notifications present in the database but not displayed

In order to keep the model simple there is no syncing of the database with
the new information, we stil rely on chrome restarts for that.

BUG=571056
==========

to

==========
Use notification display service to collect persistent notifications.

- Filter out non persistent notifications on PlatformNotificationService
- Fiter our notifications present in the database but not displayed

In order to keep the model simple there is no syncing of the database with
the new information, we stil rely on chrome restarts for that.

This change also contains https://codereview.chromium.org/2565323004/ authored
by Peter Beverloo.

BUG=571056
==========

[Miguel Garcia, 2016-12-13 16:42:21]

Description was changed from

==========
Use notification display service to collect persistent notifications.

- Filter out non persistent notifications on PlatformNotificationService
- Fiter our notifications present in the database but not displayed

In order to keep the model simple there is no syncing of the database with
the new information, we stil rely on chrome restarts for that.

This change also contains https://codereview.chromium.org/2565323004/ authored
by Peter Beverloo.

BUG=571056
==========

to

==========
Use notification display service to collect persistent notifications.

- Filter out non persistent notifications on PlatformNotificationService
- Fiter our notifications present in the database but not displayed

In order to keep the model simple there is no syncing of the database with
the new information, we stil rely on chrome restarts for that.

This CL also integrates https://codereview.chromium.org/2565323004/ authored by
Peter Beverloo.

BUG=571056
==========

[Miguel Garcia, 2016-12-13 16:42:27]

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

[Miguel Garcia, 2016-12-13 16:43:10]

Cool! I merged your patch as well (after reading carefully what base::Passed
does) and resolved the other nits.

https://codereview.chromium.org/2534443002/diff/240001/chrome/browser/notific...
File
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
(right):

https://codereview.chromium.org/2534443002/diff/240001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_interactive_uitest.cc:609:
// GetNotifications should report the correct number.
On 2016/12/13 11:41:12, Peter Beverloo wrote:
> nit: 609 is a stray comment line? what does it relate to?

It meant that GetNotifications should still report the right number even though
we haven't updated the database. It does not seem to add a lot here so I just
removed it.

https://codereview.chromium.org/2534443002/diff/240001/content/browser/notifi...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/2534443002/diff/240001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:263:
displayed_ids->count(it->notification_id)) {
On 2016/12/13 11:41:12, Peter Beverloo wrote:
> Mm. std::set<> makes this O(n log n). Could be O(n) if we'd use an
> std::unordered_set<>.
> 
> Doesn't matter a huge amount anyway - in pretty much all cases |n| will be
very
> small.

Yeah, and it makes the test more predictable (since they are returned in the
order they were inserted). Would prefer to leave it as is, otherwise I need to
read the notifications in the test, find one that is persistent to remove it
etc.

https://codereview.chromium.org/2534443002/diff/240001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:266: it =
notification_datas.erase(it);
On 2016/12/13 11:41:12, Peter Beverloo wrote:
> nit: drop this. Maybe DCHECK on
> NotificationIdGenerator::IsPersistentNotification(). The database is *only*
used
> for persistent notifications.

Done.

[commit-bot: I haz the power, 2016-12-13 16:43:19]

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

[Miguel Garcia, 2016-12-13 16:59:18]

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

[commit-bot: I haz the power, 2016-12-13 16:59:59]

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

[Peter Beverloo, 2016-12-13 17:00:46]

lgtm

[Miguel Garcia, 2016-12-13 18:09:34]

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

[Miguel Garcia, 2016-12-13 18:09:35]

+avi for  content/public/browser/platform_notification_service.h OWNERS

[Avi (use Gerrit), 2016-12-13 18:10:54]

content public lgtm

[Miguel Garcia, 2016-12-13 18:13:01]

The CQ bit was unchecked by miguelg@chromium.org

[Miguel Garcia, 2016-12-13 18:13:01]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2016-12-13 18:13:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 19:26:55]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1481652781969760,
"parent_rev": "51f8e42eb347e82f0687ca42fb95b41c7dfc102a", "commit_rev":
"c318a0a619dff025698ccd97c957090c9f968c2b"}

[commit-bot: I haz the power, 2016-12-13 19:27:22]

Description was changed from

==========
Use notification display service to collect persistent notifications.

- Filter out non persistent notifications on PlatformNotificationService
- Fiter our notifications present in the database but not displayed

In order to keep the model simple there is no syncing of the database with
the new information, we stil rely on chrome restarts for that.

This CL also integrates https://codereview.chromium.org/2565323004/ authored by
Peter Beverloo.

BUG=571056
==========

to

==========
Use notification display service to collect persistent notifications.

- Filter out non persistent notifications on PlatformNotificationService
- Fiter our notifications present in the database but not displayed

In order to keep the model simple there is no syncing of the database with
the new information, we stil rely on chrome restarts for that.

This CL also integrates https://codereview.chromium.org/2565323004/ authored by
Peter Beverloo.

BUG=571056

Review-Url: https://codereview.chromium.org/2534443002
==========

[commit-bot: I haz the power, 2016-12-13 19:27:24]

Committed patchset #9 (id:280001)

[commit-bot: I haz the power, 2016-12-13 19:29:26]

Description was changed from

==========
Use notification display service to collect persistent notifications.

- Filter out non persistent notifications on PlatformNotificationService
- Fiter our notifications present in the database but not displayed

In order to keep the model simple there is no syncing of the database with
the new information, we stil rely on chrome restarts for that.

This CL also integrates https://codereview.chromium.org/2565323004/ authored by
Peter Beverloo.

BUG=571056

Review-Url: https://codereview.chromium.org/2534443002
==========

to

==========
Use notification display service to collect persistent notifications.

- Filter out non persistent notifications on PlatformNotificationService
- Fiter our notifications present in the database but not displayed

In order to keep the model simple there is no syncing of the database with
the new information, we stil rely on chrome restarts for that.

This CL also integrates https://codereview.chromium.org/2565323004/ authored by
Peter Beverloo.

BUG=571056

Committed: https://crrev.com/fea8f2f0eba8a4b9e98281dd52f64a1241a96d9c
Cr-Commit-Position: refs/heads/master@{#438251}
==========

[commit-bot: I haz the power, 2016-12-13 19:29:28]

Patchset 9 (id:??) landed as
https://crrev.com/fea8f2f0eba8a4b9e98281dd52f64a1241a96d9c
Cr-Commit-Position: refs/heads/master@{#438251}