[Quota] Pass user gesture state with quota request message.

[Quota] Pass user gesture state with quota request message.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261588

[Greg Billock, 2014-03-03 19:02:50]

nasko for quota review; darin for content API owners

[Greg Billock, 2014-03-03 19:03:47]

On 2014/03/03 19:02:50, Greg Billock wrote:
> nasko for quota review; darin for content API owners

oops. should be "quota *messages* review"

[nasko, 2014-03-03 20:00:32]

Just a couple of small issues.

https://codereview.chromium.org/185793004/diff/40001/content/browser/quota_di...
File content/browser/quota_dispatcher_host.cc (right):

https://codereview.chromium.org/185793004/diff/40001/content/browser/quota_di...
content/browser/quota_dispatcher_host.cc:105: int request_id)
nit: request_id is already part of params, right? Why not remove the parameter?

https://codereview.chromium.org/185793004/diff/40001/webkit/common/quota/quot...
File webkit/common/quota/quota_types.cc (right):

https://codereview.chromium.org/185793004/diff/40001/webkit/common/quota/quot...
webkit/common/quota/quota_types.cc:10: : render_view_id(-1),
Use MSG_ROUTING_NONE, which is the proper value to use for uninitialized routing
id.

[Greg Billock, 2014-03-03 22:41:46]

(continuing GAE problems. I may end up killing more intermediate uploads...)

https://codereview.chromium.org/185793004/diff/40001/content/browser/quota_di...
File content/browser/quota_dispatcher_host.cc (right):

https://codereview.chromium.org/185793004/diff/40001/content/browser/quota_di...
content/browser/quota_dispatcher_host.cc:105: int request_id)
On 2014/03/03 20:00:32, nasko wrote:
> nit: request_id is already part of params, right? Why not remove the
parameter?

I thought I remembered the compiler being fussy about using params.request_id as
the parameter to the superclass, but I think that must have been a false alarm.
Switched to use it and everyone seems happy...

https://codereview.chromium.org/185793004/diff/40001/webkit/common/quota/quot...
File webkit/common/quota/quota_types.cc (right):

https://codereview.chromium.org/185793004/diff/40001/webkit/common/quota/quot...
webkit/common/quota/quota_types.cc:10: : render_view_id(-1),
On 2014/03/03 20:00:32, nasko wrote:
> Use MSG_ROUTING_NONE, which is the proper value to use for uninitialized
routing
> id.

Done.

[nasko, 2014-03-04 00:06:58]

LGTM on messages

[kinuko, 2014-03-04 01:49:08]

lgtm, but please fix build errors

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
File webkit/common/quota/quota_types.h (right):

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
webkit/common/quota/quota_types.h:32: int render_view_id;
Strictly speaking referring render_view_id in webkit/ feels a bit wrong, but
given that webkit/ is being merged into content/ this should be fine...

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
webkit/common/quota/quota_types.h:35: quota::StorageType storage_type;
nit: no need of quota:: here

[kinuko, 2014-03-04 01:55:40]

