[webkit-reviews] review granted: [Bug 80355] Add a mechanism to rebaseline new ports : [Attachment 130238] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 80355: Add a mechanism to rebaseline new ports
https://bugs.webkit.org/show_bug.cgi?id=80355

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130238&action=review


I'm not sure whether these new names are any better.  I think these things are
just hard to name.  :(

> Tools/Scripts/webkitpy/layout_tests/port/builders.py:53
> +    "Webkit Mac10.7": {"port_name": "chromium-mac-lion", "specifiers":
set(["lion"]), "new_port_fallback_port_name": "chromium-mac-snowleopard"},

Maybe new_port_fallback_port_name -> move_overwritten_baselines_to ?  It's
slightly hard to convey what this property does succinctly.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:59
> +    argument_names = "BUILDER_NAME TEST_NAME
[EXISTING_BASELINE_MOVE_TO_DIRECTORY]"

EXISTING_BASELINE_MOVE_TO_DIRECTORY => PLATFORM_TO_MOVE_EXISTING_BASELINES_TO ?
 Arg.  These are hard things to name.  This is a port, not a directory name,
right?

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:70
> +    def _copy_existing_baseline(self, baseline_move_to_directory, test_name,
suffix):

baseline_move_to_directory -> platform_to_move_existing_baselines_to

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:77
> +	   new_baseline = os.path.join(port.baseline_path(),
self._file_name_for_expected_result(test_name, suffix))

_tool.filesystem.join (we do this to make testing easier on Windows)

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:84
> +	       if not self._tool.scm().exists(new_baseline):
> +		   self._tool.scm().add(new_baseline)

We should make sure this works if we're creating a new subdirectory for
new_baseline.  I think SCM.add does this properly, but we should check.