[webkit-reviews] review denied: [Bug 67324] filter test_expectations properly for chromium-mac vs chromium-cg-mac : [Attachment 106129] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied epoger at google.com's request for
review:
Bug 67324: filter test_expectations properly for chromium-mac vs
chromium-cg-mac
https://bugs.webkit.org/show_bug.cgi?id=67324

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106129&action=review


Thanks for working on this. I'm hoping we can make the change in a way that
special-cases CG/Skia a bit less and hopefully keeps the code more
maintainable.

Also, please add tests. I'm pretty sure most of this code is already
unittested. You can run the unittests by running test-webkitpy.

At a higher level, I'm not sure we should tie CG/Skia-ness to GPU/CPU. Should
it instead just be a separate modifier that's just ignored on the non-mac
ports? e.g.
BUG_FOO CPU CG : ...
BUG_FOO SKIA : ....

> Tools/Scripts/webkitpy/layout_tests/port/base.py:390
> +	       path = self._filesystem.join(platform_dir, baseline_filename)
> +	   else:
> +	       path = self._filesystem.join(self.layout_tests_dir(),
baseline_filename)
> +	   return path

Why change this? The old code was fine. In fact, webkit generally prefers early
returns.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:958
> +	   """Tells this platform whether gpu-accelerated graphics are
enabled."""

Here and below: WebKit generally doesn't have comments that tell *what* the
code is doing. Comments should be for cases where you need extra explanation
for *why* the code is doing it.

Some of the existing code in this file doesn't follow that rule, but new code
we add should.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:962
> +    def _get_graphics_type_gpu_enabled(self):

WebKit doesn't use "get" prefixes for getters. So, this should just be
"_graphics_type_gpu_enabled".

These names seem unnecessarily verbose to me. How about:
_set_gpu_enabled
_gpu_enabled
_set_using_cg
_using_cg

> Tools/Scripts/webkitpy/layout_tests/port/base.py:983
> +    def _update_graphics_type_cached(self):
> +	   """Updates _graphics_type_cached based on other fields."""
> +	   if self._graphics_type_gpu_enabled:
> +	       graphics_type = 'gpu'
> +	   else:
> +	       graphics_type = 'cpu'
> +	   if self._graphics_type_using_cg:
> +	       graphics_type += '-cg'
> +	   self._graphics_type_cached = graphics_type

Caching this seems like premature optimization to me. Did this caching actually
make a difference in the overall time it takes to run all the tests? If not,
can you just move this logic into "graphics_type"? If so, can you give numbers?


Also, if we avoid the caching, then we can drop all these set/get methods and
just do what the old code was doing (set/get the appropriate properties
directly).

> LayoutTests/platform/chromium/test_expectations.txt:1878
> +BUGWK45991 CPU GPU : canvas/philip/tests/2d.shadow.canvas.basic.html = TEXT
> +BUGWK45991 CPU GPU : canvas/philip/tests/2d.shadow.canvas.transparent.2.html
= TEXT

Here and elsewhere, there's no point in writing "CPU GPU" since that's all the
graphics types, right? Can you just delete those modifiers?