Bug 103906 – [chromium] Turn on the new Skia mask blur algorithm.

• Component: New Bugs
• Status: RESOLVED FIXED

[Stephen White]

[chromium] Turn on the new Skia mask blur algorithm.

[Stephen White]

Created attachment 177273 [details]
Patch

[Adrienne Walker]

Comment on attachment 177273 [details]
Patch

R=me.

[WebKit Review Bot]

Comment on attachment 177273 [details]
Patch

Clearing flags on attachment: 177273

Committed r136420: <http://trac.webkit.org/changeset/136420>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen Chenney]

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.

[Stephen White]

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

[Stephen Chenney]

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

[Stephen Chenney]

Committed r136508: <http://trac.webkit.org/changeset/136508>

[Stephen Chenney]

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.

[Stephen White]

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

[Stephen White]

Created attachment 177567 [details]
Patch

[Stephen Chenney]

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?

[Stephen White]

Created attachment 177582 [details]
Patch

[Stephen White]

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

[Stephen Chenney]

Comment on attachment 177582 [details]
Patch

Now I would R+, except I do not have the power. enne?

[Adrienne Walker]

Comment on attachment 177582 [details]
Patch

Rs=me.

[Stephen White]

Committed r136600: <http://trac.webkit.org/changeset/136600>