Make GetDisplayedNotifications asynchronous.

Make GetDisplayedNotifications asynchronous.

BUG=571056
Review-Url: https://codereview.chromium.org/2749453002
Cr-Commit-Position: refs/heads/master@{#458071}
Committed: https://chromium.googlesource.com/chromium/src/+/bfe768358163d78edf9b3a48bef284427fb4d380

[Miguel Garcia, 2017-03-11 10:12:03]

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

[commit-bot: I haz the power, 2017-03-11 10:12:16]

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, 2017-03-11 10:17:49]

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

[commit-bot: I haz the power, 2017-03-11 10:17:50]

Dry run: Try jobs failed on following builders:
  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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Miguel Garcia, 2017-03-13 09:23:01]

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

[commit-bot: I haz the power, 2017-03-13 09:23:13]

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, 2017-03-13 09:30:22]

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

[commit-bot: I haz the power, 2017-03-13 09:30:23]

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_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Miguel Garcia, 2017-03-13 16:55:15]

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

[commit-bot: I haz the power, 2017-03-13 16:55:52]

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, 2017-03-13 17:11:53]

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

[commit-bot: I haz the power, 2017-03-13 17:11:53]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Miguel Garcia, 2017-03-13 17:38:50]

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

[commit-bot: I haz the power, 2017-03-13 17:39:22]

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, 2017-03-13 18:30:26]

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

[commit-bot: I haz the power, 2017-03-13 18:30:27]

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

[Miguel Garcia, 2017-03-15 13:04:25]

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

[commit-bot: I haz the power, 2017-03-15 13:04: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, 2017-03-15 14:01:17]

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

[Miguel Garcia, 2017-03-15 14:02:26]

I went with your suggestion to "reverse" 
https://codereview.chromium.org/2709213005/ so this essentially makes the
GetDisplayed call async with no actual implementation changes on the mac port

[Peter Beverloo, 2017-03-15 18:07:51]

