Allow script to request durable storage permission (chrome side)

Allow script to request durable storage permission (chrome side)

- Entry point from blink is PermissionDispatcher::requestPermission
- IPC is handled internally by the permission service
- Permission service stores user's choice in content settings

Corresponding Blink change, which must land first, is
https://codereview.chromium.org/1154573005/

BUG=482814
Committed: https://crrev.com/388c056b78a6cf1f43520e6c44b7ffcc57e7de6c
Cr-Commit-Position: refs/heads/master@{#343468}

[dgrogan, 2015-06-19 18:47:17]

Patchset #9 (id:160001) has been deleted

[dgrogan, 2015-06-19 22:20:21]

dgrogan@chromium.org changed reviewers:
+ michaeln@chromium.org, mlamouri@chromium.org

[dgrogan, 2015-06-19 22:20:23]

Hi Mounir and Michael,

This CL is not ready for landing but I'd appreciate any feedback especially
because I cargo culted a lot of it, copying from midi which recently did
something similar. I probably did some stupid stuff along the way.

Apologies for some temporary class names like WebSomeCallbacks and a few
LOG(ERROR) debug statements.

Michael, could you look at storage/ and chrome/browser/extensions? Mounir, could
you look at the rest?

[dgrogan, 2015-07-01 00:03:15]

I'm going to refactor this into >1 patches. The first will just add the new
durable storage content setting enum and prefs boilerplate.

[mlamouri (slow - plz ping), 2015-07-02 09:53:01]

Thanks for your CL dgrogan@. Given that you said it will be split in multiple
patches, I only had a quick look and left some comments. Let me know if I should
have a deeper look at that specific CL as-is.

https://codereview.chromium.org/1164073005/diff/260001/chrome/browser/content...
File chrome/browser/content_settings/permission_bubble_request_impl.cc (right):

https://codereview.chromium.org/1164073005/diff/260001/chrome/browser/content...
chrome/browser/content_settings/permission_bubble_request_impl.cc:59: //
TODO(dgrogan): Get a real icon from rolfe@.
nit: could you have a bug number?

https://codereview.chromium.org/1164073005/diff/260001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_infobar_delegate.h (left):

https://codereview.chromium.org/1164073005/diff/260001/chrome/browser/durable...
chrome/browser/durable_storage_permission_infobar_delegate.h:19: class
MidiPermissionInfoBarDelegate : public PermissionInfobarDelegate {
I thought we were killing infobars. Why do we need that?

https://codereview.chromium.org/1164073005/diff/260001/components/content_set...
File components/content_settings/core/browser/cookie_settings.h (right):

https://codereview.chromium.org/1164073005/diff/260001/components/content_set...
components/content_settings/core/browser/cookie_settings.h:93: void
GetDurableSettings(ContentSettingsForOneType* settings) const;
Why do we need that?

https://codereview.chromium.org/1164073005/diff/260001/content/renderer/durab...
File content/renderer/durable_dispatcher.cc (right):

https://codereview.chromium.org/1164073005/diff/260001/content/renderer/durab...
content/renderer/durable_dispatcher.cc:53: result =
blink::WebDurableStoragePermission::Default;
Why do you use Default and not ASK?

https://codereview.chromium.org/1164073005/diff/260001/content/renderer/durab...
File content/renderer/durable_dispatcher.h (right):

https://codereview.chromium.org/1164073005/diff/260001/content/renderer/durab...
content/renderer/durable_dispatcher.h:29:
blink::WebDurableStoragePermissionCallbacks* callbacks);
Technically, the Blink code could directly call the PermissionDispatcher instead
of going that way.

[dgrogan, 2015-07-07 04:15:10]

dgrogan@chromium.org changed reviewers:
+ felt@chromium.org

[dgrogan, 2015-07-07 04:15:11]

+felt for question about infobars

https://codereview.chromium.org/1164073005/diff/260001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_infobar_delegate.h (left):

https://codereview.chromium.org/1164073005/diff/260001/chrome/browser/durable...
chrome/browser/durable_storage_permission_infobar_delegate.h:19: class
MidiPermissionInfoBarDelegate : public PermissionInfobarDelegate {
On 2015/07/02 09:53:01, Mounir Lamouri wrote:
> I thought we were killing infobars. Why do we need that?

+felt, who recently made permission bubbles the default on some platforms. Are
we killing infobars on all platforms in this release?

https://codereview.chromium.org/1164073005/diff/260001/components/content_set...
File components/content_settings/core/browser/cookie_settings.h (right):

https://codereview.chromium.org/1164073005/diff/260001/components/content_set...
components/content_settings/core/browser/cookie_settings.h:93: void
GetDurableSettings(ContentSettingsForOneType* settings) const;
On 2015/07/02 09:53:01, Mounir Lamouri wrote:
> Why do we need that?

It's called by chrome/browser/extensions/extension_special_storage_policy.cc,
which is consulted by the quota logic to see if an origin should be exempt from
eviction.

Is there a better way?

https://codereview.chromium.org/1164073005/diff/260001/content/renderer/durab...
File content/renderer/durable_dispatcher.cc (right):

https://codereview.chromium.org/1164073005/diff/260001/content/renderer/durab...
content/renderer/durable_dispatcher.cc:53: result =
blink::WebDurableStoragePermission::Default;
On 2015/07/02 09:53:01, Mounir Lamouri wrote:
> Why do you use Default and not ASK?

Default matches the storage spec.

https://codereview.chromium.org/1164073005/diff/260001/content/renderer/durab...
File content/renderer/durable_dispatcher.h (right):

https://codereview.chromium.org/1164073005/diff/260001/content/renderer/durab...
content/renderer/durable_dispatcher.h:29:
blink::WebDurableStoragePermissionCallbacks* callbacks);
On 2015/07/02 09:53:01, Mounir Lamouri wrote:
> Technically, the Blink code could directly call the PermissionDispatcher
instead
> of going that way.

That would involve adding some kind of requestPermission() method to the
WebPermissionClient interface, correct? I will give that a try tomorrow.

[dgrogan, 2015-07-09 02:59:22]

https://codereview.chromium.org/1164073005/diff/260001/content/renderer/durab...
File content/renderer/durable_dispatcher.h (right):

https://codereview.chromium.org/1164073005/diff/260001/content/renderer/durab...
content/renderer/durable_dispatcher.h:29:
blink::WebDurableStoragePermissionCallbacks* callbacks);
On 2015/07/07 04:15:11, dgrogan wrote:
> On 2015/07/02 09:53:01, Mounir Lamouri wrote:
> > Technically, the Blink code could directly call the PermissionDispatcher
> instead
> > of going that way.
> 
> That would involve adding some kind of requestPermission() method to the
> WebPermissionClient interface, correct? I will give that a try tomorrow.

