Update tests for snapshot allowfullscreen behavior

Update tests for snapshot allowfullscreen behavior

As part of moving the Fullscreen API to Feature Policy, the behaviour of
the iframe "allowfullscreen" attribute is changing slightly -- it is now
being sampled once, when the frame content loads, and that value is used
when the frame tries to go fullscreen, regardless of whether it has been
changed. This CL updates the tests which were depending on that behavior
and introduces a new way of calling the cross-site-iframe-factory script
to attach additional attributes to iframes at creation time.

BUG=718155
Review-Url: https://codereview.chromium.org/2855973005
Cr-Commit-Position: refs/heads/master@{#469839}
Committed: https://chromium.googlesource.com/chromium/src/+/c3c6e3d369c0d2246d3e63eec0cddd12b7eaee2a

[iclelland, 2017-05-03 20:50:53]

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

[commit-bot: I haz the power, 2017-05-03 20:51:39]

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, 2017-05-03 21:38:28]

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

[commit-bot: I haz the power, 2017-05-03 21:38:29]

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

[iclelland, 2017-05-04 13:41:38]

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

[commit-bot: I haz the power, 2017-05-04 13:41:49]

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, 2017-05-04 15:08:25]

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

[commit-bot: I haz the power, 2017-05-04 15:08:26]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-05-04 15:39:13]

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

[iclelland, 2017-05-04 15:39:14]

+r alexmos, you appear to be the specific OWNER for this file, can you take a
look?

+cc foolip FYI because this is a change to Fullscreen tests

[alexmos, 2017-05-04 21:47:00]

LGTM for the test change, and +nick@ to double-check
cross_site_iframe_factory.html: Nick originally wrote it and might have
thoughts.

The factory change looks reasonable to me and something I've always wanted, so
thanks for doing it!

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
File content/test/data/cross_site_iframe_factory.html (right):

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
content/test/data/cross_site_iframe_factory.html:23:
cross_site_iframe_factory.html?a(b{allowfullscreen}(),c{sandbox-allow-scripts}(d))
Awesome, this can also simplify some of our other tests, such as the ones for
sandbox flags.  Any value in doing args with values as c{sandbox=allow-scripts},
which might be a bit easier to read?  Though I guess we can't avoid repeating
"sandbox" when you need to specify multiple sandbox values?

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
content/test/data/cross_site_iframe_factory.html:149: 
nit: no blank line, or move it one line down?

[alexmos, 2017-05-04 21:47:19]

alexmos@chromium.org changed reviewers:
+ nick@chromium.org

[alexmos, 2017-05-04 21:47:20]

Actually adding nick@

[ncarter (slow), 2017-05-04 22:06:56]

I love this

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
File content/test/data/cross_site_iframe_factory.html (right):

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
content/test/data/cross_site_iframe_factory.html:23:
cross_site_iframe_factory.html?a(b{allowfullscreen}(),c{sandbox-allow-scripts}(d))
On 2017/05/04 21:47:00, alexmos wrote:
> Awesome, this can also simplify some of our other tests, such as the ones for
> sandbox flags.  Any value in doing args with values as
c{sandbox=allow-scripts},
> which might be a bit easier to read?  Though I guess we can't avoid repeating
> "sandbox" when you need to specify multiple sandbox values?

