Remove content::NotificationPermissionDispatcher.

Remove content::NotificationPermissionDispatcher.

blink::Notification can make calls to the Mojo PermissionService itself.

BUG=561879
Committed: https://crrev.com/be876eec511985fc0f476628b7b43dc951bd04c5
Cr-Commit-Position: refs/heads/master@{#412636}

[Reilly Grant (use Gerrit), 2016-08-12 23:15:35]

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

[commit-bot: I haz the power, 2016-08-12 23:16:18]

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

[Reilly Grant (use Gerrit), 2016-08-12 23:16:22]

reillyg@chromium.org changed reviewers:
+ esprehn@chromium.org

[Reilly Grant (use Gerrit), 2016-08-12 23:16:23]

I noticed this little bit of low-hanging fruit. Please take a look.

[commit-bot: I haz the power, 2016-08-12 23:19:54]

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

[commit-bot: I haz the power, 2016-08-12 23:19:55]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[esprehn, 2016-08-12 23:22:42]

On 2016/08/12 at 23:16:23, reillyg wrote:
> I noticed this little bit of low-hanging fruit. Please take a look.

All your bots are red?

[esprehn, 2016-08-12 23:24:32]

Patch looks pretty good though :)

https://codereview.chromium.org/2244913002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2244913002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/Notification.cpp:366:
rawPermissionService->RequestPermission(
I thought the Ptr wrapper could be called through here? I don't think you
usually need to call .get() manually like this

[Reilly Grant (use Gerrit), 2016-08-12 23:28:59]

https://codereview.chromium.org/2244913002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2244913002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/Notification.cpp:366:
rawPermissionService->RequestPermission(
On 2016/08/12 at 23:24:32, esprehn wrote:
> I thought the Ptr wrapper could be called through here? I don't think you
usually need to call .get() manually like this

The arguments will be evaluated before the object pointer is dereferenced so
permissionService will be invalid because it is moved into the WTF::Function.

[Reilly Grant (use Gerrit), 2016-08-13 00:45:35]

Rebased.

[Reilly Grant (use Gerrit), 2016-08-13 00:45:45]

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

[commit-bot: I haz the power, 2016-08-13 00:46:11]

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-08-13 00:49:04]

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

[commit-bot: I haz the power, 2016-08-13 00:49:05]

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

[haraken, 2016-08-13 01:54:31]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-08-13 01:54:32]

https://codereview.chromium.org/2244913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2244913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:360:
Permissions::connectToService(context, mojo::GetProxy(&permissionService));

I'd prefer connecting to the service in the constructor and save it to
m_permissionService.

Currently m_permissionService is unused.

https://codereview.chromium.org/2244913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:368:
convertToBaseCallback(WTF::bind(&Notification::onPermissionRequestComplete,
passed(std::move(permissionService)), wrapPersistent(resolver),
wrapPersistent(deprecatedCallback))));

Instead of moving the ownership of the service, I'd prefer keeping it in
m_permissionService.

[Reilly Grant (use Gerrit), 2016-08-15 17:55:01]

https://codereview.chromium.org/2244913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2244913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:360:
Permissions::connectToService(context, mojo::GetProxy(&permissionService));
On 2016/08/13 at 01:54:32, haraken wrote:
> I'd prefer connecting to the service in the constructor and save it to
m_permissionService.
> 
> Currently m_permissionService is unused.

I forgot to remove it. This is a static function so it cannot access
m_permissionService. I should perhaps move m_permissionService into
NotificationManager so that the connection is persisted there.

[Reilly Grant (use Gerrit), 2016-08-15 19:56:41]

I've moved the new service connection to NotificationManager. Please take a
look.

haraken@, I'd rather create the connections on demand. What is the reason to
prefer creating them in the constructor?

[Peter Beverloo, 2016-08-15 19:57:31]

peter@chromium.org changed reviewers:
+ johnme@chromium.org, peter@chromium.org

[Peter Beverloo, 2016-08-15 19:57:31]

+johnme for reviewing this from a notifications pov

[esprehn, 2016-08-15 20:07:48]

https://codereview.chromium.org/2244913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(right):

