Bug 103906 - [chromium] Turn on the new Skia mask blur algorithm.
Summary: [chromium] Turn on the new Skia mask blur algorithm.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-03 09:33 PST by Stephen White
Modified: 2012-12-04 17:19 PST (History)
4 users (show)

See Also:


Attachments
Patch (6.34 KB, patch)
2012-12-03 09:34 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (11.86 KB, patch)
2012-12-04 14:36 PST, Stephen White
no flags Details | Formatted Diff | Diff
Patch (12.11 KB, patch)
2012-12-04 15:12 PST, Stephen White
enne: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2012-12-03 09:33:44 PST
[chromium] Turn on the new Skia mask blur algorithm.
Comment 1 Stephen White 2012-12-03 09:34:15 PST
Created attachment 177273 [details]
Patch
Comment 2 Adrienne Walker 2012-12-03 11:01:31 PST
Comment on attachment 177273 [details]
Patch

R=me.
Comment 3 WebKit Review Bot 2012-12-03 11:14:33 PST
Comment on attachment 177273 [details]
Patch

Clearing flags on attachment: 177273

Committed r136420: <http://trac.webkit.org/changeset/136420>
Comment 4 WebKit Review Bot 2012-12-03 11:14:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Stephen Chenney 2012-12-04 06:53:32 PST
I'm going to revert. The Skia suppression flags should not be removed until the actual rebaselines are ready. The whole point of the flag is to avoid entries in TestExpectations, because such entries hide unexpected reversions due to other patches.
Comment 6 Stephen White 2012-12-04 06:55:36 PST
(In reply to comment #5)
> I'm going to revert. The Skia suppression flags should not be removed until the actual rebaselines are ready. The whole point of the flag is to avoid entries in TestExpectations, because such entries hide unexpected reversions due to other patches.

Most of it has been reverted already.

I don't think it's fair to turn the webkit canaries red unnecessarily, when it can be avoided.  If the tests are going to fail when you turn on a flag, it's going to mask other failures anyway.  How does letting them go red help?
Comment 7 Stephen Chenney 2012-12-04 07:17:10 PST
(In reply to comment #6)
> (In reply to comment #5)
> > I'm going to revert. The Skia suppression flags should not be removed until the actual rebaselines are ready. The whole point of the flag is to avoid entries in TestExpectations, because such entries hide unexpected reversions due to other patches.
> 
> Most of it has been reverted already.
> 
> I don't think it's fair to turn the webkit canaries red unnecessarily, when it can be avoided.  If the tests are going to fail when you turn on a flag, it's going to mask other failures anyway.  How does letting them go red help?

It does not turn the WebKit canaries red, at least not unavoidably so. Read this: https://sites.google.com/site/skiadocs/developer-documentation/managing-chrome-s-use-of-skia/how-to-land-skia-changes-that-change-a-large-number-of-webkit-layout-test-results

In particular, you should have waited until Skia had rolled into Chromium, thus removing the Chromium-side skia.gyp suppression. Then waited until Chromium rolled into WebKit. The way you did it left the flag enabled, because skia.gyp was still defining it in WebKit.
Comment 8 Stephen Chenney 2012-12-04 07:20:49 PST
Committed r136508: <http://trac.webkit.org/changeset/136508>
Comment 9 Stephen Chenney 2012-12-04 07:23:35 PST
Now, if you would like to rebaseline, wait until skia.gyp in Chromium has been updated to remove the flag. Coordinate with Rob Philips on that. Then wait for a Chromium DEPS roll into WebKit (or request one yourself). Only when skia/skia.gyp no longer contains the flag can you remove it from skia_webkit.gyp and rebaseline.
Comment 10 Stephen White 2012-12-04 07:45:33 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > I'm going to revert. The Skia suppression flags should not be removed until the actual rebaselines are ready. The whole point of the flag is to avoid entries in TestExpectations, because such entries hide unexpected reversions due to other patches.
> > 
> > Most of it has been reverted already.
> > 
> > I don't think it's fair to turn the webkit canaries red unnecessarily, when it can be avoided.  If the tests are going to fail when you turn on a flag, it's going to mask other failures anyway.  How does letting them go red help?
> 
> It does not turn the WebKit canaries red, at least not unavoidably so. Read this: https://sites.google.com/site/skiadocs/developer-documentation/managing-chrome-s-use-of-skia/how-to-land-skia-changes-that-change-a-large-number-of-webkit-layout-test-results

From that doc:

"You will be making the Chromium.WebKit tree very red for an extended period, and the gardener needs to know that they are not expected to fix it."

This is what I was trying to avoid.
Comment 11 Stephen White 2012-12-04 14:36:10 PST
Created attachment 177567 [details]
Patch
Comment 12 Stephen Chenney 2012-12-04 15:07:16 PST
Comment on attachment 177567 [details]
Patch

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

> LayoutTests/platform/chromium/TestExpectations:3769
> +Bug(senorblanco) [ Win Mac Linux ] compositing/culling/scrolled-within-boxshadow.html [ ImageOnlyFailure ]

I'm not real happy about Bug(senorblanco). Why not webkit.org/b/103906, as presumably this will be the right place to track the baselines?
Comment 13 Stephen White 2012-12-04 15:12:06 PST
Created attachment 177582 [details]
Patch
Comment 14 Stephen White 2012-12-04 15:12:23 PST
(In reply to comment #12)
> (From update of attachment 177567 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177567&action=review
> 
> > LayoutTests/platform/chromium/TestExpectations:3769
> > +Bug(senorblanco) [ Win Mac Linux ] compositing/culling/scrolled-within-boxshadow.html [ ImageOnlyFailure ]
> 
> I'm not real happy about Bug(senorblanco). Why not webkit.org/b/103906, as presumably this will be the right place to track the baselines?

Done.
Comment 15 Stephen Chenney 2012-12-04 15:18:15 PST
Comment on attachment 177582 [details]
Patch

Now I would R+, except I do not have the power. enne?
Comment 16 Adrienne Walker 2012-12-04 16:23:11 PST
Comment on attachment 177582 [details]
Patch

Rs=me.
Comment 17 Stephen White 2012-12-04 17:19:44 PST
Committed r136600: <http://trac.webkit.org/changeset/136600>