[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 bugzilla-daemon at webkit.org
Fri Sep 2 11:31:42 PDT 2011

Ojan Vafai <ojan at chromium.org> has denied epoger at google.com's request for
Bug 67324: filter test_expectations properly for chromium-mac vs

Attachment 106129: Patch

------- 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

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.

> 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(),
> +	   return path

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

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

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

These names seem unnecessarily verbose to me. How about:

> 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

> 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

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?

More information about the webkit-reviews mailing list