Expose Durable Storage API to script

Expose Durable Storage API to script

Exposes 2 javascript functions to script: requestPersistent() and
persistentPermission().

StorageManager calls into the permission service, which handles IPC and
setting or retrieving the appropriate content settings.

Chromium side is here:
https://codereview.chromium.org/1164073005/

BUG=482814
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200362

[dgrogan, 2015-06-19 22:34:06]

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

[dgrogan, 2015-06-19 22:34:08]

Hi Michael and Josh,

This patch is not ready for landing but I'd appreciate feedback. As you can see
I cargo culted a lot of it.

The only layout test just tests that the promise is resolved. I'm in the middle
of fighting through a chrome-side unit test but it is not included in the chrome
side patch because it doesn't work yet.

Thanks for taking a look!

https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/D...
File Source/modules/quota/DurableStorageController.cpp (right):

https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/D...
Source/modules/quota/DurableStorageController.cpp:49: //    m_client = nullptr;
Argh, don't know why this is commented.

https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/D...
File Source/modules/quota/DurableStorageController.h (right):

https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/D...
Source/modules/quota/DurableStorageController.h:21:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(DurableStorageController);
This was all cargo culted.

[michaeln, 2015-06-19 23:24:50]

woohoo!

https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.idl (right):

https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/S...
Source/modules/quota/StorageManager.idl:14: Promise<PersistentStoragePermission>
persistentPermission();
> Exposed=(Window,Worker)

Is the worker part of that true? The strategy used in this blink cl and its
chromium sibling looks very 'window' oriented. What's the plan for the worker
part?

Did you take a look at how StorageQuota and queryStorageUsageAndQuota is put
together in blink and chromium renderer code? That strategy works for both
windows and workers. If these new durable perms interfaces really do need to
work in workers too, we might want to do something more along those lines.

[dgrogan, 2015-06-30 00:48:44]

Michael, thanks so much for taking a look.

https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.idl (right):

https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/S...
Source/modules/quota/StorageManager.idl:14: Promise<PersistentStoragePermission>
persistentPermission();
On 2015/06/19 23:24:50, michaeln wrote:
> > Exposed=(Window,Worker)
> 
> Is the worker part of that true? The strategy used in this blink cl and its
> chromium sibling looks very 'window' oriented. What's the plan for the worker
> part?

