Bug 67324 – filter test_expectations properly for chromium-mac vs chromium-cg-mac

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

http://trac.webkit.org/changeset/94031 added support for chromium-mac (non-CoreGraphics) baseline images, but it did not provide for filtering of lines in test_expectations by this characteristic.

We need this filtering so that we can allow for the known differences between CG and Skia rendering.

Created attachment 105831 [details]
Patch

[WebKit Review Bot]

Attachment 105831 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

LayoutTests/ChangeLog:3:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

P.S. For now, we assume chromium-cg-mac when running LayoutTests on a mac.  To compare against the Skia expectations, pass in "--platform chromium-mac".

Created attachment 105954 [details]
Patch

Fixed check-webkit-style errors and attached new patch.

*** Bug 66987 has been marked as a duplicate of this bug. ***

Created attachment 106129 [details]
Patch

Comment on attachment 106129 [details]
Patch

Updated patch against tip-of-tree so that buildbots can apply the patch.  No other changes made.

[Adam Barth]

Looks plausible, but I'm going to let one of the experts on this code review your patch.

[Ojan Vafai]

Comment on attachment 106129 [details]
Patch

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?

(In reply to comment #10)
> 
> Also, please add tests. I'm pretty sure most of this code is already unittested. You can run the unittests by running test-webkitpy.

(Ignoring the lower-level stuff until we get this part sorted out...)

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

I agree that it would be simpler to allow separate SKIA and/or CG modifiers, rather than combining it with the CPU/GPU modifiers.

However, the decision had been made previously (in https://bugs.webkit.org/show_bug.cgi?id=66705 ) to organize it this way.  Please see the discussion there, including some clarifying questions I had asked that were never answered. 

Should we undo the decision that was made on that other bug?  Or do I misunderstand something?

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

In the current arrangement, "CPU GPU" means "Skia, not CG".  ("CPU-CG GPU-CG" would mean "CG, not Skia")

[Dirk Pranke]

Comment on attachment 106129 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106129&action=review

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

(In reply to comment #12)
> (From update of attachment 106129 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106129&action=review
> 
> > 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...

[Dirk Pranke]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 106129 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=106129&action=review
> > 
> > > 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.

[Ojan Vafai]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 106129 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=106129&action=review
> > 
> > > 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.

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.

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 106129 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=106129&action=review
> > > 
> > > > 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.
> 
> 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.

Created attachment 106591 [details]
Patch

Comment on attachment 106129 [details]
Patch

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.

[Dirk Pranke]

Looks good. Thanks for working on this!

[WebKit Review Bot]

Comment on attachment 106591 [details]
Patch

Rejecting attachment 106591 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 106591 [details]
Patch

Clearing flags on attachment: 106591

Committed r94700: <http://trac.webkit.org/changeset/94700>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.