I deleted durable_dispatcher.h in favor of adding requestDurablePermission() to
WebPermissionClient. Take a look and let me know if that's what you had in mind.

[felt, 2015-07-13 18:43:44]

https://codereview.chromium.org/1164073005/diff/260001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_infobar_delegate.h (left):

https://codereview.chromium.org/1164073005/diff/260001/chrome/browser/durable...
chrome/browser/durable_storage_permission_infobar_delegate.h:19: class
MidiPermissionInfoBarDelegate : public PermissionInfobarDelegate {
On 2015/07/07 04:15:11, dgrogan wrote:
> On 2015/07/02 09:53:01, Mounir Lamouri wrote:
> > I thought we were killing infobars. Why do we need that?
> 
> +felt, who recently made permission bubbles the default on some platforms. Are
> we killing infobars on all platforms in this release?

Answered this in e-mail, but repeating here for posterity: need support for
infobars on mobile, but not on desktop.

[felt, 2015-07-13 18:44:02]

felt@chromium.org changed reviewers:
- felt@chromium.org

[mlamouri (slow - plz ping), 2015-07-16 12:21:02]

As said in the Blink Cl, I think this CL should use the
PermissionClient::request() currently being implemented instead of having its
own request() method.

[dgrogan, 2015-08-01 00:52:31]

Mounir, can you review this CL?

It will have to wait for the blink CL to land first, as mentioned over there.

[mlamouri (slow - plz ping), 2015-08-05 13:54:55]

I've left a few comments on the CL, see below. It looks generally good, mostly
nits.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.cc:18: }
nit: remove and use = default instead.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_context.h (right):

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.h:11: class
PermissionRequestID;
nit: you don't seem to need these two forward declare.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.h:16:
~DurableStoragePermissionContext() override;
nit: = default.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.h:17: bool
IsRestrictedToSecureOrigins() const override;
nit: add empty line

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.h:20: 
nit: remove empty line

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_context_factory.h (right):

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context_factory.h:11: class
DurableStoragePermissionContext;
nit: you don't seem to need that fwd declare.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context_factory.h:24:
~DurableStoragePermissionContextFactory() override;
= default again.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_infobar_delegate.h (right):

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_infobar_delegate.h:35:
~DurableStoragePermissionInfoBarDelegate() override;
= default.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_uma_util.cc (right):

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_uma_util.cc:135:
PERMISSION_ACTION_UMA(  // Cargo culted
nit: remove Cargo culted comment.

[dgrogan, 2015-08-07 20:11:44]

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.cc:18: }
On 2015/08/05 13:54:55, Mounir Lamouri wrote:
> nit: remove and use = default instead.