I'm okay with either of these options.

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
content/test/data/cross_site_iframe_factory.html:105: function
splitSiteAndArgs(siteAndArgsString) {
Would it make more sense to do the parsing of the argument syntax as part of
TreeParserUtil? (applyIframeFeatures belongs here, but lexing the argument list
-- the {;} tokens -- seems like maybe it would be better done in the parser?)

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
content/test/data/cross_site_iframe_factory.html:143: if
(option.startsWith("sandbox-")) {
I think this means that a(b{sandbox-}) will work like <iframe sandbox> with no
sandbox arguments?

[iclelland, 2017-05-05 14:54:28]

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
File content/test/data/cross_site_iframe_factory.html (right):

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
content/test/data/cross_site_iframe_factory.html:23:
cross_site_iframe_factory.html?a(b{allowfullscreen}(),c{sandbox-allow-scripts}(d))
On 2017/05/04 21:47:00, alexmos wrote:
> Awesome, this can also simplify some of our other tests, such as the ones for
> sandbox flags.  Any value in doing args with values as
c{sandbox=allow-scripts},
> which might be a bit easier to read?  Though I guess we can't avoid repeating
> "sandbox" when you need to specify multiple sandbox values?

Right, we would still need to write it as
{sandbox=allow-scripts;sandbox=allow-popups}. I was trying to stay away from
anything that would make the query string look like a set of key-value pairs.

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
content/test/data/cross_site_iframe_factory.html:105: function
splitSiteAndArgs(siteAndArgsString) {
On 2017/05/04 22:06:56, ncarter wrote:
> Would it make more sense to do the parsing of the argument syntax as part of
> TreeParserUtil? (applyIframeFeatures belongs here, but lexing the argument
list
> -- the {;} tokens -- seems like maybe it would be better done in the parser?)

Maybe -- I wasn't sure how generic TreeParserUtil needed to be, but I can easily
move this method into there, and it can be called (or not) by the scripts that
use it.

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
content/test/data/cross_site_iframe_factory.html:143: if
(option.startsWith("sandbox-")) {
On 2017/05/04 22:06:56, ncarter wrote:
> I think this means that a(b{sandbox-}) will work like <iframe sandbox> with no
> sandbox arguments?

It should work like that -- It's a good hack :) I had considered also adding
a(b{sandbox}) as valid syntax, to create <iframe sandbox> with no args, but
didn't know if there was a use for it.

[iclelland, 2017-05-05 15:30:13]

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

[commit-bot: I haz the power, 2017-05-05 15:30:41]

Dry run: CQ is trying da patch.

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

[iclelland, 2017-05-05 15:32:43]

On 2017/05/05 14:54:28, iclelland wrote:
>
https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
> File content/test/data/cross_site_iframe_factory.html (right):
> 
>
https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
> content/test/data/cross_site_iframe_factory.html:23:
>
cross_site_iframe_factory.html?a(b{allowfullscreen}(),c{sandbox-allow-scripts}(d))
> On 2017/05/04 21:47:00, alexmos wrote:
> > Awesome, this can also simplify some of our other tests, such as the ones
for
> > sandbox flags.  Any value in doing args with values as
> c{sandbox=allow-scripts},
> > which might be a bit easier to read?  Though I guess we can't avoid
repeating
> > "sandbox" when you need to specify multiple sandbox values?
> 
> Right, we would still need to write it as
> {sandbox=allow-scripts;sandbox=allow-popups}. I was trying to stay away from
> anything that would make the query string look like a set of key-value pairs.
> 
>
https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
> content/test/data/cross_site_iframe_factory.html:105: function
> splitSiteAndArgs(siteAndArgsString) {
> On 2017/05/04 22:06:56, ncarter wrote:
> > Would it make more sense to do the parsing of the argument syntax as part of
> > TreeParserUtil? (applyIframeFeatures belongs here, but lexing the argument
> list
> > -- the {;} tokens -- seems like maybe it would be better done in the
parser?)
> 
> Maybe -- I wasn't sure how generic TreeParserUtil needed to be, but I can
easily
> move this method into there, and it can be called (or not) by the scripts that
> use it.
> 
>
https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
> content/test/data/cross_site_iframe_factory.html:143: if
> (option.startsWith("sandbox-")) {
> On 2017/05/04 22:06:56, ncarter wrote:
> > I think this means that a(b{sandbox-}) will work like <iframe sandbox> with
no
> > sandbox arguments?
> 
> It should work like that -- It's a good hack :) I had considered also adding
> a(b{sandbox}) as valid syntax, to create <iframe sandbox> with no args, but
> didn't know if there was a use for it.

ncarter@ -- can you take another look? I moved parsing into tree_parser_util.js,
and so restructured it a bit. It let me use ',' as the attribute separator,
which I like better (I fixed a bug in the token matcher as well -- it turns out
to be the reason that the reason I could get ';','{','}' in the hostname at all)

[commit-bot: I haz the power, 2017-05-05 17:35:10]

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

[commit-bot: I haz the power, 2017-05-05 17:35:11]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2017-05-05 22:34:05]

lgtm

https://codereview.chromium.org/2855973005/diff/40001/content/test/data/tree_...
File content/test/data/tree_parser_util.js (right):

https://codereview.chromium.org/2855973005/diff/40001/content/test/data/tree_...
content/test/data/tree_parser_util.js:46: *
TreeParserUtil.parse('b{red,blue}(c,d{green})';
So it turns out that I did write unittests for this, but couldn't figure out how
to hook them up to actually run. :(

If you want to play with them, there's a file in ps4 of the original code review
here:

https://codereview.chromium.org/1264923002/#ps50001

https://codereview.chromium.org/2855973005/diff/40001/content/test/data/tree_...
content/test/data/tree_parser_util.js:105: attributes:
takeAttributeList(tokenStream),
The jsdoc suggests that attributes here will be omitted if the result of
takeAttributeList() is empty.

An empty attribute list is more caller-friendly, but omitting the attributes
property is definitely more documentation-friendly.

I guess we should just go ahead and update the jsdoc?

https://codereview.chromium.org/2855973005/diff/40001/content/test/data/tree_...
content/test/data/tree_parser_util.js:147: // Additional entries allowed with
comman.
comman -> comma

[ncarter (slow), 2017-05-05 22:37:04]

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
File content/test/data/cross_site_iframe_factory.html (right):

https://codereview.chromium.org/2855973005/diff/20001/content/test/data/cross...
content/test/data/cross_site_iframe_factory.html:143: if
(option.startsWith("sandbox-")) {
On 2017/05/05 14:54:28, iclelland wrote:
> On 2017/05/04 22:06:56, ncarter wrote:
> > I think this means that a(b{sandbox-}) will work like <iframe sandbox> with
no
> > sandbox arguments?
> 
> It should work like that -- It's a good hack :) I had considered also adding
> a(b{sandbox}) as valid syntax, to create <iframe sandbox> with no args, but
> didn't know if there was a use for it.

I think in practice folks are likely going to want to allow scripts for testing.

[iclelland, 2017-05-05 23:31:19]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2017-05-05 23:31:20]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2855973005/#ps40001
(title: "Move parsing of attribute lists to tree_parser_util.js")

[iclelland, 2017-05-05 23:32:33]

The CQ bit was unchecked by iclelland@chromium.org

[commit-bot: I haz the power, 2017-05-05 23:33:00]

CQ is trying da patch.

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

[iclelland, 2017-05-06 03:03:10]

https://codereview.chromium.org/2855973005/diff/40001/content/test/data/tree_...
File content/test/data/tree_parser_util.js (right):

https://codereview.chromium.org/2855973005/diff/40001/content/test/data/tree_...
content/test/data/tree_parser_util.js:46: *
TreeParserUtil.parse('b{red,blue}(c,d{green})';
On 2017/05/05 22:34:05, ncarter wrote:
> So it turns out that I did write unittests for this, but couldn't figure out
how
> to hook them up to actually run. :(
> 
> If you want to play with them, there's a file in ps4 of the original code
review
> here:
> 
> https://codereview.chromium.org/1264923002/#ps50001

Thanks! I will definitely take a look.

https://codereview.chromium.org/2855973005/diff/40001/content/test/data/tree_...
content/test/data/tree_parser_util.js:105: attributes:
takeAttributeList(tokenStream),
On 2017/05/05 22:34:05, ncarter wrote:
> The jsdoc suggests that attributes here will be omitted if the result of
> takeAttributeList() is empty.
> 
> An empty attribute list is more caller-friendly, but omitting the attributes
> property is definitely more documentation-friendly.
> 
> I guess we should just go ahead and update the jsdoc?

Updated.

https://codereview.chromium.org/2855973005/diff/40001/content/test/data/tree_...
content/test/data/tree_parser_util.js:147: // Additional entries allowed with
comman.
On 2017/05/05 22:34:05, ncarter wrote:
> comman -> comma

Thanks, done

[iclelland, 2017-05-06 03:03:16]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2017-05-06 03:03:17]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2855973005/#ps60001
(title: "Update docs")

[commit-bot: I haz the power, 2017-05-06 03:03:46]

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-05-06 18:44:00]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494039796655740,
"parent_rev": "9423dea0bc0392b47696282d40770ab21c9177f9", "commit_rev":
"c3c6e3d369c0d2246d3e63eec0cddd12b7eaee2a"}

[commit-bot: I haz the power, 2017-05-06 18:44:12]

Description was changed from

==========
Update tests for snapshot allowfullscreen behavior

As part of moving the Fullscreen API to Feature Policy, the behaviour of
the iframe "allowfullscreen" attribute is changing slightly -- it is now
being sampled once, when the frame content loads, and that value is used
when the frame tries to go fullscreen, regardless of whether it has been
changed. This CL updates the tests which were depending on that behavior
and introduces a new way of calling the cross-site-iframe-factory script
to attach additional attributes to iframes at creation time.

BUG=718155
==========

to

==========
Update tests for snapshot allowfullscreen behavior

As part of moving the Fullscreen API to Feature Policy, the behaviour of
the iframe "allowfullscreen" attribute is changing slightly -- it is now
being sampled once, when the frame content loads, and that value is used
when the frame tries to go fullscreen, regardless of whether it has been
changed. This CL updates the tests which were depending on that behavior
and introduces a new way of calling the cross-site-iframe-factory script
to attach additional attributes to iframes at creation time.

BUG=718155

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

[commit-bot: I haz the power, 2017-05-06 18:44:14]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c3c6e3d369c0d2246d3e63eec0cd...