Largely trivial nits, sorry

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/message_center_display_service.cc (right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/message_center_display_service.cc:39:
base::MakeUnique<std::set<std::string>>();
note: I think `auto` is a reasonable thing for this sort of declaration

  auto notifications = base::MakeUnique<std::set<std::string>>();

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge.h (right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge.h:23: using
NotificationResultCallback =
nit: here and elsewhere: what is the "result" of a notification?

DisplayedNotificationsCallback maybe?

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge.h:25: bool /* supports
synchronization */)>;
nit: blank line under this

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge_android.cc
(right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_android.cc:319:
std::unique_ptr<std::set<std::string>> displayedNotifications =
nit: displayed_notifications

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge_android.h
(right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_android.h:72: 
nit: drop the blank line (comment on line 64 applies)

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm
(right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:29: :
thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {}
Huh? Where does this test require the IO thread?

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:281:
base::RunLoop().RunUntilIdle();
nit: another paradigm you can consider is:

void ReturnNotificationCount(
    size_t* out_notification_count,
    base::Closure quit_closure,
    std::unique_ptr<std::set<std::string>> notifications,
    bool supports_sync) {
  DCHECK(out_notification_count);
  *out_notification_count = notification.size();
  quit_closure.Run();
}

---

{
  size_t notification_count;

  base::RunLoop run_loop;
  bridge->GetDisplayed("profile_id", false /* incognito */,
      base::Bind(&ReturnNotificationCount, &notification_count,
                 run_loop.QuitClosure()));
  run_loop.Run();

  EXPECT_EQ(notification_count, 1u);
}

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:390:
std::unique_ptr<std::set<std::string>> displayedNotifications =
displayed_notifications or delete per the comment on the
PlatformNotificationService interface

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:395: false /*
supports getting displayed notifications */));
I'd just take the easy path here and use:

  callback.Run(std::move(displayed_notifications), false /* supported */);

You are on the UI thread, and there's only a single user of the //content API.

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_unittest.cc
(right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_unittest.cc:157:
#endif
This'll break on Mac pretty soon.

What about:

    display_service()->GetDisplayed(base::Bind(
        &PlatformNotificationServiceTest::DidGetDisplayedNotifications,
        base::Unretained(this));
    base::RunLoop().RunUntilIdle();

    ASSERT_TRUE(displayed_notifications_);
    return displayed_notifications_.size();

With the following new method:

  void DidGetDisplayedNotifications(
      std::unique_ptr<std::set<std::string>> displayed_notifications,
      bool supports_synchronization) {
    displayed_notifications_ = std::move(displayed_notifications);
  }

And the following attribute:

  std::unique_ptr<std::set<std::string>> displayed_notifications_;

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/stub_notification_platform_bridge.cc (right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/stub_notification_platform_bridge.cc:26: for (const
auto& profile : notifications_) {
nit: s/profile/pair/

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/stub_notification_platform_bridge.h (right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/stub_notification_platform_bridge.h:26: size_t
GetNotificationCount();
This kind of feels like cheating :P

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

https://codereview.chromium.org/2749453002/diff/100001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:60: if
(!service) {
Here's an idea: should we maybe just bail out if there's no service? It's not
like there'll be data in the database in that scenario, or that registering a
Service Worker observer is going to be helpful...

  // There is no need to initialize the platform notification context if the
  // embedder-provided service is not available.
  if (!GetContentClient()->browser()->GetPlatformNotificationService())
    return;

That saves you quite some code here, as well as having the {Initialize,
InitializeOnUI, InitializeOnIO} triple.

https://codereview.chromium.org/2749453002/diff/100001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:250:
std::move(notification_ids), false /* sync_supported */);
Here too, maybe we should just drop out?

    callback.Run(false /* success */, std::vector<NotificationDatabaseData>());

https://codereview.chromium.org/2749453002/diff/100001/content/public/browser...
File content/public/browser/platform_notification_service.h (right):

https://codereview.chromium.org/2749453002/diff/100001/content/public/browser...
content/public/browser/platform_notification_service.h:38: bool /* supports
synchronization */)>;
Something to consider: now that the first argument is an std::unique_ptr<>, we
can convey that it's not supported by passing a nullptr. (Since NULL and empty
are now semantically different.) That saves you a fair number of allocations in
places where it's not supported.

https://codereview.chromium.org/2749453002/diff/100001/content/public/browser...
content/public/browser/platform_notification_service.h:85: // Retrieves Writes
the ids of all currently displaying notifications and
nit: s/Writes//

https://codereview.chromium.org/2749453002/diff/100001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/2749453002/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_notification_manager.cc:89:
std::unique_ptr<std::set<std::string>> displayedNotifications =
nit: displayed_notifications

https://codereview.chromium.org/2749453002/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_notification_manager.cc:92:
content::BrowserThread::UI, FROM_HERE,
nit: drop content:: from these, you are in that namespace

https://codereview.chromium.org/2749453002/diff/100001/content/test/mock_plat...
File content/test/mock_platform_notification_service.cc (right):

https://codereview.chromium.org/2749453002/diff/100001/content/test/mock_plat...
content/test/mock_platform_notification_service.cc:86: bool /* supports
synchronization */)>& callback) {
dito

https://codereview.chromium.org/2749453002/diff/100001/content/test/mock_plat...
content/test/mock_platform_notification_service.cc:95:
content::BrowserThread::UI, FROM_HERE,
nit: same, drop content::

https://codereview.chromium.org/2749453002/diff/100001/content/test/mock_plat...
File content/test/mock_platform_notification_service.h (right):

https://codereview.chromium.org/2749453002/diff/100001/content/test/mock_plat...
content/test/mock_platform_notification_service.h:77: bool /* success */)>&
callback) override;
Why doesn't this use the typedef?

[Miguel Garcia, 2017-03-16 14:57:24]

Patchset #7 (id:120001) has been deleted

[Miguel Garcia, 2017-03-16 14:57:34]

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

[Miguel Garcia, 2017-03-16 14:57:43]

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/message_center_display_service.cc (right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/message_center_display_service.cc:39:
base::MakeUnique<std::set<std::string>>();
On 2017/03/15 18:07:50, Peter Beverloo wrote:
> note: I think `auto` is a reasonable thing for this sort of declaration
> 
>   auto notifications = base::MakeUnique<std::set<std::string>>();

Done.

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge.h (right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge.h:23: using
NotificationResultCallback =
On 2017/03/15 18:07:50, Peter Beverloo wrote:
> nit: here and elsewhere: what is the "result" of a notification?
> 
> DisplayedNotificationsCallback maybe?

Done.

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge.h:25: bool /* supports
synchronization */)>;
On 2017/03/15 18:07:50, Peter Beverloo wrote:
> nit: blank line under this

Done.

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge_android.cc
(right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_android.cc:319:
std::unique_ptr<std::set<std::string>> displayedNotifications =
On 2017/03/15 18:07:50, Peter Beverloo wrote:
> nit: displayed_notifications

Done.

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge_android.h
(right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_android.h:72: 
On 2017/03/15 18:07:50, Peter Beverloo wrote:
> nit: drop the blank line (comment on line 64 applies)

Done.

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm
(right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:29: :
thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {}
On 2017/03/15 18:07:50, Peter Beverloo wrote:
> Huh? Where does this test require the IO thread?

duh, that was needed when I was replying in the IO thread, changed to DEFAULT
now.

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:281:
base::RunLoop().RunUntilIdle();
I rather leave it as is unless you disagree strongly :)


On 2017/03/15 18:07:50, Peter Beverloo wrote:
> nit: another paradigm you can consider is:
> 
> void ReturnNotificationCount(
>     size_t* out_notification_count,
>     base::Closure quit_closure,
>     std::unique_ptr<std::set<std::string>> notifications,
>     bool supports_sync) {
>   DCHECK(out_notification_count);
>   *out_notification_count = notification.size();
>   quit_closure.Run();
> }
> 
> ---
> 
> {
>   size_t notification_count;
> 
>   base::RunLoop run_loop;
>   bridge->GetDisplayed("profile_id", false /* incognito */,
>       base::Bind(&ReturnNotificationCount, &notification_count,
>                  run_loop.QuitClosure()));
>   run_loop.Run();
> 
>   EXPECT_EQ(notification_count, 1u);
> }

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:390:
std::unique_ptr<std::set<std::string>> displayedNotifications =
On 2017/03/15 18:07:50, Peter Beverloo wrote:
> displayed_notifications or delete per the comment on the
> PlatformNotificationService interface

Done.

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:395: false /*
supports getting displayed notifications */));
On 2017/03/15 18:07:50, Peter Beverloo wrote:
> I'd just take the easy path here and use:
> 
>   callback.Run(std::move(displayed_notifications), false /* supported */);
> 
> You are on the UI thread, and there's only a single user of the //content API.

Done.

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_unittest.cc
(right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_unittest.cc:157:
#endif
It wont' break (since the stub does not have XPC).


On 2017/03/15 18:07:50, Peter Beverloo wrote:
> This'll break on Mac pretty soon.
> 
> What about:
> 
>     display_service()->GetDisplayed(base::Bind(
>         &PlatformNotificationServiceTest::DidGetDisplayedNotifications,
>         base::Unretained(this));
>     base::RunLoop().RunUntilIdle();
> 
>     ASSERT_TRUE(displayed_notifications_);
>     return displayed_notifications_.size();
> 
> With the following new method:
> 
>   void DidGetDisplayedNotifications(
>       std::unique_ptr<std::set<std::string>> displayed_notifications,
>       bool supports_synchronization) {
>     displayed_notifications_ = std::move(displayed_notifications);
>   }
> 
> And the following attribute:
> 
>   std::unique_ptr<std::set<std::string>> displayed_notifications_;

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/stub_notification_platform_bridge.cc (right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/stub_notification_platform_bridge.cc:26: for (const
auto& profile : notifications_) {
On 2017/03/15 18:07:50, Peter Beverloo wrote:
> nit: s/profile/pair/

Done.

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/stub_notification_platform_bridge.h (right):

https://codereview.chromium.org/2749453002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/stub_notification_platform_bridge.h:26: size_t
GetNotificationCount();
On 2017/03/15 18:07:51, Peter Beverloo wrote:
> This kind of feels like cheating :P

well it's a test :)

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

https://codereview.chromium.org/2749453002/diff/100001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:60: if
(!service) {
On 2017/03/15 18:07:51, Peter Beverloo wrote:
> Here's an idea: should we maybe just bail out if there's no service? It's not
> like there'll be data in the database in that scenario, or that registering a
> Service Worker observer is going to be helpful...
> 
>   // There is no need to initialize the platform notification context if the
>   // embedder-provided service is not available.
>   if (!GetContentClient()->browser()->GetPlatformNotificationService())
>     return;
> 
> That saves you quite some code here, as well as having the {Initialize,
> InitializeOnUI, InitializeOnIO} triple.

Done.

https://codereview.chromium.org/2749453002/diff/100001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:250:
std::move(notification_ids), false /* sync_supported */);
On 2017/03/15 18:07:51, Peter Beverloo wrote:
> Here too, maybe we should just drop out?
> 
>     callback.Run(false /* success */,
std::vector<NotificationDatabaseData>());

I think we have some tests that rely on this right?

https://codereview.chromium.org/2749453002/diff/100001/content/public/browser...
File content/public/browser/platform_notification_service.h (right):

https://codereview.chromium.org/2749453002/diff/100001/content/public/browser...
content/public/browser/platform_notification_service.h:38: bool /* supports
synchronization */)>;
On 2017/03/15 18:07:51, Peter Beverloo wrote:
> Something to consider: now that the first argument is an std::unique_ptr<>, we
> can convey that it's not supported by passing a nullptr. (Since NULL and empty
> are now semantically different.) That saves you a fair number of allocations
in
> places where it's not supported.

I played with it a little because I thought it was a good idea, the problem I
ran myself into is that the callback and the underlying vector is passed around
many times and in everyone of them you'd have to check for null which is a pain.

https://codereview.chromium.org/2749453002/diff/100001/content/public/browser...
content/public/browser/platform_notification_service.h:85: // Retrieves Writes
the ids of all currently displaying notifications and
On 2017/03/15 18:07:51, Peter Beverloo wrote:
> nit: s/Writes//

Done.

https://codereview.chromium.org/2749453002/diff/100001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/2749453002/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_notification_manager.cc:89:
std::unique_ptr<std::set<std::string>> displayedNotifications =
On 2017/03/15 18:07:51, Peter Beverloo wrote:
> nit: displayed_notifications

Done.

https://codereview.chromium.org/2749453002/diff/100001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_notification_manager.cc:92:
content::BrowserThread::UI, FROM_HERE,
On 2017/03/15 18:07:51, Peter Beverloo wrote:
> nit: drop content:: from these, you are in that namespace

Done.

https://codereview.chromium.org/2749453002/diff/100001/content/test/mock_plat...
File content/test/mock_platform_notification_service.cc (right):

https://codereview.chromium.org/2749453002/diff/100001/content/test/mock_plat...
content/test/mock_platform_notification_service.cc:86: bool /* supports
synchronization */)>& callback) {
On 2017/03/15 18:07:51, Peter Beverloo wrote:
> dito

Done.

https://codereview.chromium.org/2749453002/diff/100001/content/test/mock_plat...
content/test/mock_platform_notification_service.cc:95:
content::BrowserThread::UI, FROM_HERE,
On 2017/03/15 18:07:51, Peter Beverloo wrote:
> nit: same, drop content::

Done.

https://codereview.chromium.org/2749453002/diff/100001/content/test/mock_plat...
File content/test/mock_platform_notification_service.h (right):

https://codereview.chromium.org/2749453002/diff/100001/content/test/mock_plat...
content/test/mock_platform_notification_service.h:77: bool /* success */)>&
callback) override;
On 2017/03/15 18:07:51, Peter Beverloo wrote:
> Why doesn't this use the typedef?

Done.

[commit-bot: I haz the power, 2017-03-16 14:58:00]

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, 2017-03-16 15:24:32]

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

[commit-bot: I haz the power, 2017-03-16 15:24:33]

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, 2017-03-16 15:45:26]

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