https://codereview.chromium.org/2244913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:54: if
(!m_notificationService)
why does permissionStatus() do:

       
Platform::current()->interfaceProvider()->getInterface(mojo::GetProxy(&m_notificationService));


but requestPermission() do:

    ExecutionContext* context = scriptState->getExecutionContext();
    if (!m_permissionService)
        Permissions::connectToService(context,
mojo::GetProxy(&m_permissionService));

that means the order in which you call these two methods changes where the proxy
is from. I'd rather not do that.

[dcheng, 2016-08-15 20:10:36]

On 2016/08/15 19:57:31, Peter Beverloo- OOO until 22nd wrote:
> +johnme for reviewing this from a notifications pov

FWIW, I don't think there's any good guidance around when to reuse vs when to
just keep the InterfacePtr around. Here's what I do know:

1) It is cheaper to set up a mojo interface connection, but it's still not free.
2) Some Mojo services are relatively rarely used (once per frame load) and so
the CL authors just chose to connect / disconnect every time they want to make
an interface call. For example:
https://codereview.chromium.org/2072613003/diff/610001/chrome/renderer/extern...
3) Other Mojo services don't want to incur the connection cost on construction,
so they lazily connect the first time the interface is used:
https://codereview.chromium.org/2007473004/
4) Other Mojo services just connect in the constructor.

The advantage of (2) is you don't pay the cost of connecting unless you use the
service. The disadvantage is it doesn't look like a lot of other code, and it's
potentially more copy and paste (for example, if SearchInstallProvider had more
methods, we might have had to copy and paste the mojo service connection into
more places)
The advantage of (3) is that you don't pay the cost of connecting until you use
the service. The disadvantage is that you need to write a small function to
lazily connect to the interface, and everything needs to go through your helper
function.
The advantage of (4) is the code looks simple, but you pay the connection cost
up front, even if you don't use the service. It also costs a small amount of
memory.

Personally, I'd prefer (4) unless there's evidence that (2) or (3) are really
valuable. This might be a good chromium-mojo@ discussion.

[Reilly Grant (use Gerrit), 2016-08-15 20:13:28]

On 2016/08/15 at 20:07:48, esprehn wrote:
>
https://codereview.chromium.org/2244913002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
(right):
> 
>
https://codereview.chromium.org/2244913002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/notifications/NotificationManager.cpp:54: if
(!m_notificationService)
> why does permissionStatus() do:
> 
>        
Platform::current()->interfaceProvider()->getInterface(mojo::GetProxy(&m_notificationService));
> 
> 
> but requestPermission() do:
> 
>     ExecutionContext* context = scriptState->getExecutionContext();
>     if (!m_permissionService)
>         Permissions::connectToService(context,
mojo::GetProxy(&m_permissionService));
> 
> that means the order in which you call these two methods changes where the
proxy is from. I'd rather not do that.

These are two different services and two different private fields.

[haraken, 2016-08-15 21:31:44]

LGTM

On 2016/08/15 20:10:36, dcheng wrote:
> On 2016/08/15 19:57:31, Peter Beverloo- OOO until 22nd wrote:
> > +johnme for reviewing this from a notifications pov
> 
> FWIW, I don't think there's any good guidance around when to reuse vs when to
> just keep the InterfacePtr around. Here's what I do know:
> 
> 1) It is cheaper to set up a mojo interface connection, but it's still not
free.
> 2) Some Mojo services are relatively rarely used (once per frame load) and so
> the CL authors just chose to connect / disconnect every time they want to make
> an interface call. For example:
>
https://codereview.chromium.org/2072613003/diff/610001/chrome/renderer/extern...
> 3) Other Mojo services don't want to incur the connection cost on
construction,
> so they lazily connect the first time the interface is used:
> https://codereview.chromium.org/2007473004/
> 4) Other Mojo services just connect in the constructor.
> 
> The advantage of (2) is you don't pay the cost of connecting unless you use
the
> service. The disadvantage is it doesn't look like a lot of other code, and
it's
> potentially more copy and paste (for example, if SearchInstallProvider had
more
> methods, we might have had to copy and paste the mojo service connection into
> more places)
> The advantage of (3) is that you don't pay the cost of connecting until you
use
> the service. The disadvantage is that you need to write a small function to
> lazily connect to the interface, and everything needs to go through your
helper
> function.
> The advantage of (4) is the code looks simple, but you pay the connection cost
> up front, even if you don't use the service. It also costs a small amount of
> memory.
> 
> Personally, I'd prefer (4) unless there's evidence that (2) or (3) are really
> valuable. This might be a good chromium-mojo@ discussion.

