Flip flag for Long Tasks to stable

Flip flag for Long Tasks to stable
I2S thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Mx9q5WXunSE

BUG=688439
Review-Url: https://codereview.chromium.org/2685183002
Cr-Commit-Position: refs/heads/master@{#450235}
Committed: https://chromium.googlesource.com/chromium/src/+/9775bfee2b12b717b138928078f95ff524af5564

[panicker, 2017-02-09 20:36:30]

panicker@chromium.org changed reviewers:
+ chasej@chromium.org

[panicker, 2017-02-09 20:36:30]

Hey Jason, could you sanity change this flag flip?
My intent is to flip the flag to stable for M58, and leave the origin trial as
is (active until M58 ships). Would you suggest removing the origin-trial in the
same CL?

[chasej, 2017-02-10 16:41:28]

On 2017/02/09 20:36:30, panicker wrote:
> Hey Jason, could you sanity change this flag flip?
> My intent is to flip the flag to stable for M58, and leave the origin trial as
> is (active until M58 ships). Would you suggest removing the origin-trial in
the
> same CL?

I would recommend removing the origin trial in the same CL.

There's no harm in leaving the origin trial active with the runtime flag set
to stable. However, the flag set to stable essentially makes the origin trial
redundant. The origin trials framework always checks the runtime flag first,
and only if not enabled will tokens be checked. So, having the origin trial
active is potentially confusing, and might have a trivial performance impact.
There is one exception, likely rare: the runtime flag could be explicitly
disabled (via command-line), in which case the origin trial would still
allow the feature to be used.

[panicker, 2017-02-10 20:49:39]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-02-10 20:49:42]

The CQ bit was unchecked by panicker@chromium.org

[panicker, 2017-02-10 21:09:46]

On 2017/02/10 16:41:28, chasej wrote:
> On 2017/02/09 20:36:30, panicker wrote:
> > Hey Jason, could you sanity change this flag flip?
> > My intent is to flip the flag to stable for M58, and leave the origin trial
as
> > is (active until M58 ships). Would you suggest removing the origin-trial in
> the
> > same CL?
> 
> I would recommend removing the origin trial in the same CL.
> 
> There's no harm in leaving the origin trial active with the runtime flag set
> to stable. However, the flag set to stable essentially makes the origin trial
> redundant. The origin trials framework always checks the runtime flag first,
> and only if not enabled will tokens be checked. So, having the origin trial
> active is potentially confusing, and might have a trivial performance impact.
> There is one exception, likely rare: the runtime flag could be explicitly
> disabled (via command-line), in which case the origin trial would still
> allow the feature to be used.

Thanks. PTAL.

[panicker, 2017-02-10 21:10:08]

panicker@chromium.org changed reviewers:
+ foolip@chromium.org

[panicker, 2017-02-10 21:10:09]

+foolip for API owners

[foolip, 2017-02-10 22:36:53]

Found an IDL mismatch that I think would be a good idea to fix before shipping.
Does that make sense?

https://codereview.chromium.org/2685183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl
(right):

https://codereview.chromium.org/2685183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl:5: //
https://github.com/wicg/longtasks
Can you update this link to
https://wicg.github.io/longtasks/#sec-PerformanceLongTaskTiming ?

https://codereview.chromium.org/2685183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl:7: readonly
attribute sequence<TaskAttributionTiming> attribution;
This does not match the spec, it should be a frozen array.

I've filed https://github.com/WICG/longtasks/issues/20, and if you add
[SameObject] to the spec you can also use [SaveSameObject] here to make it work

If it is always the same object, can you make sure there's a test for
entry.attribution===entry.attribution since this is so easy to get wrong?

[panicker, 2017-02-10 22:46:44]

https://codereview.chromium.org/2685183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl
(right):

https://codereview.chromium.org/2685183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl:5: //
https://github.com/wicg/longtasks
On 2017/02/10 22:36:53, foolip wrote:
> Can you update this link to
> https://wicg.github.io/longtasks/#sec-PerformanceLongTaskTiming ?

Done.

https://codereview.chromium.org/2685183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl:7: readonly
attribute sequence<TaskAttributionTiming> attribution;
On 2017/02/10 22:36:53, foolip wrote:
> This does not match the spec, it should be a frozen array.
> 
> I've filed https://github.com/WICG/longtasks/issues/20, and if you add
> [SameObject] to the spec you can also use [SaveSameObject] here to make it
work
> 
> If it is always the same object, can you make sure there's a test for
> entry.attribution===entry.attribution since this is so easy to get wrong?

This CL just landed so it is FrozenArray once I sync:
https://codereview.chromium.org/2680373004/

I'm a bit confused about the spec bug you filed, why would it always be the same
object (maybe we can discuss on the spec bug?)

[foolip, 2017-02-10 23:23:28]

https://codereview.chromium.org/2685183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl
(right):

https://codereview.chromium.org/2685183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl:7: readonly
attribute sequence<TaskAttributionTiming> attribution;
On 2017/02/10 22:46:44, panicker wrote:
> On 2017/02/10 22:36:53, foolip wrote:
> > This does not match the spec, it should be a frozen array.
> > 
> > I've filed https://github.com/WICG/longtasks/issues/20, and if you add
> > [SameObject] to the spec you can also use [SaveSameObject] here to make it
> work
> > 
> > If it is always the same object, can you make sure there's a test for
> > entry.attribution===entry.attribution since this is so easy to get wrong?
> 
> This CL just landed so it is FrozenArray once I sync:
> https://codereview.chromium.org/2680373004/
> 
> I'm a bit confused about the spec bug you filed, why would it always be the
same
> object (maybe we can discuss on the spec bug?)