[commit-bot: I haz the power, 2017-03-16 15:46:06]

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

[Miguel Garcia, 2017-03-16 15:57:25]

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

https://codereview.chromium.org/2749453002/diff/100001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.cc:60: if
(!service) {
On 2017/03/16 14:57:42, Miguel Garcia wrote:
> On 2017/03/15 18:07:51, Peter Beverloo wrote:
> > Here's an idea: should we maybe just bail out if there's no service? It's
not
> > like there'll be data in the database in that scenario, or that registering
a
> > Service Worker observer is going to be helpful...
> > 
> >   // There is no need to initialize the platform notification context if the
> >   // embedder-provided service is not available.
> >   if (!GetContentClient()->browser()->GetPlatformNotificationService())
> >     return;
> > 
> > That saves you quite some code here, as well as having the {Initialize,
> > InitializeOnUI, InitializeOnIO} triple.
> 
> Done.

I had to revert this because several tests fail as they expect to eventually be
called with the available notifications.

[commit-bot: I haz the power, 2017-03-16 17:05:35]

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

[commit-bot: I haz the power, 2017-03-16 17:05:36]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-03-17 15:46:04]

lgtm % comments

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/message_center_display_service.cc (right):

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/message_center_display_service.cc:42: }
Sorry that I missed this before -- GetAllIdsByProfile returns an std::set() with
the notification IDs. Why not do:

