[webkit-reviews] review denied: [Bug 55191] rebaseline-chromium-webkit-tests doesn't handle GPU modifiers "correctly" : [Attachment 87817] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 55191: rebaseline-chromium-webkit-tests doesn't handle GPU modifiers
"correctly"
https://bugs.webkit.org/show_bug.cgi?id=55191

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=87817&action=review

In general looks OK, mostly some questions and a rebase is probably in order.

> Tools/ChangeLog:13
> +	   actually rebaselined will be deleted, even if only one of the
> +	   variants was actually rebaselined. This may cause odd problems,

Is this still the case?  Are there changes for this patch after rebaselining
against 55608?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:443
> +	   for line in self._get_iterable_expectations(self._expectations):
> +	       lineno += 1

Can you use enumerate() instead of counting manually?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:93
> +	   if not hasattr(self, '_operating_system'):

Is it possible for to always set these here and the sub classes can overwrite
them after calling Port.__init__?  It would also encourage calling
Port.__init__ as soon as possible (which is more like how C++ works).  This is
mainly confusing in ChromiumLinux.__init__ where _version is set before and
_architecture is set afterwards.

> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:-816
> -    option_parser.add_option('-w', '--webkit_canary',
> -				action='store_true',
> -				default=False,
> -				help=('If True, pull baselines from webkit.org
'
> -				      'canary bot.'))

Do we want to keep the flag and have it just tell the user that this flag no
longer does anything?  Might be confusing if someone tries to pass -w and they
just get an error.  Alternately, we may just want to mention in --help that we
always pull from the canaries.	Also, this could be a separate change.

>
Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.p
y:132
> +    d = {}

Please use a more descriptive variable name.