[webkit-reviews] review denied: [Bug 186045] webkit-test-runner: Add support for the reftest-wait class name : [Attachment 342158] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Frédéric Wang (:fredw)
<fred.wang at free.fr>'s request for review:
Bug 186045: webkit-test-runner: Add support for the reftest-wait class name
https://bugs.webkit.org/show_bug.cgi?id=186045

Attachment 342158: Patch

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




--- Comment #40 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 342158
  --> https://bugs.webkit.org/attachment.cgi?id=342158
Patch

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

>>> Tools/Scripts/webkitpy/port/driver.py:510
>>> +		     command += "'--check-reftest-wait"
>> 
>> Something seems to be off about the quotes at the start here.
> 
> The single quote is just a separator between arguments, it does not need to
be closed (see the rest of the _command_from_driver_input function for more
context).

Why do we need to have a runtime option for this? Why can't we always check for
this condition.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:404
> +static const char* checkReftestWaitScript =

I don't think it makes sense to implement a logic like this in JS if we're
modifying WTR
since we already have a code to wait for sub-resources to load.
If we're going with the injected script route, we're much better off adding a
generic mechanism to inject arbitrary JS into the test.
I really don't want WPT specific logic creeping into WTR or DRT. r- because of
this.