The plan is that querying with persistentPermission will be exposed to workers
but requestPersistent() will only be exposed to window. I have a question out on
an IDL bug about how to best support this.
(https://code.google.com/p/chromium/issues/detail?id=503505#c2)

> Did you take a look at how StorageQuota and queryStorageUsageAndQuota is put
> together in blink and chromium renderer code?

Yeah, definitely. I even had a few patches up
(https://codereview.chromium.org/1102353009/
https://codereview.chromium.org/1110063011/) that followed the old convention.

> That strategy works for both windows and workers.

Could you give more detail about what you mean by "that strategy"?

> If these new durable perms interfaces really do need to
> work in workers too, we might want to do something more along those lines.

[michaeln, 2015-07-01 01:32:03]

On 2015/06/30 00:48:44, dgrogan wrote:
> Michael, thanks so much for taking a look.
> 
>
https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/S...
> File Source/modules/quota/StorageManager.idl (right):
> 
>
https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/S...
> Source/modules/quota/StorageManager.idl:14:
Promise<PersistentStoragePermission>
> persistentPermission();
> On 2015/06/19 23:24:50, michaeln wrote:
> > > Exposed=(Window,Worker)
> > 
> > Is the worker part of that true? The strategy used in this blink cl and its
> > chromium sibling looks very 'window' oriented. What's the plan for the
worker
> > part?
> 
> The plan is that querying with persistentPermission will be exposed to workers
> but requestPersistent() will only be exposed to window. I have a question out
on
> an IDL bug about how to best support this.
> (https://code.google.com/p/chromium/issues/detail?id=503505#c2)
> 
> > Did you take a look at how StorageQuota and queryStorageUsageAndQuota is put
> > together in blink and chromium renderer code?
> 
> Yeah, definitely. I even had a few patches up
> (https://codereview.chromium.org/1102353009/
> https://codereview.chromium.org/1110063011/) that followed the old convention.
> 
> > That strategy works for both windows and workers.
> 
> Could you give more detail about what you mean by "that strategy"?

I think i meant two things:
1) Calls out to the embedder via blink::Platform instead of the WebFrameClient.
That interface is available to both document and worker contexts.
2) Fewer instances of plumbing 'things'. Platform class methods are more or less
global functions so wecore/chromium innards have fewer instances of objects and
lifetimes to manage.

 
> > If these new durable perms interfaces really do need to
> > work in workers too, we might want to do something more along those lines.

[jsbell, 2015-07-01 17:43:30]

Handful of nits...

https://codereview.chromium.org/1154573005/diff/140001/LayoutTests/storage/qu...
File LayoutTests/storage/quota/durability.html (right):

https://codereview.chromium.org/1154573005/diff/140001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability.html:3: <script
src="../../resources/js-test.js"></script>
Can you implement this as a testharness.js test instead?

(We can upstream those and they're usually cleaner as well, no need for
'expected' files)

https://codereview.chromium.org/1154573005/diff/140001/Source/modules/quota/N...
File Source/modules/quota/NavigatorStorageQuota.idl (right):

https://codereview.chromium.org/1154573005/diff/140001/Source/modules/quota/N...
Source/modules/quota/NavigatorStorageQuota.idl:20: partial interface Navigator {
Please add a link to the spec, e.g.

// https://storage.spec.whatwg.org/#api

https://codereview.chromium.org/1154573005/diff/140001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/140001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:39: if (securityOrigin->isUnique()) {
Add a TODO to track https://github.com/whatwg/storage/issues/5 (restricting to
secure origins)

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:40:
resolver->reject(DOMError::create(NotSupportedError));
Reject with DOMException, not DOMError.

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:50: if (!controller) // needed?
Shouldn't be needed. Did you see this test elsewhere?

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:57:
resolver->reject(DOMError::create(NotSupportedError));
Reject with DOMException, not DOMError.

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.idl (right):

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
Source/modules/quota/StorageManager.idl:5: enum PersistentStoragePermission {
"default", "denied", "granted" };
Please add spec link, e.g.

// https://storage.spec.whatwg.org/#storagemanager

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
Source/modules/quota/StorageManager.idl:14: Promise<PersistentStoragePermission>
persistentPermission();
Add TODOs for the parts of the spec that aren't included.

(i.e. document all deviations from the spec apart from
bindings-generation-related extended attributes.)

[dgrogan, 2015-07-02 00:29:43]

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

[dgrogan, 2015-07-02 00:30:06]

On 2015/07/01 01:32:03, michaeln wrote:
> On 2015/06/30 00:48:44, dgrogan wrote:
> > Michael, thanks so much for taking a look.
> > 
> >
>
https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/S...
> > File Source/modules/quota/StorageManager.idl (right):
> > 
> >
>
https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/S...
> > Source/modules/quota/StorageManager.idl:14:
> Promise<PersistentStoragePermission>
> > persistentPermission();
> > On 2015/06/19 23:24:50, michaeln wrote:
> > > > Exposed=(Window,Worker)
> > > 
> > > Is the worker part of that true? The strategy used in this blink cl and
its
> > > chromium sibling looks very 'window' oriented. What's the plan for the
> worker
> > > part?
> > 
> > The plan is that querying with persistentPermission will be exposed to
workers
> > but requestPersistent() will only be exposed to window. I have a question
out
> on
> > an IDL bug about how to best support this.
> > (https://code.google.com/p/chromium/issues/detail?id=503505#c2)
> > 
> > > Did you take a look at how StorageQuota and queryStorageUsageAndQuota is
put
> > > together in blink and chromium renderer code?
> > 
> > Yeah, definitely. I even had a few patches up
> > (https://codereview.chromium.org/1102353009/
> > https://codereview.chromium.org/1110063011/) that followed the old
convention.
> > 
> > > That strategy works for both windows and workers.
> > 
> > Could you give more detail about what you mean by "that strategy"?
> 
> I think i meant two things:
> 1) Calls out to the embedder via blink::Platform instead of the
WebFrameClient.
> That interface is available to both document and worker contexts.
> 2) Fewer instances of plumbing 'things'. Platform class methods are more or
less
> global functions so wecore/chromium innards have fewer instances of objects
and
> lifetimes to manage.

Ah, got it. Yeah, that would be great. I don't know if DurableStorageController
is needed except to support these plumbing acrobatics. The problem is that I
don't know how to get to the permission service from RendererBlinkPlatformImpl.
As is, durable_dispatcher.cc calls
render_frame()->GetServiceRegistry()->ConnectToRemoteService(mojo::GetProxy(&permission_service_)).
I don't think that first render_frame() call is available to anything in
chrome's platform impl.

+Mounir, is there a way to use the permission service from
RendererBlinkPlatformImpl?

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

On 2015/07/02 at 00:30:06, dgrogan wrote:
> On 2015/07/01 01:32:03, michaeln wrote:
> > On 2015/06/30 00:48:44, dgrogan wrote:
> > > Michael, thanks so much for taking a look.
> > > 
> > >
> >
https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/S...
> > > File Source/modules/quota/StorageManager.idl (right):
> > > 
> > >
> >
https://codereview.chromium.org/1154573005/diff/120001/Source/modules/quota/S...
> > > Source/modules/quota/StorageManager.idl:14:
> > Promise<PersistentStoragePermission>
> > > persistentPermission();
> > > On 2015/06/19 23:24:50, michaeln wrote:
> > > > > Exposed=(Window,Worker)
> > > > 
> > > > Is the worker part of that true? The strategy used in this blink cl and
its
> > > > chromium sibling looks very 'window' oriented. What's the plan for the
> > worker
> > > > part?
> > > 
> > > The plan is that querying with persistentPermission will be exposed to
workers
> > > but requestPersistent() will only be exposed to window. I have a question
out
> > on
> > > an IDL bug about how to best support this.
> > > (https://code.google.com/p/chromium/issues/detail?id=503505#c2)
> > > 
> > > > Did you take a look at how StorageQuota and queryStorageUsageAndQuota is
put
> > > > together in blink and chromium renderer code?
> > > 
> > > Yeah, definitely. I even had a few patches up
> > > (https://codereview.chromium.org/1102353009/
> > > https://codereview.chromium.org/1110063011/) that followed the old
convention.
> > > 
> > > > That strategy works for both windows and workers.
> > > 
> > > Could you give more detail about what you mean by "that strategy"?
> > 
> > I think i meant two things:
> > 1) Calls out to the embedder via blink::Platform instead of the
WebFrameClient.
> > That interface is available to both document and worker contexts.
> > 2) Fewer instances of plumbing 'things'. Platform class methods are more or
less
> > global functions so wecore/chromium innards have fewer instances of objects
and
> > lifetimes to manage.
> 
> Ah, got it. Yeah, that would be great. I don't know if
DurableStorageController is needed except to support these plumbing acrobatics.
The problem is that I don't know how to get to the permission service from
RendererBlinkPlatformImpl. As is, durable_dispatcher.cc calls
render_frame()->GetServiceRegistry()->ConnectToRemoteService(mojo::GetProxy(&permission_service_)).
I don't think that first render_frame() call is available to anything in
chrome's platform impl.
> 
> +Mounir, is there a way to use the permission service from
RendererBlinkPlatformImpl?

Sure. BlinkPlatformImpl has a permission_client_. RendererBlinkPlatformImpl
inherits from BlinkPlatformImpl. I'm using this for the Permissions API.

[dgrogan, 2015-07-07 04:03:25]

https://codereview.chromium.org/1154573005/diff/140001/LayoutTests/storage/qu...
File LayoutTests/storage/quota/durability.html (right):

https://codereview.chromium.org/1154573005/diff/140001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability.html:3: <script
src="../../resources/js-test.js"></script>
On 2015/07/01 17:43:29, jsbell wrote:
> Can you implement this as a testharness.js test instead?
> 
> (We can upstream those and they're usually cleaner as well, no need for
> 'expected' files)

I switched to a tiny testharness test but it's not sufficient yet. Questions to
follow.

https://codereview.chromium.org/1154573005/diff/140001/Source/modules/quota/N...
File Source/modules/quota/NavigatorStorageQuota.idl (right):

https://codereview.chromium.org/1154573005/diff/140001/Source/modules/quota/N...
Source/modules/quota/NavigatorStorageQuota.idl:20: partial interface Navigator {
Added with links to the previous versions of the quota spec.

https://codereview.chromium.org/1154573005/diff/140001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/140001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:39: if (securityOrigin->isUnique()) {
On 2015/07/01 17:43:29, jsbell wrote:
> Add a TODO to track https://github.com/whatwg/storage/issues/5 (restricting to
> secure origins)

Done.

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:40:
resolver->reject(DOMError::create(NotSupportedError));
On 2015/07/01 17:43:29, jsbell wrote:
> Reject with DOMException, not DOMError.

Done.

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:50: if (!controller) // needed?
On 2015/07/01 17:43:29, jsbell wrote:
> Shouldn't be needed. Did you see this test elsewhere?

I thought so but don't see it now. In any case I'm going to try to obviate the
need for the controller.

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:57:
resolver->reject(DOMError::create(NotSupportedError));
On 2015/07/01 17:43:29, jsbell wrote:
> Reject with DOMException, not DOMError.

Done.

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.idl (right):

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
Source/modules/quota/StorageManager.idl:5: enum PersistentStoragePermission {
"default", "denied", "granted" };
On 2015/07/01 17:43:30, jsbell wrote:
> Please add spec link, e.g.
> 
> // https://storage.spec.whatwg.org/#storagemanager

Done.

https://codereview.chromium.org/1154573005/diff/160001/Source/modules/quota/S...
Source/modules/quota/StorageManager.idl:14: Promise<PersistentStoragePermission>
persistentPermission();
On 2015/07/01 17:43:30, jsbell wrote:
> Add TODOs for the parts of the spec that aren't included.
> 
> (i.e. document all deviations from the spec apart from
> bindings-generation-related extended attributes.)

Done.

[dgrogan, 2015-07-07 04:25:14]

https://codereview.chromium.org/1154573005/diff/220001/LayoutTests/storage/qu...
File LayoutTests/storage/quota/durability.html (right):

https://codereview.chromium.org/1154573005/diff/220001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability.html:10: // assert_class_string(promise,
"Promise");
jsbell@: I'd like to test 2 things here.

1) navigator.storage.requestPersistent returns a promise. I was hoping that
promise_test would handle that for me but it does not as returning a string from
this function doesn't cause failure. assert_class_string fails because it is an
Object, not a Promise. Recommendations? Should I check that |promise| has a
.then() method?

