Add Origin Trials Test interface, rather than attaching trials directly to internals.

Add Origin Trials Test interface, rather than attaching trials directly to internals.

This makes the test interface almost exactly like any other origin trial, and
means that we don't have to do any special handling at internals construction
time to support it. Because it lives on window.internals.originTrialTest() now,
I renamed the various test methods and attributes to be more descriptive of what
they are actually testing, rather than prefixing them with 'frobulate' to avoid
polluting the internals namespace.

BUG=647273
Committed: https://crrev.com/2076a9846ca82c6f6566d5fb7526729c853157ee
Cr-Commit-Position: refs/heads/master@{#419225}

[iclelland, 2016-09-15 15:43:39]

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

[commit-bot: I haz the power, 2016-09-15 15:44:08]

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-09-15 16:23:59]

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

[commit-bot: I haz the power, 2016-09-15 16:24:00]

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

[iclelland, 2016-09-15 17:46:02]

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

[commit-bot: I haz the power, 2016-09-15 17:46:52]

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

[iclelland, 2016-09-15 17:50:28]

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

[iclelland, 2016-09-15 17:50:29]

+r chasej, can you PTAL?

[commit-bot: I haz the power, 2016-09-15 20:54:10]

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

[commit-bot: I haz the power, 2016-09-15 20:54:11]

Dry run: This issue passed the CQ dry run.

[iclelland, 2016-09-16 13:17:35]

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

[iclelland, 2016-09-16 13:17:36]

+r haraken, can you PTAL as well? You're an OWNER, and this is also blocking
https://codereview.chromium.org/2260113002/, which I'd like to be able to
complete.

Thanks!

[haraken, 2016-09-16 13:48:46]

LGTM

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/OriginTrialsTest.idl (right):

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/OriginTrialsTest.idl:6: ] interface
OriginTrialsTest {

Remove the empty []. (foolip@ is landing a change to forbid it.)

[iclelland, 2016-09-16 14:38:43]

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/OriginTrialsTest.idl (right):

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/OriginTrialsTest.idl:6: ] interface
OriginTrialsTest {
On 2016/09/16 13:48:46, haraken wrote:
> 
> Remove the empty []. (foolip@ is landing a change to forbid it.)

Thanks, done! I saw that this morning when I tried to rebase.

[chasej, 2016-09-16 15:17:02]

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/enabled-worker.js
(right):

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/enabled-worker.js:8:
}, 'Attribute should exist and return value in worker');
It pre-dates this CL, but there intentionally isn't full coverage of all
scenarios in workers. I was just reminded, because looking at these tests, it
seems somewhat arbitrary which scenarios are tested. In fact, this CL changes
from testing an IDL binding check on an attribute, to testing a native enabled
check on an attribute.

I still think the minimal approach makes sense, since we're really just testing
that origin trials checks generally function in workers. Maybe a comment here
explaining that? And also in disabled-worker.js?

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/OriginTrialsTest.h (right):

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/OriginTrialsTest.h:18: // run-time. (This
is distinct from the InternalsFrobulate interface, which is a
InternalsFrobulate doesn't exist anymore. I gather this comment is making the
distinction from what is now OriginTrialsTestPartial. Update or remove the
comment?

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/OriginTrialsTest.idl (right):

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/OriginTrialsTest.idl:9:
[CallWith=ScriptState, RaisesException] readonly attribute boolean
throwingAttribute;
Nit: Maybe add a comment explaining that this throws when trial is not enabled
(i.e. use native enabled check)? I had to look at the .cpp to figure out the
intent.

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp (right):

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:106:
originTrialContext->setFeatureBindingsInstalled("Frobulate");
No longer using "Frobulate" in the naming of the IDL interface, and other code
artifacts. I'd suggest updating the trial name to match, or something more clear
as least.

[iclelland, 2016-09-16 15:59:47]

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/enabled-worker.js
(right):

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/enabled-worker.js:8:
}, 'Attribute should exist and return value in worker');
On 2016/09/16 15:17:02, chasej wrote:
> It pre-dates this CL, but there intentionally isn't full coverage of all
> scenarios in workers. I was just reminded, because looking at these tests, it
> seems somewhat arbitrary which scenarios are tested. In fact, this CL changes
> from testing an IDL binding check on an attribute, to testing a native enabled
> check on an attribute.

