Specify TaskType of posted Task explicitly in Quota API (13)

Specify TaskType of posted Task explicitly in Quota API

Quota API doesn't mention the task sources.
Assigning TaskType::MiscPlatformAPI here.
http://w3c.github.io/quota-api/

BUG=624696
Review-Url: https://codereview.chromium.org/2583013002
Cr-Commit-Position: refs/heads/master@{#442582}
Committed: https://chromium.googlesource.com/chromium/src/+/ddff17c30b45bf94a4cf633177d91cdc1306084e

[tzik, 2016-12-18 14:37:26]

Description was changed from

==========
Specify TaskType of posted Task explicitly in Quota API (13)

BUG=
==========

to

==========
Specify TaskType of posted Task explicitly in Quota API

Quota API doesn't mention the task sources.
Assigning TaskType::Internal here.
http://w3c.github.io/quota-api/

BUG=624696
==========

[tzik, 2016-12-18 14:37:32]

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

[commit-bot: I haz the power, 2016-12-18 14:37:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-18 16:04:07]

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

[commit-bot: I haz the power, 2016-12-18 16:04:08]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-12-18 22:46:06]

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

[tzik, 2016-12-18 22:46:06]

PTAL

[haraken, 2016-12-19 00:17:17]

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

[haraken, 2016-12-19 00:17:18]

kinuko-san: Do you have any idea on what task source quota APIs are expected to
use?

These tasks are clearly defined in the spec, so it's not really nice to use
TaskType::Internal.

[kinuko, 2016-12-20 05:22:01]

On 2016/12/19 00:17:18, haraken wrote:
> kinuko-san: Do you have any idea on what task source quota APIs are expected
to
> use?
> 
> These tasks are clearly defined in the spec, so it's not really nice to use
> TaskType::Internal.

Uh. This could be its own task source, like quota task source, or should use the
same task source as Storage API but it doesn't seem to specify the task source
either.

By the way- many not-yet-really-standard platform APIs don't mention their task
sources (e.g. notifications API, Web bluetooth API, push API etc) or are
supposed to use their own task sources (e.g. permissions API, Performance
Timeline API, sensor API etc etc).  I imagine more new APIs would follow the
latter pattern, then we'll end up having many not-really-fully-defined task
types.  I wonder it might be more scalable if instead we have a task type for
the platform APIs that are not really stable yet and could possibly use their
own task sources (i.e. different task sources from other well-defined task
sources)?

[haraken, 2016-12-20 10:09:35]

On 2016/12/20 05:22:01, kinuko wrote:
> On 2016/12/19 00:17:18, haraken wrote:
> > kinuko-san: Do you have any idea on what task source quota APIs are expected
> to
> > use?
> > 
> > These tasks are clearly defined in the spec, so it's not really nice to use
> > TaskType::Internal.
> 
> Uh. This could be its own task source, like quota task source, or should use
the
> same task source as Storage API but it doesn't seem to specify the task source
> either.
> 
> By the way- many not-yet-really-standard platform APIs don't mention their
task
> sources (e.g. notifications API, Web bluetooth API, push API etc) or are
> supposed to use their own task sources (e.g. permissions API, Performance
> Timeline API, sensor API etc etc).  I imagine more new APIs would follow the
> latter pattern, then we'll end up having many not-really-fully-defined task
> types.  I wonder it might be more scalable if instead we have a task type for
> the platform APIs that are not really stable yet and could possibly use their
> own task sources (i.e. different task sources from other well-defined task
> sources)?

Yeah.

Do you object to using TaskType::Internal until the task type is speced?

That might sound strange because TaskType::Internal is intended to be used for
Blink-internal (i.e., not-speced) tasks. However, we already have
TaskType::InternalLoading and TaskType::InternalTimer, so I'm not really happy
about introducing something like
TaskType::TaskIsSpecedButTaskTypeIsNotYetDefined.

[kinuko, 2016-12-21 13:58:45]

