[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





--- Comment #18 from epoger at google.com  2011-09-07 09:32:31 PST ---
(From update of attachment 106129)
View in context: https://bugs.webkit.org/attachment.cgi?id=106129&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:390
>> +        return path
> 
> Why change this? The old code was fine. In fact, webkit generally prefers early returns.

I reverted much of the original patch, including this part.

>>>>>> Tools/Scripts/webkitpy/layout_tests/port/base.py:596
>>>>>> +        if self._graphics_type_cached is None:
>>>>> 
>>>>> None of the cpu/cpu-cg stuff belongs in this file. You should push all of this into chromium_mac.py and just override graphics_type() there.
>>>> 
>>>> I think these implementation details are very dependent on the resolution to Ojan's higher-level concerns.
>>>>  Dirk, do you have any thoughts on those concerns?
>>>> 
>>>> Thanks...
>>> 
>>> Ojan is right in theory that CG/Skia is orthogonal to CPU/GPU, and so it could be another modifier. In practice, though, I'm not sure that it matters that much, and in theory this is a short-lived transition, so I'd probably stick with the existing plan and expand the graphics_type to the four-modifier list. It's true that other ports could ignore the CG/Skia flag, but other (non-Chromium) ports don't even have the cpu/gpu concept.
>> 
>> Sorry, I hadn't seen the discussion on that bug. I'm fine with the high-level approach. It's weird to me, but hopefully we'll be getting rid of CG entirely in the near future and then we can simplify the code again, so it's fine. The bit that confused me was that I didn't realize CPU/GPU implied Skia.
> 
> OK, I'll stick with the current high-level approach but try to address the lower-level concerns.  I'll update the bug with a new patch, probably on Tuesday morning, when it's ready for further review.
> 
> Thanks.

I have moved all of the changes into chromium_mac and chromium_gpu.

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

Reverted.

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

Reverted.

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:983
>> +        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).

Reverted / handled elsewhere.

>> LayoutTests/platform/chromium/test_expectations.txt:1878
>> +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?

As discussed elsewhere, "CPU GPU" means "all graphics types EXCEPT Core Graphics".

The test_expectations diffs have changed from the original patch; I have confirmed that all results pass for chromium-cg-mac and chromium-cg-gpu-mac.

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