Merge //content/child/permissions into Blink's permissions module.

Merge //content/child/permissions into Blink's permissions module.

The Mojo PermissionService can be used directly from Blink. Creating
separate pipes for workers also removes the need to implement an
additional thread-hopping layer.

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

[Reilly Grant (use Gerrit), 2016-06-30 00:35:00]

Patchset #1 (id:1) has been deleted

[Reilly Grant (use Gerrit), 2016-06-30 18:30:23]

reillyg@chromium.org changed reviewers:
+ haraken@chromium.org, mlamouri@chromium.org

[Reilly Grant (use Gerrit), 2016-06-30 18:30:24]

haraken@, please take a look at this as a general Onion Soup reviewer.
mlamouri@, please take a look at this as owner of the Permissions API.

[haraken, 2016-07-01 02:15:06]

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

[haraken, 2016-07-01 02:15:08]

I just scanned the CL but this makes a lot of sense! It's always to remove code
:)

[haraken, 2016-07-01 02:15:27]

On 2016/07/01 02:15:08, haraken wrote:
> I just scanned the CL but this makes a lot of sense! It's always to remove
code
> :)

s/always/always good/

[Reilly Grant (use Gerrit), 2016-07-12 00:56:40]

Ping. Mounir, can you take a look at this?

[mlamouri (slow - plz ping), 2016-07-18 10:25:57]

Thanks for doing this. Also, can you open a bug and maybe have it point to the
meta onion soup bug if there is one?