On 2016/12/20 10:09:35, haraken wrote:
> On 2016/12/20 05:22:01, kinuko wrote:
> > On 2016/12/19 00:17:18, haraken wrote:
> > > kinuko-san: Do you have any idea on what task source quota APIs are
expected
> > to
> > > use?
> > > 
> > > These tasks are clearly defined in the spec, so it's not really nice to
use
> > > TaskType::Internal.
> > 
> > Uh. This could be its own task source, like quota task source, or should use
> the
> > same task source as Storage API but it doesn't seem to specify the task
source
> > either.
> > 
> > By the way- many not-yet-really-standard platform APIs don't mention their
> task
> > sources (e.g. notifications API, Web bluetooth API, push API etc) or are
> > supposed to use their own task sources (e.g. permissions API, Performance
> > Timeline API, sensor API etc etc).  I imagine more new APIs would follow the
> > latter pattern, then we'll end up having many not-really-fully-defined task
> > types.  I wonder it might be more scalable if instead we have a task type
for
> > the platform APIs that are not really stable yet and could possibly use
their
> > own task sources (i.e. different task sources from other well-defined task
> > sources)?
> 
> Yeah.
> 
> Do you object to using TaskType::Internal until the task type is speced?
> 
> That might sound strange because TaskType::Internal is intended to be used for
> Blink-internal (i.e., not-speced) tasks. However, we already have
> TaskType::InternalLoading and TaskType::InternalTimer, so I'm not really happy
> about introducing something like
> TaskType::TaskIsSpecedButTaskTypeIsNotYetDefined.

TaskType::Internal sounds okay to me, though it feels slightly weird to me too
for the same reason as you mentioned.

What do you think about introducing something like TaskType::MiscPlatformAPI or
something?

[haraken, 2016-12-21 15:30:03]

On 2016/12/21 13:58:45, kinuko wrote:
> On 2016/12/20 10:09:35, haraken wrote:
> > On 2016/12/20 05:22:01, kinuko wrote:
> > > On 2016/12/19 00:17:18, haraken wrote:
> > > > kinuko-san: Do you have any idea on what task source quota APIs are
> expected
> > > to
> > > > use?
> > > > 
> > > > These tasks are clearly defined in the spec, so it's not really nice to
> use
> > > > TaskType::Internal.
> > > 
> > > Uh. This could be its own task source, like quota task source, or should
use
> > the
> > > same task source as Storage API but it doesn't seem to specify the task
> source
> > > either.
> > > 
> > > By the way- many not-yet-really-standard platform APIs don't mention their
> > task
> > > sources (e.g. notifications API, Web bluetooth API, push API etc) or are
> > > supposed to use their own task sources (e.g. permissions API, Performance
> > > Timeline API, sensor API etc etc).  I imagine more new APIs would follow
the
> > > latter pattern, then we'll end up having many not-really-fully-defined
task
> > > types.  I wonder it might be more scalable if instead we have a task type
> for
> > > the platform APIs that are not really stable yet and could possibly use
> their
> > > own task sources (i.e. different task sources from other well-defined task
> > > sources)?
> > 
> > Yeah.
> > 
> > Do you object to using TaskType::Internal until the task type is speced?
> > 
> > That might sound strange because TaskType::Internal is intended to be used
for
> > Blink-internal (i.e., not-speced) tasks. However, we already have
> > TaskType::InternalLoading and TaskType::InternalTimer, so I'm not really
happy
> > about introducing something like
> > TaskType::TaskIsSpecedButTaskTypeIsNotYetDefined.
> 
> TaskType::Internal sounds okay to me, though it feels slightly weird to me too
> for the same reason as you mentioned.
> 
> What do you think about introducing something like TaskType::MiscPlatformAPI
or
> something?

Sounds good.

[tzik, 2017-01-05 04:18:35]

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

[commit-bot: I haz the power, 2017-01-05 04:19:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-01-05 04:26:57]

