Check embargo status in PlatformNotificationServiceImpl::CheckPermissionOnIOThread

Check embargo status in PlatformNotificationServiceImpl::CheckPermissionOnIOThread

window.Notification.permission directly checks the
HostContentSettingsMap for notification permission. This means that if
the permission is embargoed, it will incorrectly return "default"
instead of "denied".

This CL refactors PermissionDecisionAutoBlocker::GetEmbargoResult to be
a static method that can be called with a HostContentSettingsMap
argument. This allows the PlatformNotificationServiceImpl (which is
running on the IO thread) to check the embargo status of the
notification permission and return the correct value.

The long-term fix here is that PermissionManager::GetPermissionStatus
must be made thread safe so that there is one and only one way to
correctly query permission status from any thread.

BUG=730273
Review-Url: https://codereview.chromium.org/2926773002
Cr-Commit-Position: refs/heads/master@{#477856}
Committed: https://chromium.googlesource.com/chromium/src/+/66e276cfca435df9c8c5d42580a6a88406b311fe

[dominickn, 2017-06-07 01:53:03]

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

[commit-bot: I haz the power, 2017-06-07 01:53:22]

Dry run: CQ is trying da patch.

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

[dominickn, 2017-06-07 01:54:06]

dominickn@chromium.org changed reviewers:
+ miguelg@chromium.org, raymes@chromium.org

[dominickn, 2017-06-07 01:54:06]

raymes: PTAL at permissions/

miguelg: PTAL at notifications/

This is something that should be merged to M60.

[commit-bot: I haz the power, 2017-06-07 02:53:50]

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

[commit-bot: I haz the power, 2017-06-07 02:53:50]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-06-07 02:57:53]

raymes@chromium.org changed reviewers:
+ johnme@chromium.org

[raymes, 2017-06-07 02:57:54]

lgtm thanks Dom. It would be good to add a regression test for this but I'm not
sure where the end-to-end tests are for notifications. The notifications folks
would know. +johnme

https://codereview.chromium.org/2926773002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_decision_auto_blocker.h (right):

https://codereview.chromium.org/2926773002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_decision_auto_blocker.h:66: // possible
(e.g. we're on the IO thread).
nit: The last sentence is a little hard to understand. Perhaps just say:

Prefer to use the PermissionManager::GetPermissionStatus when possible. This is
only currently exposed to facilitate permission checks from threads other than
the UI thread.

Also you can link to crbug.com/658020.

[Miguel Garcia, 2017-06-07 14:15:38]

https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio...
is a good e2e test where this could be tested.

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

https://codereview.chromium.org/2926773002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:293: //
TODO(crbug.com/730290): make PermissionManager::GetPermissionStatus thread
That bug is duped to  crbug.com/658020 assigned to raymes. Would it make sense
to change it to that one?

[Miguel Garcia, 2017-06-07 14:15:47]

lgtm

[dominickn, 2017-06-08 00:59:27]

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

[commit-bot: I haz the power, 2017-06-08 00:59:45]

Dry run: CQ is trying da patch.

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

[dominickn, 2017-06-08 01:03:04]

Thanks!

I've uploaded an end-to-end test for Notification.permission at
https://codereview.chromium.org/2925203003. I split it out in the interests of
keeping the merge to M60 simple, and verified that the test fails without this
patch.

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

https://codereview.chromium.org/2926773002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:293: //
TODO(crbug.com/730290): make PermissionManager::GetPermissionStatus thread
On 2017/06/07 14:15:38, Miguel Garcia wrote:
> That bug is duped to  crbug.com/658020 assigned to raymes. Would it make sense
> to change it to that one?

Done.

https://codereview.chromium.org/2926773002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_decision_auto_blocker.h (right):

https://codereview.chromium.org/2926773002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_decision_auto_blocker.h:66: // possible
(e.g. we're on the IO thread).
On 2017/06/07 02:57:54, raymes wrote:
> nit: The last sentence is a little hard to understand. Perhaps just say:
> 
> Prefer to use the PermissionManager::GetPermissionStatus when possible. This
is
> only currently exposed to facilitate permission checks from threads other than
> the UI thread.
> 
> Also you can link to crbug.com/658020.

Done.

[commit-bot: I haz the power, 2017-06-08 01:48:42]

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

[commit-bot: I haz the power, 2017-06-08 01:48:43]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-06-08 01:52:02]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-06-08 01:52:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
miguelg@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2926773002/#ps20001
(title: "Nits")

[commit-bot: I haz the power, 2017-06-08 01:52:20]

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-06-08 01:56:55]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496886722756710,
"parent_rev": "e19cb57e87b162eab01d5f1a51a2173c39062bf7", "commit_rev":
"66e276cfca435df9c8c5d42580a6a88406b311fe"}

[commit-bot: I haz the power, 2017-06-08 01:57:06]

Description was changed from

==========
Check embargo status in
PlatformNotificationServiceImpl::CheckPermissionOnIOThread

window.Notification.permission directly checks the
HostContentSettingsMap for notification permission. This means that if
the permission is embargoed, it will incorrectly return "default"
instead of "denied".

This CL refactors PermissionDecisionAutoBlocker::GetEmbargoResult to be
a static method that can be called with a HostContentSettingsMap
argument. This allows the PlatformNotificationServiceImpl (which is
running on the IO thread) to check the embargo status of the
notification permission and return the correct value.

The long-term fix here is that PermissionManager::GetPermissionStatus
must be made thread safe so that there is one and only one way to
correctly query permission status from any thread.

BUG=730273
==========

to

==========
Check embargo status in
PlatformNotificationServiceImpl::CheckPermissionOnIOThread

window.Notification.permission directly checks the
HostContentSettingsMap for notification permission. This means that if
the permission is embargoed, it will incorrectly return "default"
instead of "denied".

This CL refactors PermissionDecisionAutoBlocker::GetEmbargoResult to be
a static method that can be called with a HostContentSettingsMap
argument. This allows the PlatformNotificationServiceImpl (which is
running on the IO thread) to check the embargo status of the
notification permission and return the correct value.

The long-term fix here is that PermissionManager::GetPermissionStatus
must be made thread safe so that there is one and only one way to
correctly query permission status from any thread.

BUG=730273

Review-Url: https://codereview.chromium.org/2926773002
Cr-Commit-Position: refs/heads/master@{#477856}
Committed:
https://chromium.googlesource.com/chromium/src/+/66e276cfca435df9c8c5d42580a6...
==========

[commit-bot: I haz the power, 2017-06-08 01:57:07]

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