Yes it does; I should probably add the normalAttribute test as well, or replace
this one with it.

> 
> I still think the minimal approach makes sense, since we're really just
testing
> that origin trials checks generally function in workers. Maybe a comment here
> explaining that? And also in disabled-worker.js?

Done.

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/OriginTrialsTest.h (right):

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/OriginTrialsTest.h:18: // run-time. (This
is distinct from the InternalsFrobulate interface, which is a
On 2016/09/16 15:17:02, chasej wrote:
> InternalsFrobulate doesn't exist anymore. I gather this comment is making the
> distinction from what is now OriginTrialsTestPartial. Update or remove the
> comment?

Done.

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/OriginTrialsTest.idl (right):

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/OriginTrialsTest.idl:9:
[CallWith=ScriptState, RaisesException] readonly attribute boolean
throwingAttribute;
On 2016/09/16 15:17:02, chasej wrote:
> Nit: Maybe add a comment explaining that this throws when trial is not enabled
> (i.e. use native enabled check)? I had to look at the .cpp to figure out the
> intent.

Done.

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp (right):

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:106:
originTrialContext->setFeatureBindingsInstalled("Frobulate");
On 2016/09/16 15:17:02, chasej wrote:
> No longer using "Frobulate" in the naming of the IDL interface, and other code
> artifacts. I'd suggest updating the trial name to match, or something more
clear
> as least.

Can I do that as a separate CL? It will involve regenerating all of the trial
tokens, in addition to updating the code.

[iclelland, 2016-09-16 15:59:53]

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

[commit-bot: I haz the power, 2016-09-16 16:00:13]

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-09-16 17:41:11]

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

[commit-bot: I haz the power, 2016-09-16 17:41:12]

Dry run: This issue passed the CQ dry run.

[chasej, 2016-09-16 18:23:44]

LGTM

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp (right):

https://codereview.chromium.org/2340323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp:106:
originTrialContext->setFeatureBindingsInstalled("Frobulate");
On 2016/09/16 15:59:47, iclelland wrote:
> On 2016/09/16 15:17:02, chasej wrote:
> > No longer using "Frobulate" in the naming of the IDL interface, and other
code
> > artifacts. I'd suggest updating the trial name to match, or something more
> clear
> > as least.
> 
> Can I do that as a separate CL? It will involve regenerating all of the trial
> tokens, in addition to updating the code.

Yes, definitely can wait for a separate CL. I forgot about all the tokens

[iclelland, 2016-09-16 18:26:12]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-09-16 18:26:13]

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/2340323002/#ps50001
(title: "Updates")

[commit-bot: I haz the power, 2016-09-16 18:26:56]

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-09-16 18:34:05]

Committed patchset #4 (id:50001)

[commit-bot: I haz the power, 2016-09-16 18:36:10]

Description was changed from

==========
Add Origin Trials Test interface, rather than attaching trials directly to
internals.

This makes the test interface almost exactly like any other origin trial, and
means that we don't have to do any special handling at internals construction
time to support it. Because it lives on window.internals.originTrialTest() now,
I renamed the various test methods and attributes to be more descriptive of what
they are actually testing, rather than prefixing them with 'frobulate' to avoid
polluting the internals namespace.

BUG=647273
==========

to

==========
Add Origin Trials Test interface, rather than attaching trials directly to
internals.

This makes the test interface almost exactly like any other origin trial, and
means that we don't have to do any special handling at internals construction
time to support it. Because it lives on window.internals.originTrialTest() now,
I renamed the various test methods and attributes to be more descriptive of what
they are actually testing, rather than prefixing them with 'frobulate' to avoid
polluting the internals namespace.

BUG=647273

Committed: https://crrev.com/2076a9846ca82c6f6566d5fb7526729c853157ee
Cr-Commit-Position: refs/heads/master@{#419225}
==========

[commit-bot: I haz the power, 2016-09-16 18:36:11]

Patchset 4 (id:??) landed as
https://crrev.com/2076a9846ca82c6f6566d5fb7526729c853157ee
Cr-Commit-Position: refs/heads/master@{#419225}