On 2016/12/21 15:30:03, haraken wrote:
> On 2016/12/21 13:58:45, kinuko wrote:
> > On 2016/12/20 10:09:35, haraken wrote:
> > > On 2016/12/20 05:22:01, kinuko wrote:
> > > > On 2016/12/19 00:17:18, haraken wrote:
> > > > > kinuko-san: Do you have any idea on what task source quota APIs are
> > expected
> > > > to
> > > > > use?
> > > > > 
> > > > > These tasks are clearly defined in the spec, so it's not really nice
to
> > use
> > > > > TaskType::Internal.
> > > > 
> > > > Uh. This could be its own task source, like quota task source, or should
> use
> > > the
> > > > same task source as Storage API but it doesn't seem to specify the task
> > source
> > > > either.
> > > > 
> > > > By the way- many not-yet-really-standard platform APIs don't mention
their
> > > task
> > > > sources (e.g. notifications API, Web bluetooth API, push API etc) or are
> > > > supposed to use their own task sources (e.g. permissions API,
Performance
> > > > Timeline API, sensor API etc etc).  I imagine more new APIs would follow
> the
> > > > latter pattern, then we'll end up having many not-really-fully-defined
> task
> > > > types.  I wonder it might be more scalable if instead we have a task
type
> > for
> > > > the platform APIs that are not really stable yet and could possibly use
> > their
> > > > own task sources (i.e. different task sources from other well-defined
task
> > > > sources)?
> > > 
> > > Yeah.
> > > 
> > > Do you object to using TaskType::Internal until the task type is speced?
> > > 
> > > That might sound strange because TaskType::Internal is intended to be used
> for
> > > Blink-internal (i.e., not-speced) tasks. However, we already have
> > > TaskType::InternalLoading and TaskType::InternalTimer, so I'm not really
> happy
> > > about introducing something like
> > > TaskType::TaskIsSpecedButTaskTypeIsNotYetDefined.
> > 
> > TaskType::Internal sounds okay to me, though it feels slightly weird to me
too
> > for the same reason as you mentioned.
> > 
> > What do you think about introducing something like TaskType::MiscPlatformAPI
> or
> > something?
> 
> Sounds good.

OK, so I added MiscPlatformAPI and specified it in the Quota code. PTAL.

[tzik, 2017-01-05 04:27:17]

Description was changed from

==========
Specify TaskType of posted Task explicitly in Quota API

Quota API doesn't mention the task sources.
Assigning TaskType::Internal here.
http://w3c.github.io/quota-api/

BUG=624696
==========

to

==========
Specify TaskType of posted Task explicitly in Quota API

Quota API doesn't mention the task sources.
Assigning TaskType::MiscPlatformAPI here.
http://w3c.github.io/quota-api/

BUG=624696
==========

[haraken, 2017-01-05 04:32:45]

LGTM

https://codereview.chromium.org/2583013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.h (right):

https://codereview.chromium.org/2583013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:40: MiscPlatformAPI,

Add a comment like:

// Use MiscPlatformAPI for a task that is defined in the spec but is not yet
associated with any specific task runner in the spec. MiscPlatformAPI is not
encouraged. The spec should define the task runner explicitly.

[tzik, 2017-01-05 05:19:42]

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

[commit-bot: I haz the power, 2017-01-05 05:19:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-01-05 05:20:28]

https://codereview.chromium.org/2583013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.h (right):

https://codereview.chromium.org/2583013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:40: MiscPlatformAPI,
On 2017/01/05 04:32:45, haraken wrote:
> 
> Add a comment like:
> 
> // Use MiscPlatformAPI for a task that is defined in the spec but is not yet
> associated with any specific task runner in the spec. MiscPlatformAPI is not
> encouraged. The spec should define the task runner explicitly.
> 

Done.

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

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

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

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-01-05 07:52:29]

Might we also allow emerging, experimental tiny APIs to use the MiscPlatformAPI
type too until they're stabilized or we have strong motivations?  Also could we
add a comment about what task types run in an ordered way in the current
implementation (with additional note that it might change in a future but with
PSA)?  I'm afraid about psychological cost on developers for having so many task
types which say nothing about ordering.  We might introduce new code that relies
on the current ordering guarantee then, but I imagine it will happen anyway as
far as we run some task types on the same task runner.

