Making it possible to import mediastream tests.

Making it possible to import mediastream tests.

Since the W3C WebRTC tests involve permission prompts, I introduce a 
new mechanism whereby the test can specify class="instructions" 
for all elements that contain instructions to manual testers. 
These elements will be hidden by the importer upon import so 
they don't cause the test runner to be confused (it only wants 
to see pure testharness.js output).

Furthermore, since these tests depend on vendor-prefix.js to handle vendor 
prefixes, I add a slightly modified copy of that which is always on 
(i.e. doesn't need ?usePrefixes=1 in the URL to the test page), and logic 
to rewrite the paths so it points to our copy.

Note: must make the --use-fake-device-for-media-stream 
and --use-fake-ui-for-media-stream flags always be on for
layout tests before landing this patch.

BUG=389134
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177477

[phoglund_chromium, 2014-06-26 14:14:53]

On 2014/06/26 14:09:50, phoglund wrote:

Hm, when I think of it I should probably land the changes to
W3CImportExpectations separately (I need to read up on that magical five-step
dance one needs to do when importing w3c tests as well).

[Dirk Pranke, 2014-06-26 21:16:47]

Also, as you noted, probably better to leave the actual changes to
W3CImportExpectations to a separate CL.

https://codereview.chromium.org/353983003/diff/1/Tools/Scripts/webkitpy/w3c/t...
File Tools/Scripts/webkitpy/w3c/test_converter.py (right):

https://codereview.chromium.org/353983003/diff/1/Tools/Scripts/webkitpy/w3c/t...
Tools/Scripts/webkitpy/w3c/test_converter.py:148: new_path =
re.sub('/common/vendor-prefix',
I don't recall if you answered this question, but why do we need this script at
all rather than just converting the prefixes as part of the import like we do
for other things?

As I noted in the email thread, having two mechanisms for this doesn't seem
ideal.

https://codereview.chromium.org/353983003/diff/1/Tools/Scripts/webkitpy/w3c/t...
Tools/Scripts/webkitpy/w3c/test_converter.py:154: print attr_name, attr_value
this print shouldn't be here ...

https://codereview.chromium.org/353983003/diff/1/Tools/Scripts/webkitpy/w3c/t...
Tools/Scripts/webkitpy/w3c/test_converter.py:158: if attr_name == 'class' and
'instructions' in attr_value:
can we put this logic in testharnessreport.js instead?

[phoglund_chromium, 2014-06-27 08:33:45]

On 2014/06/26 21:16:47, Dirk Pranke wrote:
> Also, as you noted, probably better to leave the actual changes to
> W3CImportExpectations to a separate CL.
> 

Yeah, I'll take that out.

>
https://codereview.chromium.org/353983003/diff/1/Tools/Scripts/webkitpy/w3c/t...
> File Tools/Scripts/webkitpy/w3c/test_converter.py (right):
> 
>
https://codereview.chromium.org/353983003/diff/1/Tools/Scripts/webkitpy/w3c/t...
> Tools/Scripts/webkitpy/w3c/test_converter.py:148: new_path =
> re.sub('/common/vendor-prefix',
> I don't recall if you answered this question, but why do we need this script
at
> all rather than just converting the prefixes as part of the import like we do
> for other things?
> 
> As I noted in the email thread, having two mechanisms for this doesn't seem
> ideal.

I did kind of write an answer in the email thread, but here's a more complete
one:

There is a mechanism, but only for CSS properties. It reads
Source/core/css/CSSPropertyNames.in as the
source of truth of what is prefixed and not. I could not find a similar source
of truth for WebRTC-related
properties, so if we wanted something like that we would have to build it and
make the importer use it.
So let's assume we do that.

I guess that could be a text file that we made the test converter read. In that
case, maybe we could
put that text file somewhere in w3c and call it W3CVendorPrefixesForBlink. We
need to make replacements
like obj.srcObject -> obj.webkitSrcObject, navigator.getUserMedia ->
navigator.webkitGetUserMedia and
RTCPeerConnection -> webkitRTCPeerConnection. However, adapter.js is smart
enough to check the parent
element of obj.srcObject; it should only rewrite it if obj is a HTML media
element. There's no way we
can know that at python time though. Maybe we can get away with just brute force
replacing e.g.
(\w+)\.srcObject with \1.webkitSrcObject in the raw html.

So I thought it made sense to use vendor-prefixes.js since there wasn't really a
mechanism in place 
and that vendor-prefixes.js was very precise. But one could certainly argue it's
better to make the
new mechanism look as much as possible like the old one. If you think the above
sounds good I can give
it a shot and see if it works out.

>
https://codereview.chromium.org/353983003/diff/1/Tools/Scripts/webkitpy/w3c/t...
> Tools/Scripts/webkitpy/w3c/test_converter.py:154: print attr_name, attr_value
> this print shouldn't be here ...

Nope, good catch!

> 
>
https://codereview.chromium.org/353983003/diff/1/Tools/Scripts/webkitpy/w3c/t...
> Tools/Scripts/webkitpy/w3c/test_converter.py:158: if attr_name == 'class' and
> 'instructions' in attr_value:
> can we put this logic in testharnessreport.js instead?

No unfortunately, because these instructions never pass through testharness and
are completely outside it.
For instance:

<html>
<head>
<title>Assigning mediastream to a video element</title>
<link rel="author" title="Dominique Hazael-Massieux" href="mailto:dom@w3.org"/>
</head>
<body>
<p>When prompted, accept to share your video stream.</p>
<h1>Description</h1>
<p>This test checks that the MediaStream object returned by the success callback
in getUserMedia can be properly assigned to a video element via the
<code>srcObject</code> attribute.</p>
<div id='log'></div>
<script src=/resources/testharness.js></script>
<script src=/resources/testharnessreport.js></script>
<script>
// Testharness test...
</script>
</body>
</html>

It's the p's and h1's that will have the "instructions" class here.

[Dirk Pranke, 2014-06-28 00:18:15]

Thanks for the reply. I need to ponder what you've written and will have a
longer reply either over the weekend or on monday ...

[Dirk Pranke, 2014-06-30 21:59:53]

On 2014/06/28 00:18:15, Dirk Pranke wrote:
> Thanks for the reply. I need to ponder what you've written and will have a
> longer reply either over the weekend or on monday ...

Okay, a couple of thoughts ...

1) I now understand the object vs. css-property problem better (and have thought
about it more). It sounds like import-time is the best mechanism for css props,
but doesn't really work for objects, and vice versa for runtime and objects. So,
this is annoying, but perhaps the best we can do until we can kill all prefixes
:). So, I wouldn't try to do the W3CVendorPrefixesForBlink approach.

2) Technically, testharnessreport can do whatever we want it to do to the dom
tree, so I think we are fully capable of hiding the nodes then, and I think it
might be slightly nicer to only hide the nodes when running in testRunner mode.
So, it would be nice if we could make that work. But, given that we're stuck
rewriting some things given 1), I don't think rewriting the instructions is the
end of the world, either.