A few more nits

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
File webkit/common/quota/quota_types.h (right):

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
webkit/common/quota/quota_types.h:28: struct WEBKIT_STORAGE_COMMON_EXPORT
RequestStorageQuotaParams {
It feels having this in content/common/quota is more appropriate (putting aside
that this file itself is going to be merged into there), since this is only for
messaging/UI

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
webkit/common/quota/quota_types.h:37: bool user_gesture;
It's not obvious what does this flag means here-- can we have some comment?
Is boolean enough / best for this flag, or enum might be better?

[Greg Billock, 2014-03-04 18:49:36]

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
File webkit/common/quota/quota_types.h (right):

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
webkit/common/quota/quota_types.h:28: struct WEBKIT_STORAGE_COMMON_EXPORT
RequestStorageQuotaParams {
On 2014/03/04 01:55:40, kinuko wrote:
> It feels having this in content/common/quota is more appropriate (putting
aside
> that this file itself is going to be merged into there), since this is only
for
> messaging/UI

So in the messages file itself? I can do that.

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
webkit/common/quota/quota_types.h:32: int render_view_id;
On 2014/03/04 01:49:09, kinuko wrote:
> Strictly speaking referring render_view_id in webkit/ feels a bit wrong, but
> given that webkit/ is being merged into content/ this should be fine...

ok.

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
webkit/common/quota/quota_types.h:35: quota::StorageType storage_type;
On 2014/03/04 01:49:09, kinuko wrote:
> nit: no need of quota:: here

Done.

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
webkit/common/quota/quota_types.h:37: bool user_gesture;
On 2014/03/04 01:55:40, kinuko wrote:
> It's not obvious what does this flag means here-- can we have some comment?
> Is boolean enough / best for this flag, or enum might be better?

Basically that the message originated in the context of a user gesture. A
boolean is enough.

[kinuko, 2014-03-05 01:07:35]

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
File webkit/common/quota/quota_types.h (right):

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
webkit/common/quota/quota_types.h:28: struct WEBKIT_STORAGE_COMMON_EXPORT
RequestStorageQuotaParams {
On 2014/03/04 18:49:37, Greg Billock wrote:
> On 2014/03/04 01:55:40, kinuko wrote:
> > It feels having this in content/common/quota is more appropriate (putting
> aside
> > that this file itself is going to be merged into there), since this is only
> for
> > messaging/UI
> 
> So in the messages file itself? I can do that.

I meant another file in content/common, but the current location is ok with me
too-- can you add a brief comment to this struct? Then this lg to me

[Greg Billock, 2014-03-05 18:20:31]

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
File webkit/common/quota/quota_types.h (right):

https://codereview.chromium.org/185793004/diff/100001/webkit/common/quota/quo...
webkit/common/quota/quota_types.h:28: struct WEBKIT_STORAGE_COMMON_EXPORT
RequestStorageQuotaParams {
On 2014/03/05 01:07:35, kinuko wrote:
> On 2014/03/04 18:49:37, Greg Billock wrote:
> > On 2014/03/04 01:55:40, kinuko wrote:
> > > It feels having this in content/common/quota is more appropriate (putting
> > aside
> > > that this file itself is going to be merged into there), since this is
only
> > for
> > > messaging/UI
> > 
> > So in the messages file itself? I can do that.
> 
> I meant another file in content/common, but the current location is ok with me
> too-- can you add a brief comment to this struct? Then this lg to me

Done. I'll leave it here, then, esp. if these are moving to content/common
anyway -- then it'll move along with the enums.

[Greg Billock, 2014-03-05 18:20:39]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-05 18:22:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/185793004/340001

[commit-bot: I haz the power, 2014-03-05 18:53:58]

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

[commit-bot: I haz the power, 2014-03-05 18:53:58]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Greg Billock, 2014-03-05 19:51:06]

On 2014/03/05 18:53:58, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

+boliu for android_webview
+sky for content/ (mostly)
+darin for content/child

[boliu, 2014-03-05 19:53:38]

android_webview lgtm

[sky, 2014-03-05 21:31:07]

content/browser LGTM

[Greg Billock, 2014-03-05 22:57:35]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-05 22:59:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/185793004/340001

[commit-bot: I haz the power, 2014-03-05 23:20:08]

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

[commit-bot: I haz the power, 2014-03-05 23:20:10]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Greg Billock, 2014-03-05 23:27:59]

+pfeldman for content/shell
+avi for content/child

[Greg Billock, 2014-03-14 08:35:35]

On 2014/03/05 23:27:59, Greg Billock wrote:
> +pfeldman for content/shell
> +avi for content/child

ping

[pfeldman, 2014-03-14 16:56:15]

shell rslgtm

[Greg Billock, 2014-03-24 18:25:48]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-03-24 18:26:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/185793004/380001

[commit-bot: I haz the power, 2014-03-24 19:12:59]

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

[commit-bot: I haz the power, 2014-03-24 19:13:00]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Greg Billock, 2014-03-24 21:44:01]

On 2014/03/24 19:13:00, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

+jam for content/* owners (contains API adjustments)

[jam, 2014-03-24 21:48:58]

given that people are working on removing src/webkit, new structs should go into
src/content/public/common instead

[Greg Billock, 2014-03-24 21:53:05]

On 2014/03/24 21:48:58, jam wrote:
> given that people are working on removing src/webkit, new structs should go
into
> src/content/public/common instead

Should I move the whole file?

[Greg Billock, 2014-03-24 22:12:14]

On 2014/03/24 21:53:05, Greg Billock wrote:
> On 2014/03/24 21:48:58, jam wrote:
> > given that people are working on removing src/webkit, new structs should go
> into
> > src/content/public/common instead
> 
> Should I move the whole file?

Looking more into this, there's a bit of a tangle -- it looks like there's some
storage code that needs to move out first. I think we should just add this here
for now and move it along with the whole file in due course.

[jam, 2014-03-24 23:22:24]

On 2014/03/24 22:12:14, Greg Billock wrote:
> On 2014/03/24 21:53:05, Greg Billock wrote:
> > On 2014/03/24 21:48:58, jam wrote:
> > > given that people are working on removing src/webkit, new structs should
go
> > into
> > > src/content/public/common instead
> > 
> > Should I move the whole file?
> 
> Looking more into this, there's a bit of a tangle -- it looks like there's
some
> storage code that needs to move out first. I think we should just add this
here
> for now and move it along with the whole file in due course.

just create the new struct in content, it's not used in quota. you can leave the
existing enums there.

[Greg Billock, 2014-03-25 17:28:21]

On 2014/03/24 23:22:24, jam wrote:
> On 2014/03/24 22:12:14, Greg Billock wrote:
> > On 2014/03/24 21:53:05, Greg Billock wrote:
> > > On 2014/03/24 21:48:58, jam wrote:
> > > > given that people are working on removing src/webkit, new structs should
> go
> > > into
> > > > src/content/public/common instead
> > > 
> > > Should I move the whole file?
> > 
> > Looking more into this, there's a bit of a tangle -- it looks like there's
> some
> > storage code that needs to move out first. I think we should just add this
> here
> > for now and move it along with the whole file in due course.
> 
> just create the new struct in content, it's not used in quota. you can leave
the
> existing enums there.

ok, done

[Greg Billock, 2014-03-27 22:31:03]

On 2014/03/25 17:28:21, Greg Billock wrote:
> On 2014/03/24 23:22:24, jam wrote:
> > On 2014/03/24 22:12:14, Greg Billock wrote:
> > > On 2014/03/24 21:53:05, Greg Billock wrote:
> > > > On 2014/03/24 21:48:58, jam wrote:
> > > > > given that people are working on removing src/webkit, new structs
should
> > go
> > > > into
> > > > > src/content/public/common instead
> > > > 
> > > > Should I move the whole file?
> > > 
> > > Looking more into this, there's a bit of a tangle -- it looks like there's
> > some
> > > storage code that needs to move out first. I think we should just add this
> > here
> > > for now and move it along with the whole file in due course.
> > 
> > just create the new struct in content, it's not used in quota. you can leave
> the
> > existing enums there.
> 
> ok, done

ping

[jam, 2014-03-27 22:51:48]

lgtm with comments

https://codereview.chromium.org/185793004/diff/420001/content/child/quota_dis...
File content/child/quota_dispatcher.cc (right):

https://codereview.chromium.org/185793004/diff/420001/content/child/quota_dis...
content/child/quota_dispatcher.cc:131: content::RequestStorageQuotaParams
params;
ditto

https://codereview.chromium.org/185793004/diff/420001/content/public/browser/...
File content/public/browser/quota_permission_context.h (right):

https://codereview.chromium.org/185793004/diff/420001/content/public/browser/...
content/public/browser/quota_permission_context.h:30: const
content::RequestStorageQuotaParams& params,
nit: no "content::" in content namespace

https://codereview.chromium.org/185793004/diff/420001/content/public/common/q...
File content/public/common/quota_types.cc (right):

https://codereview.chromium.org/185793004/diff/420001/content/public/common/q...
content/public/common/quota_types.cc:11:
RequestStorageQuotaParams::RequestStorageQuotaParams()
can you try inlining this and removing the cc file? only add a cc file in
content/public if clang complains. see
http://www.chromium.org/developers/content-module/content-api

https://codereview.chromium.org/185793004/diff/420001/content/public/common/q...
content/public/common/quota_types.cc:18:
RequestStorageQuotaParams::~RequestStorageQuotaParams() {}
also drop this, it's not needed

[Greg Billock, 2014-03-28 17:38:02]

https://codereview.chromium.org/185793004/diff/420001/content/child/quota_dis...
File content/child/quota_dispatcher.cc (right):

https://codereview.chromium.org/185793004/diff/420001/content/child/quota_dis...
content/child/quota_dispatcher.cc:131: content::RequestStorageQuotaParams
params;
On 2014/03/27 22:51:49, jam wrote:
> ditto

Done.

https://codereview.chromium.org/185793004/diff/420001/content/public/browser/...
File content/public/browser/quota_permission_context.h (right):

https://codereview.chromium.org/185793004/diff/420001/content/public/browser/...
content/public/browser/quota_permission_context.h:30: const
content::RequestStorageQuotaParams& params,
On 2014/03/27 22:51:49, jam wrote:
> nit: no "content::" in content namespace

Done.

https://codereview.chromium.org/185793004/diff/420001/content/public/common/q...
File content/public/common/quota_types.cc (right):

https://codereview.chromium.org/185793004/diff/420001/content/public/common/q...
content/public/common/quota_types.cc:11:
RequestStorageQuotaParams::RequestStorageQuotaParams()
On 2014/03/27 22:51:49, jam wrote:
> can you try inlining this and removing the cc file? only add a cc file in
> content/public if clang complains. see
> http://www.chromium.org/developers/content-module/content-api

Done.

https://codereview.chromium.org/185793004/diff/420001/content/public/common/q...
content/public/common/quota_types.cc:18:
RequestStorageQuotaParams::~RequestStorageQuotaParams() {}
On 2014/03/27 22:51:49, jam wrote:
> also drop this, it's not needed

Done.

[jam, 2014-03-28 18:03:09]

sorry didn't see this earlier

https://codereview.chromium.org/185793004/diff/460001/content/public/common/q...
File content/public/common/quota_types.h (right):

https://codereview.chromium.org/185793004/diff/460001/content/public/common/q...
content/public/common/quota_types.h:1: // Copyright 2013 The Chromium Authors.
All rights reserved.
nit: 2014

also note (per content api wiki) that each struct/class/enum is in its own file.
so this should be request_storage_quota_params.h if the struct keeps the same
name. or even better, it should be storage_quota_params.h and the struct should
be called StorageQuotaParams

[Greg Billock, 2014-03-28 18:17:45]

https://codereview.chromium.org/185793004/diff/460001/content/public/common/q...
File content/public/common/quota_types.h (right):

https://codereview.chromium.org/185793004/diff/460001/content/public/common/q...
content/public/common/quota_types.h:1: // Copyright 2013 The Chromium Authors.
All rights reserved.
On 2014/03/28 18:03:10, jam wrote:
> nit: 2014

Will do.

> also note (per content api wiki) that each struct/class/enum is in its own
file.
> so this should be request_storage_quota_params.h if the struct keeps the same
> name. or even better, it should be storage_quota_params.h and the struct
should
> be called StorageQuotaParams

Since the files from webkit/common/quota/... will eventually get merged here,
should we choose a slightly more generic name? How about storage_quota_types.h?

[jam, 2014-03-31 17:27:26]

https://codereview.chromium.org/185793004/diff/460001/content/public/common/q...
File content/public/common/quota_types.h (right):

https://codereview.chromium.org/185793004/diff/460001/content/public/common/q...
content/public/common/quota_types.h:1: // Copyright 2013 The Chromium Authors.
All rights reserved.
On 2014/03/28 18:17:45, Greg Billock wrote:
> On 2014/03/28 18:03:10, jam wrote:
> > nit: 2014
> 
> Will do.
> 
> > also note (per content api wiki) that each struct/class/enum is in its own
> file.
> > so this should be request_storage_quota_params.h if the struct keeps the
same
> > name. or even better, it should be storage_quota_params.h and the struct
> should
> > be called StorageQuotaParams
> 
> Since the files from webkit/common/quota/... will eventually get merged here,
> should we choose a slightly more generic name? How about
storage_quota_types.h?

per http://www.chromium.org/developers/content-module/content-api
"each interface/struct/enum should be in a separate file"

[Greg Billock, 2014-03-31 17:32:25]

On 2014/03/31 17:27:26, jam wrote:
>
https://codereview.chromium.org/185793004/diff/460001/content/public/common/q...
> File content/public/common/quota_types.h (right):
> 
>
https://codereview.chromium.org/185793004/diff/460001/content/public/common/q...
> content/public/common/quota_types.h:1: // Copyright 2013 The Chromium Authors.
> All rights reserved.
> On 2014/03/28 18:17:45, Greg Billock wrote:
> > On 2014/03/28 18:03:10, jam wrote:
> > > nit: 2014
> > 
> > Will do.
> > 
> > > also note (per content api wiki) that each struct/class/enum is in its own
> > file.
> > > so this should be request_storage_quota_params.h if the struct keeps the
> same
> > > name. or even better, it should be storage_quota_params.h and the struct
> > should
> > > be called StorageQuotaParams
> > 
> > Since the files from webkit/common/quota/... will eventually get merged
here,
> > should we choose a slightly more generic name? How about
> storage_quota_types.h?
> 
> per http://www.chromium.org/developers/content-module/content-api
> "each interface/struct/enum should be in a separate file"

OK. Somehow the "each" in your original mail on this didn't register. Sounds
good.

[Greg Billock, 2014-04-03 00:01:00]

On 2014/03/31 17:32:25, Greg Billock wrote:
> On 2014/03/31 17:27:26, jam wrote:
> >
>
https://codereview.chromium.org/185793004/diff/460001/content/public/common/q...
> > File content/public/common/quota_types.h (right):
> > 
> >
>
https://codereview.chromium.org/185793004/diff/460001/content/public/common/q...
> > content/public/common/quota_types.h:1: // Copyright 2013 The Chromium
Authors.
> > All rights reserved.
> > On 2014/03/28 18:17:45, Greg Billock wrote:
> > > On 2014/03/28 18:03:10, jam wrote:
> > > > nit: 2014
> > > 
> > > Will do.
> > > 
> > > > also note (per content api wiki) that each struct/class/enum is in its
own
> > > file.
> > > > so this should be request_storage_quota_params.h if the struct keeps the
> > same
> > > > name. or even better, it should be storage_quota_params.h and the struct
> > > should
> > > > be called StorageQuotaParams
> > > 
> > > Since the files from webkit/common/quota/... will eventually get merged
> here,
> > > should we choose a slightly more generic name? How about
> > storage_quota_types.h?
> > 
> > per http://www.chromium.org/developers/content-module/content-api
> > "each interface/struct/enum should be in a separate file"
> 
> OK. Somehow the "each" in your original mail on this didn't register. Sounds
> good.

ping

[jam, 2014-04-03 17:22:22]

lgtm

[Greg Billock, 2014-04-03 21:29:57]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-04-03 21:31:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/185793004/540001

[commit-bot: I haz the power, 2014-04-04 00:04:31]

Change committed as 261588