[kinuko, 2017-01-05 07:52:56]

(Otherwise lgtm, thanks)

[tzik, 2017-01-05 11:40:40]

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

[commit-bot: I haz the power, 2017-01-05 11:40:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-01-05 11:42:33]

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

[commit-bot: I haz the power, 2017-01-05 11:42:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-01-05 11:50:35]

On 2017/01/05 07:52:29, kinuko wrote:
> Might we also allow emerging, experimental tiny APIs to use the
MiscPlatformAPI
> type too until they're stabilized or we have strong motivations?  Also could
we
> add a comment about what task types run in an ordered way in the current
> implementation (with additional note that it might change in a future but with
> PSA)?  I'm afraid about psychological cost on developers for having so many
task
> types which say nothing about ordering.  We might introduce new code that
relies
> on the current ordering guarantee then, but I imagine it will happen anyway as
> far as we run some task types on the same task runner.

Added some comment to mention emerging APIs and ordering.
Though most of tasks run in a sequence in the current implementation, I think
the user of TaskRunnerHelper should not assume that. Since the assumption makes
the migration hard.

[commit-bot: I haz the power, 2017-01-05 14:00:11]

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

[commit-bot: I haz the power, 2017-01-05 14:00:12]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-01-10 07:06:43]

On 2017/01/05 11:50:35, tzik wrote:
> On 2017/01/05 07:52:29, kinuko wrote:
> > Might we also allow emerging, experimental tiny APIs to use the
> MiscPlatformAPI
> > type too until they're stabilized or we have strong motivations?  Also could
> we
> > add a comment about what task types run in an ordered way in the current
> > implementation (with additional note that it might change in a future but
with
> > PSA)?  I'm afraid about psychological cost on developers for having so many
> task
> > types which say nothing about ordering.  We might introduce new code that
> relies
> > on the current ordering guarantee then, but I imagine it will happen anyway
as
> > far as we run some task types on the same task runner.
> 
> Added some comment to mention emerging APIs and ordering.
> Though most of tasks run in a sequence in the current implementation, I think
> the user of TaskRunnerHelper should not assume that. Since the assumption
makes
> the migration hard.

(Sorry the comment is a bit digressing from the change itself) existing code
relies on the old assumption, it'd be still useful to note both the current impl
and the future plan that could break the assumption (so we can more clearly say
'new code should not assume that').  Otherwise developers might need to be
puzzled why the existing code is coded like that, and could just look into how
the current impl is done and start introducing new voodoos (e.g. ah, these task
types can run sequentially!).  Task ordering issue is very subtle and code
relying on the existing impl has been totally valid, having clear documentation
would be not harmful at all (or must be beneficial) imo.

[tzik, 2017-01-10 12:26:13]

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

[commit-bot: I haz the power, 2017-01-10 12:26:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-01-10 12:28:43]

On 2017/01/10 07:06:43, kinuko wrote:
> On 2017/01/05 11:50:35, tzik wrote:
> > On 2017/01/05 07:52:29, kinuko wrote:
> > > Might we also allow emerging, experimental tiny APIs to use the
> > MiscPlatformAPI
> > > type too until they're stabilized or we have strong motivations?  Also
could
> > we
> > > add a comment about what task types run in an ordered way in the current
> > > implementation (with additional note that it might change in a future but
> with
> > > PSA)?  I'm afraid about psychological cost on developers for having so
many
> > task
> > > types which say nothing about ordering.  We might introduce new code that
> > relies
> > > on the current ordering guarantee then, but I imagine it will happen
anyway
> as
> > > far as we run some task types on the same task runner.
> > 
> > Added some comment to mention emerging APIs and ordering.
> > Though most of tasks run in a sequence in the current implementation, I
think
> > the user of TaskRunnerHelper should not assume that. Since the assumption
> makes
> > the migration hard.
> 
> (Sorry the comment is a bit digressing from the change itself) existing code
> relies on the old assumption, it'd be still useful to note both the current
impl
> and the future plan that could break the assumption (so we can more clearly
say
> 'new code should not assume that').  Otherwise developers might need to be
> puzzled why the existing code is coded like that, and could just look into how
> the current impl is done and start introducing new voodoos (e.g. ah, these
task
> types can run sequentially!).  Task ordering issue is very subtle and code
> relying on the existing impl has been totally valid, having clear
documentation
> would be not harmful at all (or must be beneficial) imo.