So, let me know how you'd like to proceed. I'd probably like to give this CL
another review once we've agreed on an approach.

Also, FYI, it looks like we're shortly going to be able to change the way
gclient works so that we can put the dependencies on w3c/web-platform-tests into
a DEPS file in Blink, which should eliminate the 3-sided and 5-sided nature of
the patches to roll the repos, which will be awesome, so you might want to wait
for that before trying to actually import things.

(and, sorry for the delay).

[phoglund_chromium, 2014-07-01 09:33:37]

On 2014/06/30 21:59:53, Dirk Pranke wrote:
> On 2014/06/28 00:18:15, Dirk Pranke wrote:
> > Thanks for the reply. I need to ponder what you've written and will have a
> > longer reply either over the weekend or on monday ...
> 
> Okay, a couple of thoughts ...
> 
> 1) I now understand the object vs. css-property problem better (and have
thought
> about it more). It sounds like import-time is the best mechanism for css
props,
> but doesn't really work for objects, and vice versa for runtime and objects.
So,
> this is annoying, but perhaps the best we can do until we can kill all
prefixes
> :). So, I wouldn't try to do the W3CVendorPrefixesForBlink approach.
> 

Ok. Yeah, I got the impression vendor prefixes are going away so perhaps we
shouldn't
invest too much into this. So I'll just go ahead and make the importer support
vendor-prefixes.js like above?

