[webkit-reviews] review granted: [Bug 83348] add a webkit-patch print-baselines command : [Attachment 136117] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 83348: add a webkit-patch print-baselines command
https://bugs.webkit.org/show_bug.cgi?id=83348

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

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


> Tools/Scripts/webkitpy/layout_tests/port/base.py:324
> +    def expected_baseline_dict(self, test_name):
> +	   """Returns a dict mapping baseline suffix to relative path for each
baseline in
> +	   a test. For reftests, it returns ".==" or ".!=" instead of the
suffix."""
> +	   # FIXME: The name similarity between this and expected_baselines()
below, is unfortunate.
> +	   # We should probably rename them both.
> +	   baseline_dict = {}
> +	   reference_files = self.reference_files(test_name)
> +	   if reference_files:
> +	       # FIXME: How should this handle more than one type of reftest?
> +	       baseline_dict['.' + reference_files[0][0]] =
self.relative_test_filename(reference_files[0][1])
> +
> +	   for extension in self.baseline_extensions():
> +	       path = self.expected_filename(test_name, extension,
return_default=False)
> +	       baseline_dict[extension] = self.relative_test_filename(path) if
path else path
> +
> +	   return baseline_dict

So, if png is missing, then this will map PNG to none?	Interesting.  The
reftest part of this seems like an odd special case that might bite us in the
future.

If you're looking for another name for this function, you might consider
expected_baselines_by_extension.  That's a naming convention we used a bit in
garden-o-matic to keep track of which dictionaries mapped what to what.

> Tools/Scripts/webkitpy/tool/commands/queries.py:509
> +	       make_option('-p', '--platform', action='store',
> +			   help='platform/port(s) to display expectations for.
Use glob-style wildcards for multiple ports (note that that will imply
--csv)'),
> +	       make_option('--all', action='store_true', default=False,
> +			   help='display the baselines for *all* tests'),
> +	       make_option('--csv', action='store_true', default=False,
> +			   help='Print a CSV-style report that includes the
port name, test_name, test platform, baseline type, baseline location, and
baseline platform'),
> +	       make_option('--include-virtual-tests', action='store_true',
> +			   help='Include virtual tests'),

You mention above that you're planning to write a followup patch that refactors
the port-factory make_option calls to be shared rather than copy/pasted.

> Tools/Scripts/webkitpy/tool/commands/queries.py:555
> +	   m = self._platform_regexp.match(relpath)

Can we use a more descriptive variable name than m ?