[webkit-reviews] review denied: [Bug 38879] new-run-webkit-tests: add an '--update-baseline' switch : [Attachment 55772] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 38879: new-run-webkit-tests: add an '--update-baseline' switch
https://bugs.webkit.org/show_bug.cgi?id=38879

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
The code logic looks correct and I agree that this is necessary functionality.
r- mostly just due to naming nits.
---------------------------------
http://wkrietveld.appspot.com/38879/diff/1/5
File WebKitTools/Scripts/webkitpy/layout_tests/port/test.py (left):

http://wkrietveld.appspot.com/38879/diff/1/5#oldcode141
WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:141: 
did you mean to delete these two blank lines?

http://wkrietveld.appspot.com/38879/diff/1/5
File WebKitTools/Scripts/webkitpy/layout_tests/port/test.py (right):

http://wkrietveld.appspot.com/38879/diff/1/5#newcode31
WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:31: from __future__
import with_statement
Is this just to satisfy the linter? __future__ is new to me.

http://wkrietveld.appspot.com/38879/diff/1/6
File WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right):

http://wkrietveld.appspot.com/38879/diff/1/6#newcode1527
WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1527: "generated
results"),
This description makes it sound like it will update every expected result for
this test, when, in practice, it will update only the result that happens to be
used on this platform.

--new-baseline and --update-baseline are too similar of names. We shouldn't
need the help text to remember which is which. How about:

--new-baseline --> --update-platform-baseline
--update-baseline --> --update-used-baseline

It's a bit more wordy, but at least it's clear which is which. We could add
short versions for easier typing (e.g. -b and -u?).

http://wkrietveld.appspot.com/38879/diff/1/7
File WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py
(right):

http://wkrietveld.appspot.com/38879/diff/1/7#newcode106
WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:106:
return original_open(name, mode, encoding)
Should we make sure the mode here is 'r' to avoid ever writing to disk?

http://wkrietveld.appspot.com/38879/diff/1/7#newcode124
WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:124:
'fast/canvas/canvas-zoom.html'])
Should we also test a case where --update-baseline would update the results in
platform/mac?

http://wkrietveld.appspot.com/38879/diff/1/9
File WebKitTools/Scripts/webkitpy/layout_tests/test_types/test_type_base.py
(right):

http://wkrietveld.appspot.com/38879/diff/1/9#newcode97
WebKitTools/Scripts/webkitpy/layout_tests/test_types/test_type_base.py:97:
new_baseline=True):
docstring?

I don't like this argument name. It reads like this is the actual new baseline,
not like a boolean. How about save_new_baseline?

Also, if we end up changing the name of the command-line flag, I think this
would be clearer. This could be update_platform_baseline.