Update <video muted=""> handling to match spec

Update <video muted=""> handling to match spec

Per spec, "When a media element is created, if it has a muted attribute
specified, the user agent must mute the media element's audio output,
overriding any user preference." Only a parser-created element can have
an attribute at the time it is created.

BUG=350303

[philipj_slow, 2014-03-26 18:25:03]

I'm not super proud of the finishParsingAttributes() solution, but I tried for a
long to find some combination of existing information that would be equivalent
but had to give up...

Some relevant discussion in https://github.com/w3c/web-platform-tests/pull/729
and https://critic.hoppipolla.co.uk/r/956

https://www.w3.org/Bugs/Public/show_bug.cgi?id=25153 is not resolved, but I
uncommented the test since it passes and should keep passing until it's changed
intentionally.

[acolwell GONE FROM CHROMIUM, 2014-03-28 00:24:09]

This looks ok to me, but you should probably have someone who works on the HTML
parser and Element to weigh in.

[philipj_slow, 2014-03-28 07:32:38]

tkent, I see that you do some reviewing in this area, can you take a look or
suggest someone who should?

[tkent, 2014-03-28 07:36:14]

Adding parser gurus.

[abarth-chromium, 2014-03-28 17:20:20]

How does this affect
http://src.chromium.org/viewvc/blink/trunk/PerformanceTests/Parser/html-parse...
?

[philipj_slow, 2014-03-29 07:08:37]

On 2014/03/28 17:20:20, abarth wrote:
> How does this affect
>
http://src.chromium.org/viewvc/blink/trunk/PerformanceTests/Parser/html-parse...
> ?

The extra virtual function call in HTMLConstructionSite::createHTMLElement is
what could matter. Here are some results from local 5 local runs before and
after my patch:

r170344:

Finished: 5.464411 s
Finished: 5.605738 s
Finished: 5.556771 s
Finished: 5.438587 s
Finished: 5.561225 s

Average: 5.525346 s

r170344 + patch:

Finished: 5.766655 s
Finished: 5.471804 s
Finished: 5.527375 s
Finished: 5.662929 s
Finished: 5.776929 s

Average: 5.641138 s

Note that every time the following was printed as well:
ERROR: CONSOLE ERROR: Blocked script execution in 'html-parser.html' because the
document's frame is sandboxed and the 'allow-scripts' permission is not set.
FAILED

Are there bots I can run to test this?

[abarth-chromium, 2014-03-29 17:52:55]

On 2014/03/29 07:08:37, philipj wrote:
> On 2014/03/28 17:20:20, abarth wrote:
> > How does this affect
> >
>
http://src.chromium.org/viewvc/blink/trunk/PerformanceTests/Parser/html-parse...
> > ?
> 
> The extra virtual function call in HTMLConstructionSite::createHTMLElement is
> what could matter. Here are some results from local 5 local runs before and
> after my patch:
> 
> r170344:
> 
> Finished: 5.464411 s
> Finished: 5.605738 s
> Finished: 5.556771 s
> Finished: 5.438587 s
> Finished: 5.561225 s
> 
> Average: 5.525346 s
> 
> r170344 + patch:
> 
> Finished: 5.766655 s
> Finished: 5.471804 s
> Finished: 5.527375 s
> Finished: 5.662929 s
> Finished: 5.776929 s
> 
> Average: 5.641138 s

Hum.  That looks like might be a 2% regression.

> Note that every time the following was printed as well:
> ERROR: CONSOLE ERROR: Blocked script execution in 'html-parser.html' because
the
> document's frame is sandboxed and the 'allow-scripts' permission is not set.
> FAILED

That's expected.  I'm not sure why it printed out "failed", but the script is
blocked on purpose.

> Are there bots I can run to test this?

It's run on these bots:

https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=android-nexus...

Generally, I just run the benchmark locally though.  It's pretty stable.

[philipj_slow, 2014-03-31 16:25:25]

