[webkit-reviews] review denied: [Bug 187039] Add support for testdriver WPT tests : [Attachment 351375] Patch

[bugzilla-daemon at webkit.org]

Frédéric Wang (:fredw) <fred.wang at free.fr> has denied darshan
<dkadu at igalia.com>'s request for review:
Bug 187039: Add support for testdriver WPT tests
https://bugs.webkit.org/show_bug.cgi?id=187039

Attachment 351375: Patch

https://bugs.webkit.org/attachment.cgi?id=351375&action=review




--- Comment #3 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 351375
  --> https://bugs.webkit.org/attachment.cgi?id=351375
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351375&action=review

> LayoutTests/imported/w3c/ChangeLog:6
> +

Extra white spaces.

> LayoutTests/imported/w3c/ChangeLog:7
> +	   Add support for testdriver WPT tess

tests

> LayoutTests/imported/w3c/ChangeLog:15
> +

I think you should unskip testdriver tests from LayoutTests/infrastructure in
order to have something to test.

> LayoutTests/imported/w3c/web-platform-tests/resources/testdriver.js:3
> +

So talking with Youenn, the proper way to implement this is actually to import
the existing testdriver.js from upstream WPT (so that it implements
window.test_driver) and implement testdriver-vendor.js so that it overrides
window.test_driver_internal (or whatever else must be modified).

For example infrastructure/testdriver/click.html loads both files:
<script src="/resources/testdriver.js"></script>
<script src="/resources/testdriver-vendor.js"></script>

BTW, there should be some doc on upstream WPT explaining that.