Can we do the description update in a separate CL?

[kinuko, 2017-01-10 12:30:37]

On 2017/01/10 12:28:43, tzik wrote:
> On 2017/01/10 07:06:43, kinuko wrote:
> > On 2017/01/05 11:50:35, tzik wrote:
> > > On 2017/01/05 07:52:29, kinuko wrote:
> > > > Might we also allow emerging, experimental tiny APIs to use the
> > > MiscPlatformAPI
> > > > type too until they're stabilized or we have strong motivations?  Also
> could
> > > we
> > > > add a comment about what task types run in an ordered way in the current
> > > > implementation (with additional note that it might change in a future
but
> > with
> > > > PSA)?  I'm afraid about psychological cost on developers for having so
> many
> > > task
> > > > types which say nothing about ordering.  We might introduce new code
that
> > > relies
> > > > on the current ordering guarantee then, but I imagine it will happen
> anyway
> > as
> > > > far as we run some task types on the same task runner.
> > > 
> > > Added some comment to mention emerging APIs and ordering.
> > > Though most of tasks run in a sequence in the current implementation, I
> think
> > > the user of TaskRunnerHelper should not assume that. Since the assumption
> > makes
> > > the migration hard.
> > 
> > (Sorry the comment is a bit digressing from the change itself) existing code
> > relies on the old assumption, it'd be still useful to note both the current
> impl
> > and the future plan that could break the assumption (so we can more clearly
> say
> > 'new code should not assume that').  Otherwise developers might need to be
> > puzzled why the existing code is coded like that, and could just look into
how
> > the current impl is done and start introducing new voodoos (e.g. ah, these
> task
> > types can run sequentially!).  Task ordering issue is very subtle and code
> > relying on the existing impl has been totally valid, having clear
> documentation
> > would be not harmful at all (or must be beneficial) imo.
> 
> Can we do the description update in a separate CL?

Sure. I didn't mean to block this cl (thus gave a lgtm)

[commit-bot: I haz the power, 2017-01-10 14:04:55]

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

[commit-bot: I haz the power, 2017-01-10 14:04:56]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-01-10 14:37:02]

The CQ bit was checked by tzik@chromium.org

[tzik, 2017-01-10 14:37:03]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2583013002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-10 14:37:25]

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

[commit-bot: I haz the power, 2017-01-10 14:41:05]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484059022724070,
"parent_rev": "05750b71186470dd0e4e2096e0209b75003b976b", "commit_rev":
"ddff17c30b45bf94a4cf633177d91cdc1306084e"}

[commit-bot: I haz the power, 2017-01-10 14:41:35]

Description was changed from

==========
Specify TaskType of posted Task explicitly in Quota API

Quota API doesn't mention the task sources.
Assigning TaskType::MiscPlatformAPI here.
http://w3c.github.io/quota-api/

BUG=624696
==========

to

==========
Specify TaskType of posted Task explicitly in Quota API

Quota API doesn't mention the task sources.
Assigning TaskType::MiscPlatformAPI here.
http://w3c.github.io/quota-api/

BUG=624696

Review-Url: https://codereview.chromium.org/2583013002
Cr-Commit-Position: refs/heads/master@{#442582}
Committed:
https://chromium.googlesource.com/chromium/src/+/ddff17c30b45bf94a4cf633177d9...
==========

[commit-bot: I haz the power, 2017-01-10 14:41:36]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ddff17c30b45bf94a4cf633177d9...