lgtm with comments applied.

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/PermissionStatus.h (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionStatus.h:62:
mojom::blink::PermissionServicePtr m_service;
nit: you could add some `using` statements inside the class block. Not super
important but would help reduce the size of these types :)

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:216:
m_service.set_connection_error_handler(createBaseCallback(bind(&Permissions::serviceConnectionError,
wrapWeakPersistent(this))));
See below (in Storage)

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/Permissions.h (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.h:22: : public
GarbageCollectedFinalized<Permissions>
Do you need this change? If I understand correctly, PermissionServicePtr is a
InterfacePtr<PermissionService> and that has an empty dtor. Doesn't that mean
the GC doesn't need to be Finalized?

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/quota/StorageManager.cpp:132:
m_permissionService.set_connection_error_handler(createBaseCallback(bind(&StorageManager::permissionServiceConnectionError,
wrapWeakPersistent(this))));
It's a bit odd because you don't handle a branch.

either you do something like:
```
if (Permissions::connectToService(...))
  m_permissionService.set_connection_error_handler(...);
else
  return nullptr;
```

or you merge the two conditions:
```
if (!m_permissionService && Permissions::connectToService(...))
  m_permissionService.set_connection_error_handler(...);
```

(Note: I know that the result would be the same, it's a clarity issue.)

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/quota/StorageManager.h (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/quota/StorageManager.h:20: : public
GarbageCollectedFinalized<StorageManager>
Do you need this change? If I understand correctly, PermissionServicePtr is a
InterfacePtr<PermissionService> and that has an empty dtor. Doesn't that mean
the GC doesn't need to be Finalized?

[esprehn, 2016-07-19 04:00:06]

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:17: using
mojom::blink::PermissionName;
MojoPermissionName ? it's weird to alias the two types differently right next to
each other

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:64:
m_service->GetNextPermissionChange(m_name,
getExecutionContext()->getSecurityOrigin()->toString(), m_status,
createBaseCallback(bind(&PermissionStatus::permissionChanged,
wrapWeakPersistent(this))));
createBaseCallback is gone, you can use WTF::bind now I think

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:91:
m_service->GetNextPermissionChange(m_name,
getExecutionContext()->getSecurityOrigin()->toString(), m_status,
createBaseCallback(bind(&PermissionStatus::permissionChanged,
wrapWeakPersistent(this))));
ditto

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:241:
HeapVector<Member<PermissionStatus>> result(callerIndexToInternalIndex.size());
this is actually calling the default constructor, did you want to do

HeapVector<Member<PermissionStatus>> result;
result.reserveInitialCapacity(...);

and use .append() below? then you can also write a range loop

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/Permissions.h (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.h:22: : public
GarbageCollectedFinalized<Permissions>
On 2016/07/18 at 10:25:57, Mounir Lamouri wrote:
> Do you need this change? If I understand correctly, PermissionServicePtr is a
InterfacePtr<PermissionService> and that has an empty dtor. Doesn't that mean
the GC doesn't need to be Finalized?

Yeah this code is very confusing,
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_ptr.h...
claims it closes the message pipe but there's no code in there, is that comment
just wrong?

[Reilly Grant (use Gerrit), 2016-07-19 20:10:22]

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

[Reilly Grant (use Gerrit), 2016-07-19 20:10:28]

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:17: using
mojom::blink::PermissionName;
On 2016/07/19 at 04:00:05, esprehn wrote:
> MojoPermissionName ? it's weird to alias the two types differently right next
to each other

I needed to use MojoPermissionStatus to avoid a conflict with the Blink type
name. I'll alias this to MojoPermissionName as well so it is less confusing.

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionStatus.cpp:64:
m_service->GetNextPermissionChange(m_name,
getExecutionContext()->getSecurityOrigin()->toString(), m_status,
createBaseCallback(bind(&PermissionStatus::permissionChanged,
wrapWeakPersistent(this))));
On 2016/07/19 at 04:00:05, esprehn wrote:
> createBaseCallback is gone, you can use WTF::bind now I think

Someone just renamed it to convertToBaseCallback and removed "using WTF::bind"
from the end of wtf/Functional.h. I'm guessing it's a step towards actually
making WTF::bind return base::Callbacks.

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/PermissionStatus.h (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/PermissionStatus.h:62:
mojom::blink::PermissionServicePtr m_service;
On 2016/07/18 at 10:25:57, Mounir Lamouri wrote:
> nit: you could add some `using` statements inside the class block. Not super
important but would help reduce the size of these types :)

Done.

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:216:
m_service.set_connection_error_handler(createBaseCallback(bind(&Permissions::serviceConnectionError,
wrapWeakPersistent(this))));
On 2016/07/18 at 10:25:57, Mounir Lamouri wrote:
> See below (in Storage)

Done.

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:241:
HeapVector<Member<PermissionStatus>> result(callerIndexToInternalIndex.size());
On 2016/07/19 at 04:00:05, esprehn wrote:
> this is actually calling the default constructor, did you want to do
> 
> HeapVector<Member<PermissionStatus>> result;
> result.reserveInitialCapacity(...);
> 
> and use .append() below? then you can also write a range loop

Done.

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/Permissions.h (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.h:22: : public
GarbageCollectedFinalized<Permissions>
On 2016/07/19 at 04:00:06, esprehn wrote:
> On 2016/07/18 at 10:25:57, Mounir Lamouri wrote:
> > Do you need this change? If I understand correctly, PermissionServicePtr is
a InterfacePtr<PermissionService> and that has an empty dtor. Doesn't that mean
the GC doesn't need to be Finalized?
> 
> Yeah this code is very confusing,
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_ptr.h...
> claims it closes the message pipe but there's no code in there, is that
comment just wrong?

The comment is not wrong. Destroying the InterfacePtr destroys its
internal::InterfacePtrState.

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/quota/StorageManager.cpp:132:
m_permissionService.set_connection_error_handler(createBaseCallback(bind(&StorageManager::permissionServiceConnectionError,
wrapWeakPersistent(this))));
On 2016/07/18 at 10:25:57, Mounir Lamouri wrote:
> It's a bit odd because you don't handle a branch.
> 
> either you do something like:
> ```
> if (Permissions::connectToService(...))
>   m_permissionService.set_connection_error_handler(...);
> else
>   return nullptr;
> ```
> 
> or you merge the two conditions:
> ```
> if (!m_permissionService && Permissions::connectToService(...))
>   m_permissionService.set_connection_error_handler(...);
> ```
> 
> (Note: I know that the result would be the same, it's a clarity issue.)

I took the second option.

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/quota/StorageManager.h (right):

https://codereview.chromium.org/2108003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/quota/StorageManager.h:20: : public
GarbageCollectedFinalized<StorageManager>
On 2016/07/18 at 10:25:57, Mounir Lamouri wrote:
> Do you need this change? If I understand correctly, PermissionServicePtr is a
InterfacePtr<PermissionService> and that has an empty dtor. Doesn't that mean
the GC doesn't need to be Finalized?

InterfacePtr's destructor is non-trivial as it destroys the
internal::InterfacePtrState which is what closes the Mojo pipe.

[commit-bot: I haz the power, 2016-07-19 20:11:04]

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-07-19 20:12:41]

Description was changed from

==========
Merge //content/child/permissions into Blink's permissions module.

The Mojo PermissionService can be used directly from Blink. Creating
separate pipes for workers also removes the need to implement an
additional thread-hopping layer.

BUG=None
==========

to

==========
Merge //content/child/permissions into Blink's permissions module.

The Mojo PermissionService can be used directly from Blink. Creating
separate pipes for workers also removes the need to implement an
additional thread-hopping layer.

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

[commit-bot: I haz the power, 2016-07-19 23:06:55]

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

[commit-bot: I haz the power, 2016-07-19 23:06:57]

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

[esprehn, 2016-07-21 22:08:26]

lgtm

[Reilly Grant (use Gerrit), 2016-07-21 23:12:00]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-07-21 23:12:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2108003002/#ps60001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-07-21 23:12:37]

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-07-22 01:43:18]

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

[commit-bot: I haz the power, 2016-07-22 01:43:19]

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

[Reilly Grant (use Gerrit), 2016-07-22 02:14:20]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2016-07-22 02:15:09]

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-07-22 03:20:06]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-22 03:23:25]

Description was changed from

==========
Merge //content/child/permissions into Blink's permissions module.

The Mojo PermissionService can be used directly from Blink. Creating
separate pipes for workers also removes the need to implement an
additional thread-hopping layer.

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

to

==========
Merge //content/child/permissions into Blink's permissions module.

The Mojo PermissionService can be used directly from Blink. Creating
separate pipes for workers also removes the need to implement an
additional thread-hopping layer.

BUG=561879

Committed: https://crrev.com/9f21f26bc57f97075cc85c2fa87e0c4973407433
Cr-Commit-Position: refs/heads/master@{#407014}
==========

[commit-bot: I haz the power, 2016-07-22 03:23:27]

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