Done.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_context.h (right):

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.h:11: class
PermissionRequestID;
On 2015/08/05 13:54:55, Mounir Lamouri wrote:
> nit: you don't seem to need these two forward declare.

Done.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.h:16:
~DurableStoragePermissionContext() override;
On 2015/08/05 13:54:55, Mounir Lamouri wrote:
> nit: = default.

Done.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.h:17: bool
IsRestrictedToSecureOrigins() const override;
On 2015/08/05 13:54:55, Mounir Lamouri wrote:
> nit: add empty line

Done.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.h:20: 
On 2015/08/05 13:54:55, Mounir Lamouri wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_context_factory.h (right):

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context_factory.h:11: class
DurableStoragePermissionContext;
On 2015/08/05 13:54:55, Mounir Lamouri wrote:
> nit: you don't seem to need that fwd declare.

I think I need it for line 17.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context_factory.h:24:
~DurableStoragePermissionContextFactory() override;
On 2015/08/05 13:54:55, Mounir Lamouri wrote:
> = default again.

Done.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_infobar_delegate.h (right):

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/durable...
chrome/browser/durable_storage_permission_infobar_delegate.h:35:
~DurableStoragePermissionInfoBarDelegate() override;
On 2015/08/05 13:54:55, Mounir Lamouri wrote:
> = default.

Done.

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_uma_util.cc (right):