Yeah, let's discuss there (although we won't need to block this CL by that).

[johnme, 2016-08-16 12:41:54]

Much cleaner - thanks! lgtm from a notifications perspective; I'll defer to
others on the Mojo aspects.

[Michael van Ouwerkerk, 2016-08-16 12:44:43]

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

[commit-bot: I haz the power, 2016-08-16 12:44:57]

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-08-16 12:47:32]

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

[commit-bot: I haz the power, 2016-08-16 12:47:33]

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

[Michael van Ouwerkerk, 2016-08-16 13:01:10]

rs lgtm module red bots

https://codereview.chromium.org/2244913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (left):

https://codereview.chromium.org/2244913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:362:
DCHECK(context->activeDOMObjectsAreStopped());
Is this case handled in the new code?

[Reilly Grant (use Gerrit), 2016-08-16 17:46:37]

https://codereview.chromium.org/2244913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (left):

https://codereview.chromium.org/2244913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:362:
DCHECK(context->activeDOMObjectsAreStopped());
On 2016/08/16 at 13:01:10, Michael van Ouwerkerk wrote:
> Is this case handled in the new code?

Yes, if the browser-side Mojo service isn't available then callback will simply
never be called so we get the same behavior.

[Reilly Grant (use Gerrit), 2016-08-16 19:52:44]

Rebased.

[Reilly Grant (use Gerrit), 2016-08-16 19:52:49]

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

[commit-bot: I haz the power, 2016-08-16 19:53:11]

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-08-16 19:59:05]

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

[commit-bot: I haz the power, 2016-08-16 19:59:06]

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

[Reilly Grant (use Gerrit), 2016-08-17 17:10:04]

Rebased.

[Reilly Grant (use Gerrit), 2016-08-17 17:10:16]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-08-17 17:10:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, johnme@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2244913002/#ps80001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-08-17 17:10:50]

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-08-17 17:18:20]

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

[commit-bot: I haz the power, 2016-08-17 17:18:22]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[esprehn, 2016-08-17 18:19:57]

lgtm

[Reilly Grant (use Gerrit), 2016-08-17 18:20:51]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2016-08-17 18:21:27]

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-08-17 21:11:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-17 21:14:00]

Description was changed from

==========
Remove content::NotificationPermissionDispatcher.

blink::Notification can make calls to the Mojo PermissionService itself.

BUG=561879
==========

to

==========
Remove content::NotificationPermissionDispatcher.

blink::Notification can make calls to the Mojo PermissionService itself.

BUG=561879

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

[commit-bot: I haz the power, 2016-08-17 21:14:01]

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

[megjablon, 2016-08-17 22:55:10]

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

The reason for reverting is: Causing failure on Builder WebKit Linux Leak

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...

request-permission-detached-context.html

15:25:46.371 7635 worker/0
http/tests/notifications/request-permission-detached-context.html leaked
15:25:46.371 7635   Xlib:  extension "RANDR" missing on display ":9".
15:25:46.371 7635   Xlib:  extension "RANDR" missing on display ":9".
15:25:46.374 17565 [4/95]
http/tests/notifications/request-permission-detached-context.html failed
unexpectedly (leak detected:
({"numberOfLiveActiveDOMObjects":[2,6],"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,36],"numberOfLiveResources":[0,3]}))
15:25:46.372 7635 worker/0
http/tests/notifications/request-permission-detached-context.html failed:
15:25:46.372 7635 worker/0  leak detected:
({"numberOfLiveActiveDOMObjects":[2,6],"numberOfLiveDocuments":[1,3],"numberOfLiveNodes":[4,36],"numberOfLiveResources":[0,3]}).