[Webkit-unassigned] [Bug 67324] filter test_expectations properly for chromium-mac vs chromium-cg-mac

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67324


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #106129|review?                     |review-
               Flag|                            |




--- Comment #10 from Ojan Vafai <ojan at chromium.org>  2011-09-02 11:31:42 PST ---
(From update of attachment 106129)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.