Bug 103906

Summary: [chromium] Turn on the new Skia mask blur algorithm.
Product: WebKit Reporter: Stephen White <senorblanco>
Component: New BugsAssignee: Stephen White <senorblanco>
Status: RESOLVED FIXED    
Severity: Normal CC: enne, schenney, webkit.review.bot, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch enne: review+

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>