Summary: | [chromium] factor out the optimization pass in CCRenderSurfaceFilters::apply | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Labour <piman> | ||||||||||||||
Component: | New Bugs | Assignee: | Antoine Labour <piman> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, cc-bugs, danakj, dglazkov, enne, fishd, jamesr, nduca, senorblanco, tkent+wkapi, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 92452 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Antoine Labour
2012-07-26 21:16:20 PDT
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. |