[chromium] factor out the optimization pass in CCRenderSurfaceFilters::apply
Created attachment 154830 [details] Patch
Let the bikeshedding begin.
Comment on attachment 154830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154830&action=review > Source/Platform/chromium/src/WebFilterOperation.cpp:42 > + && m_dropShadowOffset == other.m_dropShadowOffset > + && m_dropShadowColor == other.m_dropShadowColor; quick readthru style nit: indent only 4 spaces > Source/WebKit/chromium/tests/CCRenderSurfaceFiltersTest.cpp:2 > + * Copyright (C) 2011 Google Inc. All rights reserved. 2012
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 154830 [details] Patch Attachment 154830 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13368365 New failing tests: CCRenderSurfaceFiltersTest.testColorMatrixFiltersCombined
Created attachment 154898 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 154830 [details] Patch Attachment 154830 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13379270 New failing tests: CCRenderSurfaceFiltersTest.testColorMatrixFiltersCombined
Created attachment 154904 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 155059 [details] Patch
(In reply to comment #3) > (From update of attachment 154830 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154830&action=review > > > Source/Platform/chromium/src/WebFilterOperation.cpp:42 > > + && m_dropShadowOffset == other.m_dropShadowOffset > > + && m_dropShadowColor == other.m_dropShadowColor; > > quick readthru style nit: indent only 4 spaces Done. > > > Source/WebKit/chromium/tests/CCRenderSurfaceFiltersTest.cpp:2 > > + * Copyright (C) 2011 Google Inc. All rights reserved. > > 2012 Done. Note: the failing unittests are because of https://bugs.webkit.org/show_bug.cgi?id=92452 not having landed yet.
Comment on attachment 155059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155059&action=review > Source/Platform/chromium/public/WebFilterOperation.h:94 > + float m_matrix[20]; This should probably be SkScalar (and elsewhere). > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:275 > + int scratchCount = std::min(2, filterCount); Why the min? Shouldn't a filter chain of length 1 only need a single buffer? > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:394 > + state.init(optimizedFilters.size()); I think you should be checking the return value here and doing an early-out on failure.
(In reply to comment #11) > (From update of attachment 155059 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155059&action=review > > > Source/Platform/chromium/public/WebFilterOperation.h:94 > > + float m_matrix[20]; > > This should probably be SkScalar (and elsewhere). > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:275 > > + int scratchCount = std::min(2, filterCount); > > Why the min? Shouldn't a filter chain of length 1 only need a single buffer? Ignore that. I have Monday brain. > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:394 > > + state.init(optimizedFilters.size()); > > I think you should be checking the return value here and doing an early-out on failure.
Created attachment 155429 [details] Patch
(In reply to comment #11) > (From update of attachment 155059 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155059&action=review > > > Source/Platform/chromium/public/WebFilterOperation.h:94 > > + float m_matrix[20]; > > This should probably be SkScalar (and elsewhere). Done. > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:275 > > + int scratchCount = std::min(2, filterCount); > > Why the min? Shouldn't a filter chain of length 1 only need a single buffer? > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:394 > > + state.init(optimizedFilters.size()); > > I think you should be checking the return value here and doing an early-out on failure. Done - that's what I meant, I just forgot to write the code :/
Ping? James & all, any comments about the WebFilterOperation API addition?
Seems fine - IIUC Stephen White wants to rewrite this soon anyway, so it may all go away.
Comment on attachment 155429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155429&action=review > Source/Platform/Platform.gyp/Platform.gyp:39 > + # Location of the chromium src directory and target type is different > + # if webkit is built inside chromium or as standalone project. skia should be available at <(DEPTH)/skia/... no matter what sort of checkout we're in. In a full chromium checkout, <(DEPTH) is src/ and skia is at src/skia/.... In a WebKit standalone, <(DEPTH) is Source/WebKit/chromium and skia/ is pulled in to Source/WebKit/chromium/skia/...
Created attachment 155917 [details] Patch
(In reply to comment #17) > (From update of attachment 155429 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155429&action=review > > > Source/Platform/Platform.gyp/Platform.gyp:39 > > + # Location of the chromium src directory and target type is different > > + # if webkit is built inside chromium or as standalone project. > > skia should be available at <(DEPTH)/skia/... no matter what sort of checkout we're in. In a full chromium checkout, <(DEPTH) is src/ and skia is at src/skia/.... In a WebKit standalone, <(DEPTH) is Source/WebKit/chromium and skia/ is pulled in to Source/WebKit/chromium/skia/... Ok, done.
Comment on attachment 155917 [details] Patch R=me
Comment on attachment 155917 [details] Patch Clearing flags on attachment: 155917 Committed r124392: <http://trac.webkit.org/changeset/124392>
All reviewed patches have been landed. Closing bug.