I've tried to clarify the spec issue. I took a look at the code, and since
m_attribution is never changed after the constructor, adding [SameObject,
SaveSameObject] will do the right thing.

[panicker, 2017-02-10 23:45:01]

https://codereview.chromium.org/2685183002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl
(right):

https://codereview.chromium.org/2685183002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl:7: readonly
attribute sequence<TaskAttributionTiming> attribution;
On 2017/02/10 23:23:28, foolip wrote:
> On 2017/02/10 22:46:44, panicker wrote:
> > On 2017/02/10 22:36:53, foolip wrote:
> > > This does not match the spec, it should be a frozen array.
> > > 
> > > I've filed https://github.com/WICG/longtasks/issues/20, and if you add
> > > [SameObject] to the spec you can also use [SaveSameObject] here to make it
> > work
> > > 
> > > If it is always the same object, can you make sure there's a test for
> > > entry.attribution===entry.attribution since this is so easy to get wrong?
> > 
> > This CL just landed so it is FrozenArray once I sync:
> > https://codereview.chromium.org/2680373004/
> > 
> > I'm a bit confused about the spec bug you filed, why would it always be the
> same
> > object (maybe we can discuss on the spec bug?)
> 
> I've tried to clarify the spec issue. I took a look at the code, and since
> m_attribution is never changed after the constructor, adding [SameObject,
> SaveSameObject] will do the right thing.

Gotcha. Updated. Thanks for looking into  this!

[foolip, 2017-02-10 23:48:27]

https://codereview.chromium.org/2685183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl
(right):

https://codereview.chromium.org/2685183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl:7:
[SameObject, SaveSameObject] readonly attribute sequence<TaskAttributionTiming>
attribution;
This will conflict with https://codereview.chromium.org/2680373004/, but LGTM
with this rebased.

[foolip, 2017-02-10 23:49:08]

https://codereview.chromium.org/2685183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl
(right):

https://codereview.chromium.org/2685183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl:7:
[SameObject, SaveSameObject] readonly attribute sequence<TaskAttributionTiming>
attribution;
On 2017/02/10 23:48:27, foolip wrote:
> This will conflict with https://codereview.chromium.org/2680373004/, but LGTM
> with this rebased.

I also think you have to update some header file for SaveSameObject to work, but
the compiler will complain :)

[panicker, 2017-02-11 00:19:22]

Rebased & updated V8PrivateProperty.h

https://codereview.chromium.org/2685183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl
(right):

https://codereview.chromium.org/2685183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceLongTaskTiming.idl:7:
[SameObject, SaveSameObject] readonly attribute sequence<TaskAttributionTiming>
attribution;
On 2017/02/10 23:49:08, foolip wrote:
> On 2017/02/10 23:48:27, foolip wrote:
> > This will conflict with https://codereview.chromium.org/2680373004/, but
LGTM
> > with this rebased.
> 
> I also think you have to update some header file for SaveSameObject to work,
but
> the compiler will complain :)
Yeah I had to update V8PrivateProperty.h (interesting)
Also rebased.

[panicker, 2017-02-11 00:19:53]

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

[commit-bot: I haz the power, 2017-02-11 00:20:30]

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

[chasej, 2017-02-11 00:58:44]

lgtm

[commit-bot: I haz the power, 2017-02-11 02:00:47]

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

[commit-bot: I haz the power, 2017-02-11 02:00:48]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[panicker, 2017-02-14 00:14:53]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-02-14 00:14:53]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
chasej@chromium.org
Link to the patchset: https://codereview.chromium.org/2685183002/#ps140001
(title: "update webexposed virtual/stable tests and delete longtask origin-trial
tests (as the CL removes th…")

[commit-bot: I haz the power, 2017-02-14 00:15:29]

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-02-14 00:27:48]

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

[commit-bot: I haz the power, 2017-02-14 00:27:49]

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

[panicker, 2017-02-14 01:59:13]

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

[commit-bot: I haz the power, 2017-02-14 02:00:09]

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

[panicker, 2017-02-14 02:02:12]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-02-14 02:02:13]

The patchset sent to the CQ was uploaded after l-g-t-m from chasej@chromium.org,
foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2685183002/#ps160001
(title: "undo change to webview test (likely not needed)")

[commit-bot: I haz the power, 2017-02-14 02:03:41]

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-02-14 04:07:09]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487037732871700,
"parent_rev": "82e2114b9d4b41b5d619e0a401b77caf3074a97c", "commit_rev":
"9775bfee2b12b717b138928078f95ff524af5564"}

[commit-bot: I haz the power, 2017-02-14 04:08:05]

Description was changed from

==========
Flip flag for Long Tasks to stable
I2S thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Mx9q5WXunSE

BUG=688439
==========

to

==========
Flip flag for Long Tasks to stable
I2S thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Mx9q5WXunSE

BUG=688439

Review-Url: https://codereview.chromium.org/2685183002
Cr-Commit-Position: refs/heads/master@{#450235}
Committed:
https://chromium.googlesource.com/chromium/src/+/9775bfee2b12b717b138928078f9...
==========

[commit-bot: I haz the power, 2017-02-14 04:08:09]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/9775bfee2b12b717b138928078f9...