Deprecate native and extension notification delegates.

Deprecate native and extension notification delegates.

WebNotificationDelegate can now handle extensions and all web
notifications as a result.

TBR=avi@chromium.org
BUG=720345
Review-Url: https://codereview.chromium.org/2928303002
Cr-Commit-Position: refs/heads/master@{#479347}
Committed: https://chromium.googlesource.com/chromium/src/+/d95830c60e22fa695f0521004149caf03bcc2f77

[Miguel Garcia, 2017-06-11 13:17:09]

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

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

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-06-11 13:17:28]

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

[commit-bot: I haz the power, 2017-06-11 14:50:09]

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

[commit-bot: I haz the power, 2017-06-11 14:50:10]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-06-13 22:56:42]

Largely nits, thanks :) lgtm

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/notifications/extension_notification_handler.cc
(right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/extension_notification_handler.cc:136:
bool ExtensionNotificationHandler::ShouldDisplayOnFullScreen(
nit: put this method on line 113 to keep header and implementation method order
in sync

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/extension_notification_handler.cc:142:
GetExtensionId(origin));
Should we DCHECK that GetExtensionId is not the empty string?

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_common.cc (right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_common.cc:25: }  // namespace features
nit: blank line between 24 and 25

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_common.cc:34: #endif  //
defined(OS_ANDROID)
Why did you change this? I prefer the #if/#else/#endif paradigm over an early
return with the compiler removing the unused code.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_common.h (right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_common.h:12: // TODO(miguelg) We can
probably get rid of this altogether.
Yes. The extension one too.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_common.h:22: class GURL;
nit: alphabetize

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_common.h:51: static bool
ActiveTabIsFullScreen(Profile* profile, const GURL& origin);
++docs. Maybe just call this ShouldDisplayOnFullScreen to match the handlers
too?

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_display_service.cc (right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service.cc:80: return
handler->ShouldDisplayOnFullScreen(profile_, origin.spec());
DCHECK(handler)

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_handler.h (right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_handler.h:48: const std::string&
origin) const = 0;
Out of interest, why wouldn't we pass |origin| as a GURL or url::Origin?

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_handler.h:48: const std::string&
origin) const = 0;
nit: drop `const`, I think we've learned our lesson about const correctness and
interfaces. Anita ran into another one today.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/web_notification_delegate.h (right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/web_notification_delegate.h:25: class
WebNotificationDelegate : public NotificationDelegate {
Eventually the Message Center display bridge will create this, right? The name
WebNotificationDelegate isn't really appropriate for extensions. Do you have
thoughts on what we might call it eventually?

https://codereview.chromium.org/2928303002/diff/1/content/test/mock_platform_...
File content/test/mock_platform_notification_service.cc (right):

https://codereview.chromium.org/2928303002/diff/1/content/test/mock_platform_...
content/test/mock_platform_notification_service.cc:169:
content::NotificationEventDispatcher::GetInstance()
nit: drop `content`, we are in that namespace

https://codereview.chromium.org/2928303002/diff/1/content/test/mock_platform_...
content/test/mock_platform_notification_service.cc:171:
non_persistent_notifications_.erase(non_persistent_iter);
Just call n_p_n_.erase() with |notification_id| if we don't use the iterator for
other purposes.

[Miguel Garcia, 2017-06-14 09:05:38]

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

[Miguel Garcia, 2017-06-14 09:05:39]

+avi for content

[Miguel Garcia, 2017-06-14 09:39:15]

Description was changed from

==========
Deprecate native and extension notification delegates.

WebNotificationDelegate can now handle extensions and all web
notifications as a result.

BUG=720345
==========

to

==========
Deprecate native and extension notification delegates.

WebNotificationDelegate can now handle extensions and all web
notifications as a result.

TBR=avi@chromium.org
BUG=720345
==========

[Miguel Garcia, 2017-06-14 09:39:20]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2017-06-14 09:39:21]

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

[commit-bot: I haz the power, 2017-06-14 09:39:30]

CQ is trying da patch.

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

[Miguel Garcia, 2017-06-14 09:39:39]

TBR avi for content/test/mock_platform_notification_service.cc

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/notifications/extension_notification_handler.cc
(right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/extension_notification_handler.cc:136:
bool ExtensionNotificationHandler::ShouldDisplayOnFullScreen(
On 2017/06/13 22:56:40, Peter Beverloo wrote:
> nit: put this method on line 113 to keep header and implementation method
order
> in sync

Done.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/extension_notification_handler.cc:142:
GetExtensionId(origin));
On 2017/06/13 22:56:40, Peter Beverloo wrote:
> Should we DCHECK that GetExtensionId is not the empty string?

Done.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_common.cc (right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_common.cc:25: }  // namespace features
On 2017/06/13 22:56:40, Peter Beverloo wrote:
> nit: blank line between 24 and 25

Done.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_common.cc:34: #endif  //
defined(OS_ANDROID)
On 2017/06/13 22:56:40, Peter Beverloo wrote:
> Why did you change this? I prefer the #if/#else/#endif paradigm over an early
> return with the compiler removing the unused code.

Felt easier to read, will change it back.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_common.h (right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_common.h:12: // TODO(miguelg) We can
probably get rid of this altogether.
On 2017/06/13 22:56:41, Peter Beverloo wrote:
> Yes. The extension one too.

Acknowledged.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_common.h:22: class GURL;
On 2017/06/13 22:56:40, Peter Beverloo wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_common.h:51: static bool
ActiveTabIsFullScreen(Profile* profile, const GURL& origin);
On 2017/06/13 22:56:41, Peter Beverloo wrote:
> ++docs. Maybe just call this ShouldDisplayOnFullScreen to match the handlers
> too?

Done.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_display_service.cc (right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_display_service.cc:80: return
handler->ShouldDisplayOnFullScreen(profile_, origin.spec());
On 2017/06/13 22:56:41, Peter Beverloo wrote:
> DCHECK(handler)

Done.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_handler.h (right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_handler.h:48: const std::string&
origin) const = 0;
On 2017/06/13 22:56:41, Peter Beverloo wrote:
> nit: drop `const`, I think we've learned our lesson about const correctness
and
> interfaces. Anita ran into another one today.

Done.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_handler.h:48: const std::string&
origin) const = 0;
On 2017/06/13 22:56:42, Peter Beverloo wrote:
> Out of interest, why wouldn't we pass |origin| as a GURL or url::Origin?

I wanted to minimize the number of times string is converted to GURL and
viceversa just to go through all the layering.

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/web_notification_delegate.h (right):

https://codereview.chromium.org/2928303002/diff/1/chrome/browser/notification...
chrome/browser/notifications/web_notification_delegate.h:25: class
WebNotificationDelegate : public NotificationDelegate {
On 2017/06/13 22:56:42, Peter Beverloo wrote:
> Eventually the Message Center display bridge will create this, right? The name
> WebNotificationDelegate isn't really appropriate for extensions. Do you have
> thoughts on what we might call it eventually?

Yes, at this point the only reason why we cannot create it in the message center
directly is the delegate_id() method which requires another round of
refactoring. Almost there.

Agreed that WebNotificationDelegate is not great for extensions.
NotificationDelegate per se is also not good since there are other (non web and
non extension) notifications as well. Ideas welcome. They are essentially
DeveloperGeneratedNotifications but that sound pretty horrible :)

https://codereview.chromium.org/2928303002/diff/1/content/test/mock_platform_...
File content/test/mock_platform_notification_service.cc (right):

https://codereview.chromium.org/2928303002/diff/1/content/test/mock_platform_...
content/test/mock_platform_notification_service.cc:169:
content::NotificationEventDispatcher::GetInstance()
On 2017/06/13 22:56:42, Peter Beverloo wrote:
> nit: drop `content`, we are in that namespace

Done.

https://codereview.chromium.org/2928303002/diff/1/content/test/mock_platform_...
content/test/mock_platform_notification_service.cc:171:
non_persistent_notifications_.erase(non_persistent_iter);
On 2017/06/13 22:56:42, Peter Beverloo wrote:
> Just call n_p_n_.erase() with |notification_id| if we don't use the iterator
for
> other purposes.

Done.

[Peter Beverloo, 2017-06-14 10:06:08]

On 2017/06/14 09:39:39, Miguel Garcia wrote:
> TBR avi for content/test/mock_platform_notification_service.cc

fwiw we own that file through per-file owners

[commit-bot: I haz the power, 2017-06-14 11:01:22]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497433160824780,
"parent_rev": "be89d3153d2c173742d473cc3e5b04e12770c507", "commit_rev":
"d95830c60e22fa695f0521004149caf03bcc2f77"}

[commit-bot: I haz the power, 2017-06-14 11:01:35]

Description was changed from

==========
Deprecate native and extension notification delegates.

WebNotificationDelegate can now handle extensions and all web
notifications as a result.

TBR=avi@chromium.org
BUG=720345
==========

to

==========
Deprecate native and extension notification delegates.

WebNotificationDelegate can now handle extensions and all web
notifications as a result.

TBR=avi@chromium.org
BUG=720345

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

[commit-bot: I haz the power, 2017-06-14 11:01:36]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d95830c60e22fa695f0521004149...

[Henrik Grunell, 2017-06-14 12:32:00]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2941743002/ by grunell@chromium.org.

The reason for reverting is: Breaks
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty....