Summary: | [CSS Shaders] Implement a StyleCustomFilterProgram cache | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexandru Chiculita <achicu> | ||||||||
Component: | CSS | Assignee: | Alexandru Chiculita <achicu> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | allan.jensen, cmarcelo, dino, eric, esprehn+autocc, luiz, macpherson, menard, mvujovic, noam, ojan.autocc, rego+ews, webkit-ews, webkit.review.bot, xan.lopez | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 112844 | ||||||||||
Attachments: |
|
Description
Alexandru Chiculita
2013-03-18 11:33:21 PDT
Created attachment 194130 [details]
Patch V1
Comment on attachment 194130 [details] Patch V1 Attachment 194130 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17202544 Comment on attachment 194130 [details] Patch V1 Attachment 194130 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17202550 Created attachment 194135 [details]
Patch V2
Comment on attachment 194135 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=194135&action=review > Source/WebCore/ChangeLog:27 > + using the pointer value and not the values. However, that will always invalidate the "fitler" property because typo fitler > Source/WebCore/ChangeLog:49 > + (WebCore): Remove (there are a few of these) > Source/WebCore/ChangeLog:52 > + if no program is found. loadPendingShaders is responsible with adding the program in the cache if it is actually going to be used. s/with/for/ > Source/WebCore/css/StyleResolver.cpp:3941 > + if (value->isWebKitCSSShaderValue()) > + return static_cast<WebKitCSSShaderValue*>(value); We should probably add a toWebKitCSSShaderValue() method, the way other classes have done. > Source/WebCore/css/StyleResolver.cpp:3996 > + WebKitCSSShaderValue* shaderValue = static_cast<StylePendingShader*>(program->vertexShader())->cssShaderValue(); And here for toStylePendingShader, although that could be in a separate patch (where we fix all occurrences) > Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:81 > + // CustomFilterPrograms are unique combinations of shaders and can be > + // compared using just the pointer value instead. > + bool operator==(const CustomFilterProgram&) const; > + bool operator!=(const CustomFilterProgram&) const; I suggest mentioning here that you're trying to catch people comparing directly (the way toWhatever function comments mention catching people doing unnecessary casting). > Source/WebCore/rendering/style/StyleCustomFilterProgram.cpp:41 > +namespace WebCore { > + > + > +StyleCustomFilterProgram::~StyleCustomFilterProgram() Nit: extra space > Source/WebCore/rendering/style/StyleCustomFilterProgramCache.cpp:76 > + ASSERT(m_cache.find(key) != m_cache.end()); Is this necessary? (In reply to comment #5) > (From update of attachment 194135 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194135&action=review > Thanks, that was quick! > > Source/WebCore/ChangeLog:27 > > + using the pointer value and not the values. However, that will always invalidate the "fitler" property because > > typo fitler I always do that. Thanks. :) > > > Source/WebCore/ChangeLog:49 > > + (WebCore): > > Remove (there are a few of these) > That's generated automatically by the script. Should I always remove it? Should I log a bug for the script? > > Source/WebCore/ChangeLog:52 > > + if no program is found. loadPendingShaders is responsible with adding the program in the cache if it is actually going to be used. > > s/with/for/ > Ok. > > Source/WebCore/css/StyleResolver.cpp:3941 > > + if (value->isWebKitCSSShaderValue()) > > + return static_cast<WebKitCSSShaderValue*>(value); > > We should probably add a toWebKitCSSShaderValue() method, the way other classes have done. > > > Source/WebCore/css/StyleResolver.cpp:3996 > > + WebKitCSSShaderValue* shaderValue = static_cast<StylePendingShader*>(program->vertexShader())->cssShaderValue(); > > And here for toStylePendingShader, although that could be in a separate patch (where we fix all occurrences) > > > Source/WebCore/platform/graphics/filters/CustomFilterProgram.h:81 > > + // CustomFilterPrograms are unique combinations of shaders and can be > > + // compared using just the pointer value instead. > > + bool operator==(const CustomFilterProgram&) const; > > + bool operator!=(const CustomFilterProgram&) const; > > I suggest mentioning here that you're trying to catch people comparing directly (the way toWhatever function comments mention catching people doing unnecessary casting). > Ok. > > Source/WebCore/rendering/style/StyleCustomFilterProgram.cpp:41 > > +namespace WebCore { > > + > > + > > +StyleCustomFilterProgram::~StyleCustomFilterProgram() > > Nit: extra space > > > Source/WebCore/rendering/style/StyleCustomFilterProgramCache.cpp:76 > > + ASSERT(m_cache.find(key) != m_cache.end()); > > Is this necessary? Not really. I was using it for debugging and figured it might be good to have. It should assert that the hash of the CustomFitlerProgramInfo works correctly. I will remove it. Comment on attachment 194135 [details]
Patch V2
cq- as I need to include the feedback.
Created attachment 194351 [details]
Patch for commit
Comment on attachment 194351 [details] Patch for commit Clearing flags on attachment: 194351 Committed r146529: <http://trac.webkit.org/changeset/146529> All reviewed patches have been landed. Closing bug. |