auto displayed_notifications =
    base::MakeUnique<std::set>(
        std::move(ui_manager_->GetAllIdsByProfile(
            NotificationUIManager::GetProfileID(profile_))));

That eliminates all copies :).

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_display_service.h (right):

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service.h:44: // |callback|
with the result.
nit: generally we clarify *where* we post a callback. In this case it doesn't
matter since the class lives on a single thread, so you could rephrase as "and
invokes |callback| with the result once known."

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm
(right):

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:281:
base::RunLoop().RunUntilIdle();
You need to confirm that CheckNotificationCount actually ran. Here, too, a
set-up like this is easier:

  void StoreNotificationCount(
      int* out_notification_count,
      std::unique_ptr<std::set<std::string>> notifications,
      bool supports_synchronization) {
    DCHECK(out_notification_count);
    DCHECK(notifications);
    *out_notification_count = notifications->size();
  }

  // ----

  int notification_count = -1;
  bridge->GetDisplayed("profile_id", false /* incognito */,
                       base::Bind(&StoreNotificationCount,
&notification_count));
  base::RunLoop().RunUntilIdle();
  EXPECT_EQ(notification_count, 1);

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_unittest.cc
(right):

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_unittest.cc:157:
#endif
Even if it won't break on Mac, my proposed code here would avoid the #ifdef.
What's your consideration for not using it?