> 2) Technically, testharnessreport can do whatever we want it to do to the dom
> tree, so I think we are fully capable of hiding the nodes then, and I think it
> might be slightly nicer to only hide the nodes when running in testRunner
mode.
> So, it would be nice if we could make that work. But, given that we're stuck
> rewriting some things given 1), I don't think rewriting the instructions is
the
> end of the world, either.
> 

Yeah. I like that the rewrite is pretty explicit, and that if a developer loads
the html file manually they will see the same thing as the test runner. I'm not
quite happy with the code (it feels a bit hacky since it just rewrites the HTML
tag). It would be nicer if I could say like targetTag.attributes += 
("style", "display:none") and have something rebuild the HTML tag rather than
the direct text manipulation. I don't know though, I guess the above works.

> So, let me know how you'd like to proceed. I'd probably like to give this CL
> another review once we've agreed on an approach.
> 

I think the above approach is reasonable, so please do a review.

> Also, FYI, it looks like we're shortly going to be able to change the way
> gclient works so that we can put the dependencies on w3c/web-platform-tests
into
> a DEPS file in Blink, which should eliminate the 3-sided and 5-sided nature of
> the patches to roll the repos, which will be awesome, so you might want to
wait
> for that before trying to actually import things.
> 
> (and, sorry for the delay).

No worries :)

[phoglund_chromium, 2014-07-02 16:02:11]

Meanwhile, I've checked that layout tests indeed bypass the getusermedia
permission prompt and always assume the user answered "yes". That's good,
because almost all tests expect that. Please review this patch and I can proceed
to get the very most basic tests importing and running cleanly.

The only caveat so far is that I haven't been able to dig out pixel values from
video tags while running under DRT (I guess DRT doesn't bother about rendering
videos), so tests that check that aren't going to run right. For instance,
there's a test that ensures that we only get black pixels from a video tag after
disabling a media stream. I am however only getting opaque white pixels
regardless of what the video feed seems to be. That's acceptable for now though.

[Dirk Pranke, 2014-07-02 16:09:02]

lgtm. Sorry for the delay -- again! -- it totally slipped my mind yesterday :(.

https://codereview.chromium.org/353983003/diff/20001/LayoutTests/resources/ve...
File LayoutTests/resources/vendor-prefix.js (right):

https://codereview.chromium.org/353983003/diff/20001/LayoutTests/resources/ve...
LayoutTests/resources/vendor-prefix.js:1: /* Use this script when you want to
test APIs that use vendor prefixes
Is this file actually cloned from the w3c somewhere? If it is, it seems like
there should be a note to that effect (noting the original location), and
perhaps a copyright notice?

If it isn't, perhaps we don't need to worry about the non-blink prefixes?

[phoglund_chromium, 2014-07-03 08:39:40]

On 2014/07/02 16:09:02, Dirk Pranke wrote:
> lgtm. Sorry for the delay -- again! -- it totally slipped my mind yesterday
:(.
> 
>
https://codereview.chromium.org/353983003/diff/20001/LayoutTests/resources/ve...
> File LayoutTests/resources/vendor-prefix.js (right):
> 
>
https://codereview.chromium.org/353983003/diff/20001/LayoutTests/resources/ve...
> LayoutTests/resources/vendor-prefix.js:1: /* Use this script when you want to
> test APIs that use vendor prefixes
> Is this file actually cloned from the w3c somewhere? If it is, it seems like
> there should be a note to that effect (noting the original location), and
> perhaps a copyright notice?
> 
> If it isn't, perhaps we don't need to worry about the non-blink prefixes?

Added copyright notice and link to original source. I started a discussion on
email about the license.

[phoglund_chromium, 2014-07-03 08:39:47]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2014-07-03 08:40:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/phoglund@chromium.org/353983003/40001

[commit-bot: I haz the power, 2014-07-03 09:47:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/9234)

[commit-bot: I haz the power, 2014-07-03 09:50:13]

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

[commit-bot: I haz the power, 2014-07-03 09:50:14]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/9239)

[phoglund_chromium, 2014-07-03 12:41:21]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2014-07-03 12:42:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/phoglund@chromium.org/353983003/60001

[commit-bot: I haz the power, 2014-07-03 13:48:13]

Change committed as 177477