The previous tests were run on my laptop on battery so to be sure I re-ran them
on a desktop doing nothing else, and with 20 runs instead of 5. I also
calculated the standard deviation.

Before: avergate 8.80143675, stddev 0.039106552030092

After: average 8.9874171, stddev 0.030701540693709

The error ranges using twice the stddev (95% confidence) don't even overlap, so
this is indeed a 2% regression.

Clearly that's not acceptable. Do the parser gurus have any other ideas for how
to do this?

We could ask to have the spec changed to not treat the parser case specially,
but this feature was designed like this quite deliberately and has been
implemented per spec in Presto and Gecko, so I expect some resistance. (FWIW, I
checked how Gecko does this, and it also has a special callback for this
purpose, called DoneCreatingElement.)

[philipj_slow, 2014-03-31 16:55:25]

I tried moving the virtual call into Element::parserSetAttributes but it's still
a 1.5% regression.

[abarth-chromium, 2014-03-31 21:17:07]

Philosophically, I think it would be better if the parser wasn't a special case.
 We want developers to be able to create their own HTML parsers that work just
as well as the built-in one.  If there's a magical hook here that can be called
only by the parser, that becomes more difficult.

[philipj_slow, 2014-04-01 03:57:53]

On 2014/03/31 21:17:07, abarth wrote:
> Philosophically, I think it would be better if the parser wasn't a special
case.
>  We want developers to be able to create their own HTML parsers that work just
> as well as the built-in one.  If there's a magical hook here that can be
called
> only by the parser, that becomes more difficult.

In this case it could be replicated by just setting the muted IDL attribute, but
that would be a special case. I'll try to push a little harder to change the
spec.

[eseidel, 2014-04-02 19:48:40]

Seems we could just cheat and do this in finishedParsingChildren.  If someone
has a script tag which changes this while parsing the children, I doubt that
matters much.

[philipj_slow, 2014-04-02 20:33:13]

On 2014/04/02 19:48:40, eseidel wrote:
> Seems we could just cheat and do this in finishedParsingChildren.  If someone
> has a script tag which changes this while parsing the children, I doubt that
> matters much.

That was one of the other things I tried, but instead I wrote negative tests for
it, see "These tests are inside <audio>/<video> so that the steps for updating
the muted IDL attribute cannot be delayed until the end tag is parsed."

If the spec won't budge then I agree it would still be an improvement, but not
fail-safe. For example, the HTML parser can block on the network just before
</video>. HTMLMediaElement can happily load and play a video at that point,
which won't be muted until the parser sees </video>. Not fun to debug "muted
sometimes still makes a bit of sound, but not when testing locally"...

[philipj_slow, 2014-04-25 11:33:41]

I filed a new spec bug to hopefully move this along:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25460

I'll close this review until the spec bug is resolved.

[eseidel, 2014-05-06 08:45:27]

Sorry if you've covered this above.

Can't we just mute from inside parseAttribute()? and only if the element has
createdByParser set (which the parser knows how to do, you may need to set the
right flag in HTMLTags.in).

[philipj_slow, 2014-05-06 10:53:12]

On 2014/05/06 08:45:27, eseidel wrote:
> Sorry if you've covered this above.
> 
> Can't we just mute from inside parseAttribute()? and only if the element has
> createdByParser set (which the parser knows how to do, you may need to set the
> right flag in HTMLTags.in).

Unfortunately, that also wouldn't match the spec, in a case like this:

<!DOCTYPE html>
<video>
<script>
var v = document.querySelector('video');
v.setAttribute('muted', '');
w(v.muted);
</script>
</video>

(script inside video just to show that also checking isFinishedParsingChildren()
isn't going to help)

It's pretty easy to approximate what the spec says, but I've found no
combination of hacks to match it exactly.

One thought is to add a setByParser attribute to parseAttribute, but would that
be acceptable, assuming it doesn't affect performance?