https://codereview.chromium.org/2749453002/diff/160001/content/browser/notifi...
File content/browser/notifications/platform_notification_context_impl.h (right):

https://codereview.chromium.org/2749453002/diff/160001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.h:148: // if
|sync_supported|.
nit: fix weird line break (also on line 137)

https://codereview.chromium.org/2749453002/diff/160001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/2749453002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_notification_manager.cc:93: false
/* supports getting displayed notifications */));
nit: here and elsewhere - it would be much easier to keep things consistent if
you'd use an argument name instead of a textual description. (You use different
descriptions throughout the code.)

i.e.
  platform_notification_service.h
      "supports synchronization" --> "supports_synchronization"

  layout_test_notification_manager.cc
      "supports getting displayed notifications"
          --> "supports_synchronization"

Your argument names are also very inconsistent:

  supported
  sync_supported
  supports_synchronization
  notification_synchronization_supported

Would be cool if we can align all of that.

[Miguel Garcia, 2017-03-17 15:57:15]

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

[Miguel Garcia, 2017-03-17 15:57:16]

+avi for the content/public and content/test changes

[Avi (use Gerrit), 2017-03-17 16:34:06]

lgtm

[Miguel Garcia, 2017-03-20 14:11:14]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2017-03-20 14:11:14]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2749453002/#ps180001
(title: "review")

[commit-bot: I haz the power, 2017-03-20 14:11:21]

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

