[webkit-reviews] review granted: [Bug 72841] NRWT: option --skip-pixel-test-if-no-baseline support on DRT : [Attachment 134242] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has granted Nandor Huszka
<hnandor at inf.u-szeged.hu>'s request for review:
Bug 72841: NRWT: option --skip-pixel-test-if-no-baseline support on DRT
https://bugs.webkit.org/show_bug.cgi?id=72841

Attachment 134242: Patch
https://bugs.webkit.org/attachment.cgi?id=134242&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134242&action=review


>> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:149
>> +		    test_input.should_run_pixel_test = False
> 
> This could be:
> 
> if self._port.get_option('pixel_tests') and
self._port.get_option('skip_pixel_test_if_no_baseline'):
>     test_input.should_run_pixel_test =
(self._port.expected_image(test_input.test_name) != None)
> else:
>      test_input.should_run_pixel_test = self._port.get_option('pixel_tests')

I'm not sure that this is clearer; the logic is kinda convoluted either way.

>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:41
>> +	    self.run_pixel_test = run_pixel_test
> 
> Nit: I like should_run_pixel_test (sounds like a bool) better than
run_pixel_test (sounds like an action or function).

works for me.

>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:227
>> +	    self._driver.start(True, [])
> 
> It's not obvious to me why we pass True here? Does this mean perftestrunner
will have pixel dumping enabled (is that the old behavior)? Maybe it's OK
because DriverProxy.start will be removed soon?

The value is overridden in practice by the run_test() call, but picking the
right default might save us from starting a non-pixel-test DRT and then having
to start a second one down the road (or vice versa). This should probably
either be self._port.get_option('pixel_tests') or stay as self._pixel_tests
(and then keep line 184 as well.

Tony's other nits seem fine to me. I'll mark this R+, CQ-; you can upload
another patch w/ the nits addressed and then anyone should be able to CQ+ it.