2) The promise resolves, not rejects.
https://github.com/w3c/testharness.js/blob/master/docs/api.md#promise-tests says
"The test completes when the returned promise resolves. The test fails if the
returned promise rejects," but I'm not seeing that. Forcing the promise to
reject in C++ still results in the test passing. Again, recommendations?

We can talk in person tomorrow. There's a decent chance I have a fundamental
misunderstanding somewhere.

[jsbell, 2015-07-07 16:51:05]

https://codereview.chromium.org/1154573005/diff/220001/LayoutTests/storage/qu...
File LayoutTests/storage/quota/durability.html (right):

https://codereview.chromium.org/1154573005/diff/220001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability.html:10: // assert_class_string(promise,
"Promise");
On 2015/07/07 04:25:14, dgrogan wrote:
> jsbell@: I'd like to test 2 things here.
> 
> 1) navigator.storage.requestPersistent returns a promise. I was hoping that
> promise_test would handle that for me but it does not as returning a string
from
> this function doesn't cause failure. assert_class_string fails because it is
an
> Object, not a Promise. Recommendations? Should I check that |promise| has a
> .then() method?

The return value is automagically wrapped in a promise, and the "class string"
is Object for native JS object types.

You should be able to do `foo instanceof Promise`

> 
> 2) The promise resolves, not rejects.
> https://github.com/w3c/testharness.js/blob/master/docs/api.md#promise-tests
says
> "The test completes when the returned promise resolves. The test fails if the
> returned promise rejects," but I'm not seeing that. Forcing the promise to
> reject in C++ still results in the test passing. Again, recommendations?

That's odd - what you're doing sounds right, so something weird is going on. We
can take a look together.

[dgrogan, 2015-07-09 02:56:05]

https://codereview.chromium.org/1154573005/diff/260001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/260001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:21: WebPermissionClient*
getPermissionClient(ExecutionContext* executionContext)
Mounir, you have a TODO about this block of code in PermissionStatus.cpp. I
pasted from there, which 1) is obviously lame and 2) requires hacking
Source/modules/quota/DEPS to allow the necessary PermissionController.h include
here.

So where should this block live so that it is available to StorageManager?

I gave it a go in
https://codereview.chromium.org/1228873002/diff/1/public/platform/modules/per...
and
https://codereview.chromium.org/1228873002/diff/1/Source/web/WebPermissionCli...
but couldn't get it to link in the component build.

Advice needed.

[mlamouri (slow - plz ping), 2015-07-16 10:12:18]

https://codereview.chromium.org/1154573005/diff/300001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/300001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:21: WebPermissionClient*
getPermissionClient(ExecutionContext* executionContext)
Maybe you can move that to static Permissions::GetClient(ExecutionContext*)?

Add a TODO on my name. I think ideally, the client should supplement the
ExecutionContext or something like that.

https://codereview.chromium.org/1154573005/diff/300001/public/platform/module...
File public/platform/modules/permissions/WebPermissionClient.h (right):

https://codereview.chromium.org/1154573005/diff/300001/public/platform/module...
public/platform/modules/permissions/WebPermissionClient.h:24: virtual void
requestDurablePermission(const WebURL&, WebPermissionQueryCallback*) { }
This is simply requestPermission) without the type specified, right? lalitm@ is
working on implementing that and has CLs waiting for reviews. You guys might
want to sync.

[dgrogan, 2015-07-16 19:15:53]

https://codereview.chromium.org/1154573005/diff/300001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/300001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:21: WebPermissionClient*
getPermissionClient(ExecutionContext* executionContext)
On 2015/07/16 10:12:17, Mounir Lamouri wrote:
> Maybe you can move that to static Permissions::GetClient(ExecutionContext*)?

