Update list of resource files to copy from web-platform-tests.

Update list of resource files to copy from web-platform-tests.

Context: There is a list of files from web-platform-tests that are
put in LayoutTests/resources. Because these are copied from
web-platform-tests, the copy in LayoutTests/resources is expected
to be identical to the one in the imported wpt directory, and this
is enforced by a PRESUBMIT.

This CL updates both the list in the PRESUBMIT, and the list of files
that are automatically copied over during the import process.

Associating this with bug 629275 since the w3c test autoroller
is currently getting stuck trying to import when idlharness.js
is updated but it's not copied over so it's failing the presubmit.

BUG=629275
Committed: https://crrev.com/051c4fd1f1b9b3fa5e1dc8387b2c0e69d84726a4
Cr-Commit-Position: refs/heads/master@{#416083}

[qyearsley, 2016-08-31 23:37:32]

Description was changed from

==========
Update list of resource files to copy from web-platform-tests.

Context: There is a list of files from web-platform-tests that are
put in LayoutTests/resources. Because these are copied from
web-platform-tests, the copy in LayoutTests/resources is expected
to be identical to the one in the imported wpt directory, and this
is enforced by a PRESUBMIT.

This CL updates both the list in the PRESUBMIT, and the list of files
that are automatically copied over during the import process.

BUG=629275
==========

to

==========
Update list of resource files to copy from web-platform-tests.

Context: There is a list of files from web-platform-tests that are
put in LayoutTests/resources. Because these are copied from
web-platform-tests, the copy in LayoutTests/resources is expected
to be identical to the one in the imported wpt directory, and this
is enforced by a PRESUBMIT.

This CL updates both the list in the PRESUBMIT, and the list of files
that are automatically copied over during the import process.

Associating this with bug 629275 since the w3c test autoroller
is currently getting stuck trying to import when idlharness.js
is updated but it's not copied over so it's failing the presubmit.

BUG=629275
==========

[qyearsley, 2016-08-31 23:41:02]

qyearsley@chromium.org changed reviewers:
+ jsbell@chromium.org, rbyers@chromium.org

[qyearsley, 2016-08-31 23:41:03]

Regarding testharnessreport.js:

As far as I understand, I think we *do* want
LayoutTests/resources/testharnessreport.js to be identical to
LayoutTests/imported/wpt/resources/testharnessreport.js, but we want it to be
our version of the file, not from upstream wpt, since I think that file is
supposed to contain Blink-specific additions to testharness.js which are
supposed to be added when running testharness.js tests both in imported wpt
tests and in Blink-only layout tests.

So, maybe as a follow-up we want to change the import process to copy from
LayoutTests/resources/testharnessreport.js to
LayoutTests/imported/wpt/resources/testharnessreport.js?

https://codereview.chromium.org/2302653002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (left):

https://codereview.chromium.org/2302653002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:112:
('testharnessreport.js', 'resources'),
Looking at this here, I'm pretty sure that we *don't* want to import
testharnessreport.js if it's modified, since that will overwrite our own custom
version of testharnessreport.js.

[tkent, 2016-09-01 02:01:22]

On 2016/08/31 at 23:41:03, qyearsley wrote:
> Regarding testharnessreport.js:
> 
> As far as I understand, I think we *do* want
LayoutTests/resources/testharnessreport.js to be identical to
LayoutTests/imported/wpt/resources/testharnessreport.js, but we want it to be
our version of the file, not from upstream wpt, since I think that file is
supposed to contain Blink-specific additions to testharness.js which are
supposed to be added when running testharness.js tests both in imported wpt
tests and in Blink-only layout tests.

True.

> 
> So, maybe as a follow-up we want to change the import process to copy from
LayoutTests/resources/testharnessreport.js to
LayoutTests/imported/wpt/resources/testharnessreport.js?

I think we already do it.  Patch Set 2 looks to stop it.

[qyearsley, 2016-09-01 16:41:29]

On 2016/09/01 at 02:01:22, tkent wrote:
> > So, maybe as a follow-up we want to change the import process to copy from
LayoutTests/resources/testharnessreport.js to
LayoutTests/imported/wpt/resources/testharnessreport.js?
> 
> I think we already do it.  Patch Set 2 looks to stop it.

Before this patch, testharnessreport.js is copied from imported/wpt/resources/
to resources/ on import; in this CL (as of Patch 2) this is removed, because if
we find a new testharnessreport.js when importing, then we don't want it to
overwrite our copy. I'm proposing that in a follow-up patch we could copy in the
reverse direction (from resources/ to imported/wpt/resources/).

[jsbell, 2016-09-01 16:51:05]

I'm confused about (1) the current behavior and (2) the desired behavior. Can we
list that out?

https://codereview.chromium.org/2302653002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/PRESUBMIT.py (left):

https://codereview.chromium.org/2302653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/PRESUBMIT.py:68:
'imported/wpt/resources/testharnessreport.js',
We still want testharnessreport.js to be identical, don't we?

(It's just that resources/ should be the "master", unlike the others)

https://codereview.chromium.org/2302653002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (left):

https://codereview.chromium.org/2302653002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:112:
('testharnessreport.js', 'resources'),
On 2016/08/31 23:41:03, qyearsley wrote:
> Looking at this here, I'm pretty sure that we *don't* want to import
> testharnessreport.js if it's modified, since that will overwrite our own
custom
> version of testharnessreport.js.

We *always* want to use "our" copy of testharnessreport.js - the one in wpt is
useless to us.

I thought we'd settled on using the upstream (wpt) version of the other files.
This method is "_copy_resources_to_wpt" so I assume it's copying e.g.
resources/testharness.js to imported/wpt/resources/testharness.js - which seems
backwards to me.

Is the function name incorrect? Or am I very confused?

[qyearsley, 2016-09-01 17:41:50]

On 2016/09/01 at 16:51:05, jsbell wrote:
> I'm confused about (1) the current behavior and (2) the desired behavior. Can
we list that out?
> 
>
https://codereview.chromium.org/2302653002/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/PRESUBMIT.py (left):
> 
>
https://codereview.chromium.org/2302653002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/PRESUBMIT.py:68:
'imported/wpt/resources/testharnessreport.js',
> We still want testharnessreport.js to be identical, don't we?
> 
> (It's just that resources/ should be the "master", unlike the others)
> 
>
https://codereview.chromium.org/2302653002/diff/20001/third_party/WebKit/Tool...
> File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (left):
> 
>
https://codereview.chromium.org/2302653002/diff/20001/third_party/WebKit/Tool...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:112:
('testharnessreport.js', 'resources'),
> On 2016/08/31 23:41:03, qyearsley wrote:
> > Looking at this here, I'm pretty sure that we *don't* want to import
> > testharnessreport.js if it's modified, since that will overwrite our own
custom
> > version of testharnessreport.js.
> 
> We *always* want to use "our" copy of testharnessreport.js - the one in wpt is
useless to us.
> 
> I thought we'd settled on using the upstream (wpt) version of the other files.
This method is "_copy_resources_to_wpt" so I assume it's copying e.g.
resources/testharness.js to imported/wpt/resources/testharness.js - which seems
backwards to me.
> 
> Is the function name incorrect? Or am I very confused?

Ah, you and Kent are right -- I was confused.

Right, so the current behavior (before this CL) is:
  (a) PRESUBMIT asserts that testharness.js, testharnessreport.js, and
idlharness.js are identical between the two places.
  (b) deps_updater copies WebIDLParser.js, testharnessreport.js, and
vendor-prefix.js from LayoutTests/resources into the newly-updated imported/wpt.

The desired behavior as far as I know is:
  (a) PRESUBMIT should also check that WebIDLParser.js and vendor-prefix.js are
identical.
  (b) deps_updater should also copy testharness.js and idlharness.js from wpt to
LayoutTests/resources.

I'm basing my idea about desired behavior (a) just on the fact that those are
copied into the wpt directory after update.
I'm basing my idea about desired behavior (b) on the fact that there's an
assertion in PRESUBMIT that they should be equal.

Does that sound correct?d

[jsbell, 2016-09-01 17:55:56]

On 2016/09/01 17:41:50, qyearsley wrote:
> The desired behavior as far as I know is:
>   (a) PRESUBMIT should also check that WebIDLParser.js and vendor-prefix.js
are
> identical.

Agreed. 

>   (b) deps_updater should also copy testharness.js and idlharness.js from wpt
to
> LayoutTests/resources.

Agreed, assuming I'm remembering history correctly.

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right):

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:110: we always
want to use our version, so these are copied after import to
nit: run-on sentence; can you split it up?

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:114: in
LayoutTests/PRESUBMIT.py should also be cahnged.
typo: "cahnged" -> "changed"

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:117: # upstream
versions.
Can drop "There are some"

But maybe document why we want "our" versions for each entry? (so that it's
clear to future maintainers)

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:120:
('WebIDLParser.js', 'resources'),
Why do we want our WebIDLParser.js instead of wpt's? 

If there's a good reason, can we call it out? If there's not, maybe a TODO to
decide/document it?

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:129: # There are
also some resources from the upstream repository that
Maybe move this list up near the other one near the start of the method? (just
personal preference; helps me compare them)

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:131:
resources_to_copy_from_wpt = [
As above, can we document why we want wpt's versions for each entry?

[qyearsley, 2016-09-01 18:16:25]

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right):

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:110: we always
want to use our version, so these are copied after import to
On 2016/09/01 at 17:55:55, jsbell wrote:
> nit: run-on sentence; can you split it up?

Done; also I think the comments below are redundant with this one, so I removed
those.

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:114: in
LayoutTests/PRESUBMIT.py should also be cahnged.
On 2016/09/01 at 17:55:55, jsbell wrote:
> typo: "cahnged" -> "changed"

Done

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:117: # upstream
versions.
On 2016/09/01 at 17:55:55, jsbell wrote:
> Can drop "There are some"
> 
> But maybe document why we want "our" versions for each entry? (so that it's
clear to future maintainers)

I'm actually not sure except for testharnessreport.js :-/ Added a note for
testharnessreport.js.

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:120:
('WebIDLParser.js', 'resources'),
On 2016/09/01 at 17:55:55, jsbell wrote:
> Why do we want our WebIDLParser.js instead of wpt's? 
> 
> If there's a good reason, can we call it out? If there's not, maybe a TODO to
decide/document it?

Not sure; added a TODO.

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:129: # There are
also some resources from the upstream repository that
On 2016/09/01 at 17:55:55, jsbell wrote:
> Maybe move this list up near the other one near the start of the method? (just
personal preference; helps me compare them)

Done

https://codereview.chromium.org/2302653002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:131:
resources_to_copy_from_wpt = [
On 2016/09/01 at 17:55:55, jsbell wrote:
> As above, can we document why we want wpt's versions for each entry?

Again, not completely sure :-/

[jsbell, 2016-09-01 18:30:07]

lgtm

(still unclear on why we prefer our WebIDLParser.js but that's not a behavior
change - we should try and dig it up)

[qyearsley, 2016-09-01 19:55:30]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2016-09-01 19:55:51]

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-01 22:23:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-01 22:25:14]

Description was changed from

==========
Update list of resource files to copy from web-platform-tests.

Context: There is a list of files from web-platform-tests that are
put in LayoutTests/resources. Because these are copied from
web-platform-tests, the copy in LayoutTests/resources is expected
to be identical to the one in the imported wpt directory, and this
is enforced by a PRESUBMIT.

This CL updates both the list in the PRESUBMIT, and the list of files
that are automatically copied over during the import process.

Associating this with bug 629275 since the w3c test autoroller
is currently getting stuck trying to import when idlharness.js
is updated but it's not copied over so it's failing the presubmit.

BUG=629275
==========

to

==========
Update list of resource files to copy from web-platform-tests.

Context: There is a list of files from web-platform-tests that are
put in LayoutTests/resources. Because these are copied from
web-platform-tests, the copy in LayoutTests/resources is expected
to be identical to the one in the imported wpt directory, and this
is enforced by a PRESUBMIT.

This CL updates both the list in the PRESUBMIT, and the list of files
that are automatically copied over during the import process.

Associating this with bug 629275 since the w3c test autoroller
is currently getting stuck trying to import when idlharness.js
is updated but it's not copied over so it's failing the presubmit.

BUG=629275

Committed: https://crrev.com/051c4fd1f1b9b3fa5e1dc8387b2c0e69d84726a4
Cr-Commit-Position: refs/heads/master@{#416083}
==========

[commit-bot: I haz the power, 2016-09-01 22:25:15]

Patchset 5 (id:??) landed as
https://crrev.com/051c4fd1f1b9b3fa5e1dc8387b2c0e69d84726a4
Cr-Commit-Position: refs/heads/master@{#416083}

[tkent, 2016-09-01 23:44:21]

On 2016/09/01 at 18:30:07, jsbell wrote:
> (still unclear on why we prefer our WebIDLParser.js but that's not a behavior
change - we should try and dig it up)

The original web-platform-tests/resources/ doesn't contain WebIDLParser.js.  In
wptserve, WebIDLParser.js is an alias of resources/webidl2/lib/webidl2.js [1]. 
We need to put an actual file resources/WebIDLParser.js because we don't run a
web server. So copying imported/wpt/resources/webidl2/lib/webidl2.js to
imported/wpt/resources/WebIDLParser.js would be more correct, and ideally we
should stop using WebIDLParser.js in the upstream.

[1]
https://github.com/w3c/wpt-tools/blob/de4d6ebd746bbba215da537b3f35e00b6b67482...

[jsbell, 2016-09-01 23:54:39]

On 2016/09/01 23:44:21, tkent wrote:
> The original web-platform-tests/resources/ doesn't contain WebIDLParser.js. 
In
> wptserve, WebIDLParser.js is an alias of resources/webidl2/lib/webidl2.js [1].

> We need to put an actual file resources/WebIDLParser.js because we don't run a
> web server. 

Ah, right. Thanks!

> So copying imported/wpt/resources/webidl2/lib/webidl2.js to
> imported/wpt/resources/WebIDLParser.js would be more correct

Yep, that sounds correct. And then copy imported/wpt/resources/WebIDLParser.js
to resources/WebIDLParser.js

(FWIW, it looks like we only use idlharness.js/WebIDLParser.js in two
non-imported tests. But I suppose we should not attempt to remove it until we
can land things directly in wpt.)

> and ideally we
> should stop using WebIDLParser.js in the upstream.

Just to confirm, you mean: fix web-platform-tests to reference webidl2.js by its
actual name rather than rely on an alias?
(That is, not something we should do in our repro.) Agreed!

[tkent, 2016-09-02 14:44:30]

On 2016/09/01 at 23:54:39, jsbell wrote:
> Just to confirm, you mean: fix web-platform-tests to reference webidl2.js by
its actual name rather than rely on an alias?
> (That is, not something we should do in our repro.) Agreed!

That's right!