Summary: | Add support for the isolation property | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihai Tica <mitica> | ||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | allan.jensen, commit-queue, dino, d-r, esprehn+autocc, fmalita, glenn, gyuyoung.kim, kondapallykalyan, macpherson, menard, mihnea, pdr, schenney, sergio, simon.fraser, WebkitBugTracker | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 129661 | ||||||||||||||
Attachments: |
|
Description
Mihai Tica
2014-02-17 23:41:28 PST
Created attachment 224476 [details]
Patch V1
Created attachment 224479 [details]
Patch V2
Comment on attachment 224479 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=224479&action=review > Source/WebCore/css/CSSPrimitiveValueMappings.h:4158 > +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(Isolation i) Short variable names like "i" are not consistent with WebKit style. We tend to prefer e.g like "isolation". > Source/WebCore/css/CSSPrimitiveValueMappings.h:4169 > + switch (i) { > + case IsolationAuto: > + m_value.valueID = CSSValueAuto; > + break; > + case IsolationIsolate: > + m_value.valueID = CSSValueIsolate; > + break; > + } We should have a default case here with ASSERT_NOT_REACHED(), just like we do when converting in the other direction. > Source/WebCore/rendering/style/RenderStyle.h:974 > + void setIsolation(Isolation v) { rareNonInheritedData.access()->m_isolation = v; } Using rareNonInheritedData.access() like this will force the RenderStyle to create its own unique copy of the StyleRareNonInheritedData. If you look at other setters in this file, they use a macro (SET_VAR) that first checks if the value being written is identical to the existing one, and avoids a copy in that case. Created attachment 224608 [details]
Patch V3
Addressed issues highlighted by Andreas Kling.
Comment on attachment 224608 [details] Patch V3 View in context: https://bugs.webkit.org/attachment.cgi?id=224608&action=review > Source/WebCore/rendering/style/RenderStyle.h:975 > + bool hasIsolation() const { return rareNonInheritedData->m_isolation != IsolationAuto; } Won't this crash if rareNonInheritedData hasn't been allocated? (In reply to comment #5) > (From update of attachment 224608 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224608&action=review > > > Source/WebCore/rendering/style/RenderStyle.h:975 > > + bool hasIsolation() const { return rareNonInheritedData->m_isolation != IsolationAuto; } > > Won't this crash if rareNonInheritedData hasn't been allocated? All of RenderStyle's constructors initialise a valid rareNonInheritedData member, so I don't think this will crash. Created attachment 225055 [details]
Patch V4
Add SVG implementation. Add additional tests for HTML and SVG. Please have another look.
Comment on attachment 225055 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=225055&action=review The patch looks good to me. Just two snippets. I give the other reviewers some time to review the patch as well before giving my r+. Maybe Simon has more comments. > Source/WebCore/rendering/style/RenderStyle.h:974 > + void setBlendMode(BlendMode v) { SET_VAR(rareNonInheritedData, m_effectiveBlendMode, v); } > bool hasBlendMode() const { return static_cast<BlendMode>(rareNonInheritedData->m_effectiveBlendMode) != BlendModeNormal; } > + > + Isolation isolation() const { return static_cast<Isolation>(rareNonInheritedData->m_isolation); } > + void setIsolation(Isolation v) { SET_VAR(rareNonInheritedData, m_isolation, v); } should still have longer names like blendMode and isolation instead of v. Regardless of the use elsewhere. Created attachment 225068 [details]
Patch V4. Addressed Dirk's comments
Comment on attachment 225068 [details]
Patch V4. Addressed Dirk's comments
LGTM. Since no further review comments came from Simon and kling: r=me
Comment on attachment 225068 [details] Patch V4. Addressed Dirk's comments Clearing flags on attachment: 225068 Committed r164795: <http://trac.webkit.org/changeset/164795> All reviewed patches have been landed. Closing bug. css3/compositing/isolation-isolate-blended-child.html test added in this patch fails every time on one of the bots. Filed bug 129468. |