Summary: | [chromium] Turn on the new Skia mask blur algorithm. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephen White <senorblanco> | ||||||||
Component: | New Bugs | Assignee: | 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
Stephen White
2012-12-03 09:33:44 PST
Created attachment 177273 [details]
Patch
Comment on attachment 177273 [details]
Patch
R=me.
Comment on attachment 177273 [details] Patch Clearing flags on attachment: 177273 Committed r136420: <http://trac.webkit.org/changeset/136420> All reviewed patches have been landed. Closing bug. 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. (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? (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. Committed r136508: <http://trac.webkit.org/changeset/136508> 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. (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. Created attachment 177567 [details]
Patch
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? Created attachment 177582 [details]
Patch
(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 on attachment 177582 [details]
Patch
Now I would R+, except I do not have the power. enne?
Comment on attachment 177582 [details]
Patch
Rs=me.
Committed r136600: <http://trac.webkit.org/changeset/136600> |