[Miguel Garcia, 2017-03-20 14:11:21]

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/message_center_display_service.cc (right):

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/message_center_display_service.cc:42: }
On 2017/03/17 15:46:04, Peter Beverloo wrote:
> Sorry that I missed this before -- GetAllIdsByProfile returns an std::set()
with
> the notification IDs. Why not do:
> 
> auto displayed_notifications =
>     base::MakeUnique<std::set>(
>         std::move(ui_manager_->GetAllIdsByProfile(
>             NotificationUIManager::GetProfileID(profile_))));
> 
> That eliminates all copies :).

Done.

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_display_service.h (right):

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service.h:44: // |callback|
with the result.
On 2017/03/17 15:46:04, Peter Beverloo wrote:
> nit: generally we clarify *where* we post a callback. In this case it doesn't
> matter since the class lives on a single thread, so you could rephrase as "and
> invokes |callback| with the result once known."

Done.

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm
(right):

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:281:
base::RunLoop().RunUntilIdle();
On 2017/03/17 15:46:04, Peter Beverloo wrote:
> You need to confirm that CheckNotificationCount actually ran. Here, too, a
> set-up like this is easier:
> 
>   void StoreNotificationCount(
>       int* out_notification_count,
>       std::unique_ptr<std::set<std::string>> notifications,
>       bool supports_synchronization) {
>     DCHECK(out_notification_count);
>     DCHECK(notifications);
>     *out_notification_count = notifications->size();
>   }
> 
>   // ----
> 
>   int notification_count = -1;
>   bridge->GetDisplayed("profile_id", false /* incognito */,
>                        base::Bind(&StoreNotificationCount,
> &notification_count));
>   base::RunLoop().RunUntilIdle();
>   EXPECT_EQ(notification_count, 1);

Done.

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_unittest.cc
(right):

https://codereview.chromium.org/2749453002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_unittest.cc:157:
#endif
On 2017/03/17 15:46:04, Peter Beverloo wrote:
> Even if it won't break on Mac, my proposed code here would avoid the #ifdef.
> What's your consideration for not using it?

Discussed offline and agreed to change it to this and temporarily making
get_displayed_notifications_ mutable with the idea of removing the cosntness
later on.

https://codereview.chromium.org/2749453002/diff/160001/content/browser/notifi...
File content/browser/notifications/platform_notification_context_impl.h (right):

https://codereview.chromium.org/2749453002/diff/160001/content/browser/notifi...
content/browser/notifications/platform_notification_context_impl.h:148: // if
|sync_supported|.
On 2017/03/17 15:46:04, Peter Beverloo wrote:
> nit: fix weird line break (also on line 137)

Done.

https://codereview.chromium.org/2749453002/diff/160001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/2749453002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_notification_manager.cc:93: false
/* supports getting displayed notifications */));
Good idea, I changed it everywhere now.

On 2017/03/17 15:46:04, Peter Beverloo wrote:
> nit: here and elsewhere - it would be much easier to keep things consistent if
> you'd use an argument name instead of a textual description. (You use
different
> descriptions throughout the code.)
> 
> i.e.
>   platform_notification_service.h
>       "supports synchronization" --> "supports_synchronization"
> 
>   layout_test_notification_manager.cc
>       "supports getting displayed notifications"
>           --> "supports_synchronization"
> 
> Your argument names are also very inconsistent:
> 
>   supported
>   sync_supported
>   supports_synchronization
>   notification_synchronization_supported
> 
> Would be cool if we can align all of that.

[commit-bot: I haz the power, 2017-03-20 15:34:37]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490019074105490,
"parent_rev": "38eaf47941e93a84f9fa8ba53d32a236b215a292", "commit_rev":
"bfe768358163d78edf9b3a48bef284427fb4d380"}

[commit-bot: I haz the power, 2017-03-20 15:35:23]

Description was changed from

==========
Make GetDisplayedNotifications asynchronous.

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

to

==========
Make GetDisplayedNotifications asynchronous.

BUG=571056

Review-Url: https://codereview.chromium.org/2749453002
Cr-Commit-Position: refs/heads/master@{#458071}
Committed:
https://chromium.googlesource.com/chromium/src/+/bfe768358163d78edf9b3a48bef2...
==========

[commit-bot: I haz the power, 2017-03-20 15:35:24]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/bfe768358163d78edf9b3a48bef2...