Ok, I will give that a shot and break it off into a different CL if it works
out. Though the cross-module dependency issue will be part of the TODO.

> Add a TODO on my name. I think ideally, the client should supplement the
> ExecutionContext or something like that.

https://codereview.chromium.org/1154573005/diff/300001/public/platform/module...
File public/platform/modules/permissions/WebPermissionClient.h (right):

https://codereview.chromium.org/1154573005/diff/300001/public/platform/module...
public/platform/modules/permissions/WebPermissionClient.h:24: virtual void
requestDurablePermission(const WebURL&, WebPermissionQueryCallback*) { }
On 2015/07/16 10:12:18, Mounir Lamouri wrote:
> This is simply requestPermission) without the type specified, right? 

That's right.

> lalitm@ is
> working on implementing that and has CLs waiting for reviews. You guys might
> want to sync.

Good to hear. I figured this was a placeholder and would either be removed in
favor of or evolve into the generic version. I will ping him.

[dgrogan, 2015-08-01 00:02:00]

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

[dgrogan, 2015-08-01 00:02:01]

This is ready for a real review.

jsbell, can you look at at least StorageManager.idl, StorageManager.h,
NavigatorStorageQuota.*, and the layout test?

mlamouri, can you look at StorageManager.cpp and WebPermissionType.h?

kinuko, I'm bugging you as an OWNER of modules/quota. Look at anything you'd
like :)

The commit plan is to first commit this CL minus the layout test, then the
chrome CL, then the layout test.

[jsbell, 2015-08-03 19:57:41]

Feedback on my assignments.

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
File LayoutTests/storage/quota/durability-basics.html (right):

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability-basics.html:1: <!DOCTYPE html>
Since this is not under http/tests this is implicitly running as file:/// rather
than http/https.

I'd recommend moving it there (and/or copying it, if you want to maintain the
file:/// aspect, which might be worthwhile...)

