Summary: | [CSS Shaders] Add blend mode and composite op to compiled program cache key | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Max Vujovic <mvujovic> | ||||||||
Component: | Layout and Rendering | Assignee: | Max Vujovic <mvujovic> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achicu, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 85086 | ||||||||||
Attachments: |
|
Description
Max Vujovic
2012-08-09 09:50:35 PDT
Created attachment 157477 [details]
Patch
Alex, could you take a look at this patch? Especially the hash computation in CustomFilterProgramInfo::hash(). I was debating between a bunch of ways we could do the hash- I want your opinion on it.
Comment on attachment 157477 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157477&action=review Looks good. Just a few comments. > Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:43 > + , compositeOperator(CompositeSourceOver) nit: Identation here. > Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:49 > + return enabled == o.enabled I think enabled == o.enabled && enabled == false should ignore the other two properties. Right? > Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:57 > +} CustomFilterProgramMixSettings; Why do you need this instance? > Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:72 > +CustomFilterProgramInfo::CustomFilterProgramInfo(const String& vertexShader, const String& fragmentShader, CustomFilterProgramMixSettings mixSettings) I would pass a const ref here. > Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:86 > + unsigned hash = hashPossiblyNullString(m_vertexShaderString); This would look better if you encapsulated it into a separate class: HashBuilder hash; hash.append(m_vertexShaderString); hash.append(m_fragmentShaderString); ... hash.toUnsigned(); > Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.h:46 > + CustomFilterProgramInfo(const String&, const String&, CustomFilterProgramMixSettings); const& in here. Created attachment 157583 [details]
Patch
Updated patch based on Alex's comments.
Comment on attachment 157477 [details] Patch Thanks for the review, Alex. Responses inline: View in context: https://bugs.webkit.org/attachment.cgi?id=157477&action=review >> Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:43 >> + , compositeOperator(CompositeSourceOver) > > nit: Identation here. Thanks. Done. >> Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:49 >> + return enabled == o.enabled > > I think enabled == o.enabled && enabled == false should ignore the other two properties. Right? Yeah, that would be more robust. Done. >> Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:57 >> +} CustomFilterProgramMixSettings; > > Why do you need this instance? It's a typedef, but I removed it now. (It's old C habit of mine to typedef structs.) >> Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:72 >> +CustomFilterProgramInfo::CustomFilterProgramInfo(const String& vertexShader, const String& fragmentShader, CustomFilterProgramMixSettings mixSettings) > > I would pass a const ref here. Done. >> Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:86 >> + unsigned hash = hashPossiblyNullString(m_vertexShaderString); > > This would look better if you encapsulated it into a separate class: > > HashBuilder hash; > hash.append(m_vertexShaderString); > hash.append(m_fragmentShaderString); > ... > > hash.toUnsigned(); I found something similar to a HashBuilder in StringHasher::hashMemory. I use that instead now, so it should look a lot better. >> Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.h:46 >> + CustomFilterProgramInfo(const String&, const String&, CustomFilterProgramMixSettings); > > const& in here. Done. Comment on attachment 157583 [details]
Patch
Bots are green. Setting r?, cq?.
Comment on attachment 157583 [details]
Patch
Do you need to create a new header for this struct? Can't you put it into CustomFilterProgramInfo.h? Other than that the patch looks fine.
Created attachment 157779 [details] Patch (In reply to comment #6) > (From update of attachment 157583 [details]) > Do you need to create a new header for this struct? Can't you put it into CustomFilterProgramInfo.h? Other than that the patch looks fine. It doesn't need a new header file. I've updated the patch and moved it to CustomFilterProgramInfo.h. I've also made one other change since the last patch. I've added m_mixSettings.enabled to the hash because I realized the following (a) and (b) were hashing to the same value without it: (a) m_mixSettings.enabled=1 m_mixSettings.blendMode=0 m_mixSettings.compositeOperator=0 (b) m_mixSettings.enabled=0 m_mixSettings.blendMode=anything m_mixSettings.compositeOperator=anything Comment on attachment 157779 [details]
Patch
LGTM. r=me.
Comment on attachment 157779 [details] Patch Clearing flags on attachment: 157779 Committed r125331: <http://trac.webkit.org/changeset/125331> All reviewed patches have been landed. Closing bug. |