https://codereview.chromium.org/1164073005/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_uma_util.cc:135:
PERMISSION_ACTION_UMA(  // Cargo culted
On 2015/08/05 13:54:55, Mounir Lamouri wrote:
> nit: remove Cargo culted comment.

Done.

[mlamouri (slow - plz ping), 2015-08-10 14:41:15]

permissions related code lgtm

https://codereview.chromium.org/1164073005/diff/570001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_infobar_delegate.cc (left):

https://codereview.chromium.org/1164073005/diff/570001/chrome/browser/durable...
chrome/browser/durable_storage_permission_infobar_delegate.cc:41: int
MidiPermissionInfoBarDelegate::GetIconID() const {
You will need an icon for the InfoBarDelegate. You should make sure this is
added when there is one for the bubble manager.

[dgrogan, 2015-08-10 22:39:19]

https://codereview.chromium.org/1164073005/diff/570001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_infobar_delegate.cc (left):

https://codereview.chromium.org/1164073005/diff/570001/chrome/browser/durable...
chrome/browser/durable_storage_permission_infobar_delegate.cc:41: int
MidiPermissionInfoBarDelegate::GetIconID() const {
On 2015/08/10 14:41:15, Mounir Lamouri wrote:
> You will need an icon for the InfoBarDelegate. You should make sure this is
> added when there is one for the bubble manager.

Acknowledged.

[dgrogan, 2015-08-11 00:24:20]

dgrogan@chromium.org changed reviewers:
+ jochen@chromium.org, tsepez@chromium.org
- michaeln@chromium.org

[dgrogan, 2015-08-11 00:24:21]

tsepez, can you review permission_service.mojom?

isherman, can you review histograms.xml?

jochen, could you review as OWNER of chrome/browser/ and
content/public/browser/? I think mlamouri has looked over everything related to
permissions, which is the whole point of the CL. Mounir, let me know if I've
misinterpreted.

[dgrogan, 2015-08-11 00:25:04]

dgrogan@chromium.org changed reviewers:
+ isherman@chromium.org

[dgrogan, 2015-08-11 00:25:05]

isherman, can you review histograms.xml?

[Ilya Sherman, 2015-08-11 19:50:51]

histograms.xml

[dgrogan, 2015-08-11 19:56:27]

On 2015/08/11 19:50:51, Ilya Sherman wrote:
> histograms.xml

I hope you meant to add 4 more letters there?

[Ilya Sherman, 2015-08-11 20:24:40]

On 2015/08/11 19:56:27, dgrogan wrote:
> On 2015/08/11 19:50:51, Ilya Sherman wrote:
> > histograms.xml
> 
> I hope you meant to add 4 more letters there?

Oof, sorry.  histograms.xml lgtm.

[Tom Sepez, 2015-08-11 20:34:20]

Mojom LGTM

[jochen (gone - plz use gerrit), 2015-08-12 16:26:51]

please move the durable storage files into a subdirectory of chrome/browser/ and
add an appropriate owners file

[jochen (gone - plz use gerrit), 2015-08-12 16:29:03]

lgtm with my previous comment addressed

https://codereview.chromium.org/1164073005/diff/630001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/1164073005/diff/630001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.cc:20: 
no empty line

[dgrogan, 2015-08-12 19:09:15]

Thanks for the reviews, all.

Created chrome/browser/storage subdir.

https://codereview.chromium.org/1164073005/diff/630001/chrome/browser/durable...
File chrome/browser/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/1164073005/diff/630001/chrome/browser/durable...
chrome/browser/durable_storage_permission_context.cc:20: 
On 2015/08/12 16:29:03, jochen (ooo) wrote:
> no empty line

Done.

[dgrogan, 2015-08-12 19:09:53]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2015-08-12 19:09:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, tsepez@chromium.org, jochen@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1164073005/#ps710001
(title: "fix compile after url_formatter.h changes")

[commit-bot: I haz the power, 2015-08-12 19:10:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164073005/710001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1164073005/710001

[commit-bot: I haz the power, 2015-08-12 19:37:01]

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

[commit-bot: I haz the power, 2015-08-12 19:37:02]

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

[dgrogan, 2015-08-12 22:29:55]

dgrogan@chromium.org changed reviewers:
+ torne@chromium.org

[dgrogan, 2015-08-12 22:29:56]

Torne, can you review  android_webview/browser/aw_permission_manager.cc?

Who should I talk to about enabling durable storage in WebView? Is there a
WebView storage person?

[Torne, 2015-08-14 11:10:02]

android_webview LGTM for now, but we would really prefer to be able to ship this
in webview at the same time as the other platforms if possible.

File a bug about needing the permission plumbing through for this with label
Cr-Mobile-WebView and cc michaelbai@ and myself. Is the permission the only
thing that's needed for this to function in webview?

[dgrogan, 2015-08-14 16:46:10]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2015-08-14 16:46:11]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jochen@chromium.org, isherman@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1164073005/#ps730001
(title: "fix enums in webview")

[commit-bot: I haz the power, 2015-08-14 16:46:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164073005/730001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1164073005/730001

[dgrogan, 2015-08-14 16:50:24]

On 2015/08/14 11:10:02, Torne wrote:
> android_webview LGTM for now, but we would really prefer to be able to ship
this
> in webview at the same time as the other platforms if possible.

Thanks, and understood.

> File a bug about needing the permission plumbing through for this with label
> Cr-Mobile-WebView and cc michaelbai@ and myself. Is the permission the only
> thing that's needed for this to function in webview?

I'm not sure. I'll ask questions on the bug.

[commit-bot: I haz the power, 2015-08-14 17:33:09]

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

[commit-bot: I haz the power, 2015-08-14 17:33:10]

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

[dgrogan, 2015-08-14 17:56:23]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2015-08-14 17:56:24]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jochen@chromium.org, isherman@chromium.org, torne@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1164073005/#ps770001
(title: "add switch entry for vector icon")

[commit-bot: I haz the power, 2015-08-14 17:56:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164073005/770001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1164073005/770001

[commit-bot: I haz the power, 2015-08-14 17:58:01]

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

[commit-bot: I haz the power, 2015-08-14 17:58:02]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgrogan, 2015-08-14 18:50:55]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2015-08-14 18:50:56]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jochen@chromium.org, isherman@chromium.org, torne@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1164073005/#ps780025
(title: "fix conflicts after midi constant permission landed")

[commit-bot: I haz the power, 2015-08-14 18:51:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164073005/780025
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1164073005/780025

[commit-bot: I haz the power, 2015-08-14 20:01:25]

Committed patchset #41 (id:780025)

[commit-bot: I haz the power, 2015-08-14 20:02:06]

Patchset 41 (id:??) landed as
https://crrev.com/388c056b78a6cf1f43520e6c44b7ffcc57e7de6c
Cr-Commit-Position: refs/heads/master@{#343468}