We'd also want a test case where the requests are done in an non-secure origin
and hence fail.

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability-basics.html:5: <script
type="text/javascript">
No need for type here

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability-basics.html:12: return promise;
This looks like it's missing the test of the promise result itself?

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability-basics.html:18: return promise;
Ditto?

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:9: #include "core/dom/DOMError.h"
DOMError is not used?

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:19: class
DurableStoragePermissionCallbacks : public WebCallbacks<WebPermissionStatus*,
void> {
Put this class in anonymous namespace?

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:19: class
DurableStoragePermissionCallbacks : public WebCallbacks<WebPermissionStatus*,
void> {
final?

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:59: // process. Can we do it here too,
before jumping the process boundary?
Yes, use something like:

String errorMessage;
if (!executionContext->isPrivilegedContext(&errorMessage)) {
  resolver->reject(...);
  return promise;
}

Then on the Chromium side, terminate the renderer(via BadMessage) if a request
comes through from a non-secure origin (since it means this client-side test was
bypassed by a compromised renderer)

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.h (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.h:8: #include
"bindings/core/v8/ScriptPromise.h"
Can ScriptPromise and ScriptState be forward-declarations rather than #includes?

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.h:15: class StorageManager final : public
GarbageCollected<StorageManager>, public ScriptWrappable { // Cargo culted
Ditch the 'cargo culted' comments. This is correct.

[jsbell, 2015-08-03 23:11:16]

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
File LayoutTests/storage/quota/durability-basics.html (right):

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability-basics.html:1: <!DOCTYPE html>
On 2015/08/03 19:57:41, jsbell wrote:
> We'd also want a test case where the requests are done in an non-secure origin
> and hence fail.

... and this can be based on (and live with) the tests in
LayoutTests/http/tests/security/powerfulFeatureRestrictions

[mlamouri (slow - plz ping), 2015-08-04 13:03:51]

+lalitm@ FYI

[mlamouri (slow - plz ping), 2015-08-04 13:15:19]

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/DEPS
File Source/modules/quota/DEPS (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/D...
Source/modules/quota/DEPS:8: "+modules/permissions",
I don't think that's right. The end goal of https://crbug.com/510948 isn't to
remove modules/permissions.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/N...
File Source/modules/quota/NavigatorStorageQuota.idl (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/N...
Source/modules/quota/NavigatorStorageQuota.idl:29:
[RuntimeEnabled=DurableStorage] readonly attribute StorageManager storage;
Usually, we have a comment before the interface pointing to the spec related to
the interface. We don't usually have links to the specs related to the objects
the interface is using.

IOW, I think "// https://storage.spec.whatwg.org/#api" should be in
StorageManager.idl.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:64: if (executionContext->isDocument())
{
Why do you want to check if it's in a document? The call will work and will
return the current permission state if you are not in a frame.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.h (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.h:20: DEFINE_INLINE_TRACE() { } // Cargo
culted
Inlining isn't recommended:
https://www.chromium.org/developers/coding-style#TOC-Inline-functions

Could you DECLARE_TRACE() / DEFINE_TRACE() ?

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.idl (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.idl:7: enum PersistentStoragePermission {
"default", "denied", "granted" };
Why isn't that using PermissionState from the Permissions API?

[Lalit Maganti, 2015-08-04 13:27:40]

lalitm@google.com changed reviewers:
+ lalitm@google.com

[Lalit Maganti, 2015-08-04 13:27:41]

Not a reviewer but I've become quite familiar with the code over the last few so
left a few comments :)

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:41: m_resolver->resolve(toReturn);
Not really any work to be done but very similar code to the above exists in
PermissionStatus - maybe find a nice place to consolidate?

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:45: ASSERT_NOT_REACHED();
I think propogating the error to the resolver may be the right thing to do here.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:66: ASSERT(permissionClient);
Should this be DASSERT or ASSERT? I think you can simply return an exception
like is done in Permissions if this is null.

[jsbell, 2015-08-04 15:32:04]

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/N...
File Source/modules/quota/NavigatorStorageQuota.idl (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/N...
Source/modules/quota/NavigatorStorageQuota.idl:29:
[RuntimeEnabled=DurableStorage] readonly attribute StorageManager storage;
On 2015/08/04 13:15:19, Mounir Lamouri wrote:
> Usually, we have a comment before the interface pointing to the spec related
to
> the interface. We don't usually have links to the specs related to the objects
> the interface is using.
> 
> IOW, I think "// https://storage.spec.whatwg.org/#api%22 should be in
> StorageManager.idl.

I believe David is documenting the various "partial interface Navigator"
contributions made by 3 different specs (two historical, one modern). So these
are in the correct location.

This could be made more specific: 

https://storage.spec.whatwg.org/#navigatorstorage

The alternative would be to split this into two or three separate IDLs that each
have "partial interface Navigator", but I don't think that's worthwhile. The
spec does this mixin via "implements" rather than "partial interface" anyway so
it doesn't buy us spec alignment.

[mlamouri (slow - plz ping), 2015-08-04 15:36:47]

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/N...
File Source/modules/quota/NavigatorStorageQuota.idl (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/N...
Source/modules/quota/NavigatorStorageQuota.idl:29:
[RuntimeEnabled=DurableStorage] readonly attribute StorageManager storage;
On 2015/08/04 at 15:32:04, jsbell wrote:
> On 2015/08/04 13:15:19, Mounir Lamouri wrote:
> > Usually, we have a comment before the interface pointing to the spec related
to
> > the interface. We don't usually have links to the specs related to the
objects
> > the interface is using.
> > 
> > IOW, I think "// https://storage.spec.whatwg.org/#api%22 should be in
> > StorageManager.idl.
> 
> I believe David is documenting the various "partial interface Navigator"
contributions made by 3 different specs (two historical, one modern). So these
are in the correct location.
> 
> This could be made more specific: 
> 
> https://storage.spec.whatwg.org/#navigatorstorage
> 
> The alternative would be to split this into two or three separate IDLs that
each have "partial interface Navigator", but I don't think that's worthwhile.
The spec does this mixin via "implements" rather than "partial interface" anyway
so it doesn't buy us spec alignment.

Oh, I did not understand that. Maybe this could be explicitly mentioned at the
top of the file like:
// This partial interface is a merge of three different partial interface
related to storage, see:
//
http://www.w3.org/TR/2012/WD-quota-api-20120703/#idl-def-StorageQuotaEnvironment
//
http://www.w3.org/TR/2015/WD-quota-api-20150409/#accessing-storagequota-inter...
// https://storage.spec.whatwg.org/#api

[jsbell, 2015-08-04 16:30:32]

On 2015/08/04 15:36:47, Mounir Lamouri wrote:
> Oh, I did not understand that. Maybe this could be explicitly mentioned at the
> top of the file like:

SGTM!

[dgrogan, 2015-08-06 23:52:03]

Thanks for the comments all. Please take another look.

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
File LayoutTests/storage/quota/durability-basics.html (right):

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability-basics.html:1: <!DOCTYPE html>
On 2015/08/03 23:11:16, jsbell wrote:
> On 2015/08/03 19:57:41, jsbell wrote:
> > We'd also want a test case where the requests are done in an non-secure
origin
> > and hence fail.
> 
> ... and this can be based on (and live with) the tests in
> LayoutTests/http/tests/security/powerfulFeatureRestrictions

Done.

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability-basics.html:5: <script
type="text/javascript">
On 2015/08/03 19:57:41, jsbell wrote:
> No need for type here

Done.

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability-basics.html:12: return promise;
On 2015/08/03 19:57:41, jsbell wrote:
> This looks like it's missing the test of the promise result itself?

That was intentional,
content/shell/browser/layout_test/layout_test_permission_manager.cc just returns
DENIED unless something was set beforehand via testRunner.setPermission(). I
_could_ add such a call and test the promise result but it'd just be testing the
testRunner plumbing, nothing real. I suppose I could add a test that the result
is a non-zero-length string.

https://codereview.chromium.org/1154573005/diff/480001/LayoutTests/storage/qu...
LayoutTests/storage/quota/durability-basics.html:18: return promise;
On 2015/08/03 19:57:41, jsbell wrote:
> Ditto?

Acknowledged.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/DEPS
File Source/modules/quota/DEPS (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/D...
Source/modules/quota/DEPS:8: "+modules/permissions",
On 2015/08/04 13:15:19, Mounir Lamouri wrote:
> I don't think that's right. The end goal of https://crbug.com/510948 isn't to
> remove modules/permissions.

That issue is the only reason we need this extra dependency.

How's the updated phrasing?

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/N...
File Source/modules/quota/NavigatorStorageQuota.idl (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/N...
Source/modules/quota/NavigatorStorageQuota.idl:29:
[RuntimeEnabled=DurableStorage] readonly attribute StorageManager storage;
> Oh, I did not understand that. Maybe this could be explicitly mentioned at the
> top of the file like:
> // This partial interface is a merge of three different partial interface
> related to storage, see:

I want anyone doing archaeology to have an easier time knowing which object came
from which spec so prefer the inline approach. But I added your comment at the
top to explain why.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:9: #include "core/dom/DOMError.h"
On 2015/08/03 19:57:41, jsbell wrote:
> DOMError is not used?

Only as a vehicle for ExceptionCode.h. Fixed.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:19: class
DurableStoragePermissionCallbacks : public WebCallbacks<WebPermissionStatus*,
void> {
On 2015/08/03 19:57:41, jsbell wrote:
> final?

Done.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:19: class
DurableStoragePermissionCallbacks : public WebCallbacks<WebPermissionStatus*,
void> {
On 2015/08/03 19:57:41, jsbell wrote:
> Put this class in anonymous namespace?

Done.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:41: m_resolver->resolve(toReturn);
On 2015/08/04 13:27:41, Lalit Maganti wrote:
> Not really any work to be done but very similar code to the above exists in
> PermissionStatus - maybe find a nice place to consolidate?

It'd be awkward to consolidate while the strings are different. But if, as we
talk about elsewhere, this enum is merged with PermissionStatus then a
consolidation would be in order.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:45: ASSERT_NOT_REACHED();
On 2015/08/04 13:27:41, Lalit Maganti wrote:
> I think propogating the error to the resolver may be the right thing to do
here.

I was trying to document that this method can never be reached and that I wasn't
omitting it unintentionally. But you'd know better than me, can it ever be
reached? I don't see it being called in permission_dispatcher.cc.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:59: // process. Can we do it here too,
before jumping the process boundary?
On 2015/08/03 19:57:41, jsbell wrote:
> Yes, use something like:
> 
> String errorMessage;
> if (!executionContext->isPrivilegedContext(&errorMessage)) {
>   resolver->reject(...);
>   return promise;
> }

Done, thanks a lot for the pointer.

> Then on the Chromium side, terminate the renderer(via BadMessage) if a request
> comes through from a non-secure origin (since it means this client-side test
was
> bypassed by a compromised renderer)

Makes sense. I'll have to figure out how to do implement it though. The code I
wrote doesn't handle the messages directly but is 2ish steps removed.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:64: if (executionContext->isDocument())
{
On 2015/08/04 13:15:19, Mounir Lamouri wrote:
> Why do you want to check if it's in a document? The call will work and will
> return the current permission state if you are not in a frame.

I should probably rather DCHECK that it's a Document seeing as the IDL calls for
it to not even be exposed to anything but a Window.

I'd been thinking that I want the poor web dev who tries to call
requestPersistent from a worker that it won't actually prompt the user.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:66: ASSERT(permissionClient);
On 2015/08/04 13:27:41, Lalit Maganti wrote:
> Should this be DASSERT or ASSERT? I think you can simply return an exception
> like is done in Permissions if this is null.

I don't think DASSERT exists:
https://code.google.com/p/chromium/codesearch#search/&q=case:yes%20DASSERT&sq...

... but you're right, this case should be handled and not ASSERTed.
Done.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.h (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.h:8: #include
"bindings/core/v8/ScriptPromise.h"
On 2015/08/03 19:57:41, jsbell wrote:
> Can ScriptPromise and ScriptState be forward-declarations rather than
#includes?

Done.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.h:15: class StorageManager final : public
GarbageCollected<StorageManager>, public ScriptWrappable { // Cargo culted
On 2015/08/03 19:57:41, jsbell wrote:
> Ditch the 'cargo culted' comments. This is correct.

Thanks!

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.h:20: DEFINE_INLINE_TRACE() { } // Cargo
culted
On 2015/08/04 13:15:19, Mounir Lamouri wrote:
> Inlining isn't recommended:
> https://www.chromium.org/developers/coding-style#TOC-Inline-functions
> 
> Could you DECLARE_TRACE() / DEFINE_TRACE() ?

Done. Though note that the blink style guide is mostly silent on inline
functions. And it doesn't generally refer back to the chromium style guide, only
in specific situations.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.idl (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.idl:7: enum PersistentStoragePermission {
"default", "denied", "granted" };
On 2015/08/04 13:15:19, Mounir Lamouri wrote:
> Why isn't that using PermissionState from the Permissions API?

I was hoping you and Anne figured that out during your F2F a while ago. I am
going by what is defined here
https://storage.spec.whatwg.org/#enumdef-persistentstoragepermission

[Lalit Maganti, 2015-08-07 08:47:28]

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:45: ASSERT_NOT_REACHED();
On 2015/08/06 at 23:52:02, dgrogan wrote:
> On 2015/08/04 13:27:41, Lalit Maganti wrote:
> > I think propogating the error to the resolver may be the right thing to do
here.
> 
> I was trying to document that this method can never be reached and that I
wasn't omitting it unintentionally. But you'd know better than me, can it ever
be reached? I don't see it being called in permission_dispatcher.cc.

You're right - currently it cannot be reached. However, I would think that one
day we would make use of this in some way and in that case, it makes sense to be
implemented in a sensible way especially because it's very easy to simply reject
to the promise with no extra effort required.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:66: ASSERT(permissionClient);
On 2015/08/06 at 23:52:02, dgrogan wrote:
> On 2015/08/04 13:27:41, Lalit Maganti wrote:
> > Should this be DASSERT or ASSERT? I think you can simply return an exception
> > like is done in Permissions if this is null.
> 
> I don't think DASSERT exists:
>
https://code.google.com/p/chromium/codesearch#search/&q=case:yes%20DASSERT&sq...
> 
> ... but you're right, this case should be handled and not ASSERTed.
> Done.

Apologies - I meant DCHECK

[mlamouri (slow - plz ping), 2015-08-07 10:19:14]

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/DEPS
File Source/modules/quota/DEPS (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/D...
Source/modules/quota/DEPS:8: "+modules/permissions",
On 2015/08/06 at 23:52:02, dgrogan wrote:
> On 2015/08/04 13:15:19, Mounir Lamouri wrote:
> > I don't think that's right. The end goal of https://crbug.com/510948 isn't
to
> > remove modules/permissions.
> 
> That issue is the only reason we need this extra dependency.
> 
> How's the updated phrasing?

You need the dependency to access the PermissionClient. You might very likely
need to use the permissions module for that anyway.

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.idl (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.idl:7: enum PersistentStoragePermission {
"default", "denied", "granted" };
On 2015/08/06 at 23:52:02, dgrogan wrote:
> On 2015/08/04 13:15:19, Mounir Lamouri wrote:
> > Why isn't that using PermissionState from the Permissions API?
> 
> I was hoping you and Anne figured that out during your F2F a while ago. I am
going by what is defined here
https://storage.spec.whatwg.org/#enumdef-persistentstoragepermission

He agreed to 'prompt' in the Permissions API. I'm surprised this API is still
using 'default'. I'm going to email him and CC you and jsbell@.

[dgrogan, 2015-08-07 17:24:54]

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/DEPS
File Source/modules/quota/DEPS (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/D...
Source/modules/quota/DEPS:8: "+modules/permissions",
On 2015/08/07 10:19:14, Mounir Lamouri wrote:
> On 2015/08/06 at 23:52:02, dgrogan wrote:
> > On 2015/08/04 13:15:19, Mounir Lamouri wrote:
> > > I don't think that's right. The end goal of https://crbug.com/510948 isn't
> to
> > > remove modules/permissions.
> > 
> > That issue is the only reason we need this extra dependency.
> > 
> > How's the updated phrasing?
> 
> You need the dependency to access the PermissionClient.

I don't think that's true. StorageManager.cpp's include of
public/platform/modules/permissions/WebPermissionClient.h should suffice.

> You might very likely
> need to use the permissions module for that anyway.

[dgrogan, 2015-08-07 17:37:56]

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

[dgrogan, 2015-08-07 17:37:56]

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/DEPS
File Source/modules/quota/DEPS (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/D...
Source/modules/quota/DEPS:8: "+modules/permissions",
On 2015/08/07 17:24:54, dgrogan wrote:
> On 2015/08/07 10:19:14, Mounir Lamouri wrote:
> > On 2015/08/06 at 23:52:02, dgrogan wrote:
> > > On 2015/08/04 13:15:19, Mounir Lamouri wrote:
> > > > I don't think that's right. The end goal of https://crbug.com/510948
isn't
> > to
> > > > remove modules/permissions.
> > > 
> > > That issue is the only reason we need this extra dependency.
> > > 
> > > How's the updated phrasing?
> > 
> > You need the dependency to access the PermissionClient.
> 
> I don't think that's true. StorageManager.cpp's include of
> public/platform/modules/permissions/WebPermissionClient.h should suffice.

To add more detail, I imagine that 510948 is fixed by adding a method somewhere
that takes an ExecutionContext and returns a WebPermissionClient. Right now that
method lives in modules/permissions, hence the need for the extra DEPS line. We
could move that method to WebPermissionClient. Or maybe we add a new method to
ExecutionContext itself: `WebPermissionClient*
ExecutionContext::getPermissionClient()`. Such a method would also obviate the
need for this extra DEPS line.

I don't know, maybe this extra DEPS line isn't such a big deal. Can one module
depend on another? +haraken for his thoughts on that.

[haraken, 2015-08-10 00:35:11]

On 2015/08/07 17:37:56, dgrogan wrote:
>
https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/DEPS
> File Source/modules/quota/DEPS (right):
> 
>
https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/D...
> Source/modules/quota/DEPS:8: "+modules/permissions",
> On 2015/08/07 17:24:54, dgrogan wrote:
> > On 2015/08/07 10:19:14, Mounir Lamouri wrote:
> > > On 2015/08/06 at 23:52:02, dgrogan wrote:
> > > > On 2015/08/04 13:15:19, Mounir Lamouri wrote:
> > > > > I don't think that's right. The end goal of https://crbug.com/510948
> isn't
> > > to
> > > > > remove modules/permissions.
> > > > 
> > > > That issue is the only reason we need this extra dependency.
> > > > 
> > > > How's the updated phrasing?
> > > 
> > > You need the dependency to access the PermissionClient.
> > 
> > I don't think that's true. StorageManager.cpp's include of
> > public/platform/modules/permissions/WebPermissionClient.h should suffice.
> 
> To add more detail, I imagine that 510948 is fixed by adding a method
somewhere
> that takes an ExecutionContext and returns a WebPermissionClient. Right now
that
> method lives in modules/permissions, hence the need for the extra DEPS line.
We
> could move that method to WebPermissionClient. Or maybe we add a new method to
> ExecutionContext itself: `WebPermissionClient*
> ExecutionContext::getPermissionClient()`. Such a method would also obviate the
> need for this extra DEPS line.
> 
> I don't know, maybe this extra DEPS line isn't such a big deal. Can one module
> depend on another? +haraken for his thoughts on that.

module => module dependency is allowed (although it would be nicer if we can
limit the number of lines in DEPS :-)

[jsbell, 2015-08-10 17:49:41]

https://codereview.chromium.org/1154573005/diff/540001/LayoutTests/http/tests...
File LayoutTests/http/tests/storage/durability-basics.html (right):

https://codereview.chromium.org/1154573005/diff/540001/LayoutTests/http/tests...
LayoutTests/http/tests/storage/durability-basics.html:7: test(function() {
assert_true(!!navigator.storage); }, "These tests requires navigator.storage");
Nit (if you're touching this file) - can you format so the test fits in 80 cols?
(not a hard rule, but we're generally trying to follow that in
testharness.js-based tests)

https://codereview.chromium.org/1154573005/diff/540001/LayoutTests/http/tests...
LayoutTests/http/tests/storage/durability-basics.html:13: assert_equals(typeof
result, "string", result + " should be a string");
As we've discussed offline, layout tests get canned results, not the strings per
spec. Can you add a comment here explaining that?

[jsbell, 2015-08-10 18:00:37]

a few more nits, otherwise: lgtm

https://codereview.chromium.org/1154573005/diff/540001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/540001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:62: if (securityOrigin->isUnique()) {
I think the isPrivilegedcontext() check would cover the isUnique() case but I'm
not 100% sure. Maybe toss in a TODO to verify?

(From header docs in SecurityOrigin, unique implies sandboxed, and sandboxed
implies not privileged.)

https://codereview.chromium.org/1154573005/diff/540001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:74:
resolver->reject(DOMException::create(InvalidStateError, "In its current state,
the global scope can't query permissions."));
nit: s/query/request/ (to match Permissions.cpp)

[dgrogan, 2015-08-10 18:39:56]

https://codereview.chromium.org/1154573005/diff/540001/LayoutTests/http/tests...
File LayoutTests/http/tests/storage/durability-basics.html (right):

https://codereview.chromium.org/1154573005/diff/540001/LayoutTests/http/tests...
LayoutTests/http/tests/storage/durability-basics.html:7: test(function() {
assert_true(!!navigator.storage); }, "These tests requires navigator.storage");
On 2015/08/10 17:49:41, jsbell wrote:
> Nit (if you're touching this file) - can you format so the test fits in 80
cols?
> (not a hard rule, but we're generally trying to follow that in
> testharness.js-based tests)

Done.

https://codereview.chromium.org/1154573005/diff/540001/LayoutTests/http/tests...
LayoutTests/http/tests/storage/durability-basics.html:13: assert_equals(typeof
result, "string", result + " should be a string");
On 2015/08/10 17:49:41, jsbell wrote:
> As we've discussed offline, layout tests get canned results, not the strings
per
> spec. Can you add a comment here explaining that?

Done.

https://codereview.chromium.org/1154573005/diff/540001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/540001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:62: if (securityOrigin->isUnique()) {
On 2015/08/10 18:00:37, jsbell wrote:
> I think the isPrivilegedcontext() check would cover the isUnique() case but
I'm
> not 100% sure. Maybe toss in a TODO to verify?
> 
> (From header docs in SecurityOrigin, unique implies sandboxed, and sandboxed
> implies not privileged.)

Done.

https://codereview.chromium.org/1154573005/diff/540001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:74:
resolver->reject(DOMException::create(InvalidStateError, "In its current state,
the global scope can't query permissions."));
On 2015/08/10 18:00:37, jsbell wrote:
> nit: s/query/request/ (to match Permissions.cpp)

Done.

[dgrogan, 2015-08-10 22:26:41]

Mounir, could you take another look?

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/480001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:45: ASSERT_NOT_REACHED();
On 2015/08/07 08:47:28, Lalit Maganti wrote:
> You're right - currently it cannot be reached. However, I would think that one
> day we would make use of this in some way and in that case, it makes sense to
be
> implemented in a sensible way especially because it's very easy to simply
reject
> to the promise with no extra effort required.

I thought about this and still prefer to leave it as is for a few reasons. The
first is that there'd be no way to test the code that I add; there's no way to
invoke the error path because it doesn't exist. Adding untestable code seems
suboptimal. The second is what would I make this signature? Currently I specify
void in the template above. What would I change that to? I'd have to guess as to
what type of the potential future error condition is and if I guess wrong then I
haven't saved the future maintainer any work.

This is good to think through though, so let me know if I've missed something or
you remain unconvinced.

https://codereview.chromium.org/1154573005/diff/560001/Source/modules/quota/DEPS
File Source/modules/quota/DEPS (right):

https://codereview.chromium.org/1154573005/diff/560001/Source/modules/quota/D...
Source/modules/quota/DEPS:7: # Can modules/permissions be removed when
https://crbug.com/510948 is fixed?
Mounir, I changed this comment in light of Kentaro's message. Let me know what
you think.

[kinuko, 2015-08-11 08:53:54]

lgtm for the stuff I care about.

https://codereview.chromium.org/1154573005/diff/580001/Source/modules/quota/N...
File Source/modules/quota/NavigatorStorageQuota.idl (right):

https://codereview.chromium.org/1154573005/diff/580001/Source/modules/quota/N...
Source/modules/quota/NavigatorStorageQuota.idl:21: // related to storage.
Meh, thanks for adding a nice comment for explaining this messy situation.

https://codereview.chromium.org/1154573005/diff/580001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/580001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:30: OwnPtr<WebPermissionStatus> status =
adoptPtr(rawStatus);
nit: you should be able to pass these permission status enums by value now (see
https://codereview.chromium.org/1240763002/)

[mlamouri (slow - plz ping), 2015-08-11 11:56:01]

lgtm

FTR, using "default" instead of "prompt" is bad for the platform consistency.
Though, it seems that the new design of the API is moving toward a boolean which
is slightly better.

[dgrogan, 2015-08-11 19:07:50]

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

[dgrogan, 2015-08-11 19:07:51]

Jochen, can you review the RuntimeEnabledFeatures.in change? Durable storage is
being added behind the experimental-web-platform-features flag.

https://codereview.chromium.org/1154573005/diff/580001/Source/modules/quota/S...
File Source/modules/quota/StorageManager.cpp (right):

https://codereview.chromium.org/1154573005/diff/580001/Source/modules/quota/S...
Source/modules/quota/StorageManager.cpp:30: OwnPtr<WebPermissionStatus> status =
adoptPtr(rawStatus);
On 2015/08/11 08:53:54, kinuko wrote:
> nit: you should be able to pass these permission status enums by value now
(see
> https://codereview.chromium.org/1240763002/)

That would be nicer, yes. I changed it locally before realizing that
WebPermissionClient still expects WebPermissionStatus*.

[dgrogan, 2015-08-11 19:49:44]

dgrogan@chromium.org changed reviewers:
+ pdr@chromium.org
- jochen@chromium.org

[dgrogan, 2015-08-11 19:49:46]

-jochen (traveling - slow), +pdr

pdr, can you review RuntimeEnabledFeatures.in? Durable storage is being added
behind the experimental-web-platform-features flag.

Jochen has been consulting/reviewing a bit for this feature which is why I
picked him first but if you're comfortable with the RuntimeEnabledFeatures.in
change, it'd be nice to get this patch in.

[pdr., 2015-08-11 19:50:55]

On 2015/08/11 at 19:49:46, dgrogan wrote:
> -jochen (traveling - slow), +pdr
> 
> pdr, can you review RuntimeEnabledFeatures.in? Durable storage is being added
behind the experimental-web-platform-features flag.
> 
> Jochen has been consulting/reviewing a bit for this feature which is why I
picked him first but if you're comfortable with the RuntimeEnabledFeatures.in
change, it'd be nice to get this patch in.

RuntimeEnabledFeatures.in LGTM

[dgrogan, 2015-08-11 19:55:37]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2015-08-11 19:55:37]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org,
mlamouri@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1154573005/#ps620001
(title: "remove one layout test for landing 3rd")

[commit-bot: I haz the power, 2015-08-11 19:55:57]

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

[commit-bot: I haz the power, 2015-08-11 21:36:14]

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

[commit-bot: I haz the power, 2015-08-11 21:36:15]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/73347)

[dgrogan, 2015-08-11 22:48:26]

jsbell@, can you review the changes to *webexposed*-expected.txt? It seems wrong
that the interface is exposed to workers when there's no way to access it.

[jsbell, 2015-08-11 23:03:14]

On 2015/08/11 22:48:26, dgrogan wrote:
> jsbell@, can you review the changes to *webexposed*-expected.txt? It seems
wrong
> that the interface is exposed to workers when there's no way to access it.

This all looks correct to me. We will expose StorageManager via NavigatorWorker
at some point (once we have tests). The interface is useless but harmless.

so still lgtm

[jsbell, 2015-08-11 23:03:22]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2015-08-11 23:03:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, kinuko@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1154573005/#ps640001
(title: "just reset the results")

[commit-bot: I haz the power, 2015-08-11 23:03:31]

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

[commit-bot: I haz the power, 2015-08-12 00:35:14]

Committed patchset #33 (id:640001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200362