Summary: | [css exclusions] Move ExclusionShapeInsideInfo into RenderBlockRareData | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bear Travis <betravis> | ||||||||||||||||||
Component: | CSS | Assignee: | Bear Travis <betravis> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | abarth, bjonesbe, dglazkov, eric, esprehn+autocc, jchaffraix, ojan.autocc, philn, rego+ews, rniwa, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 111261 | ||||||||||||||||||||
Bug Blocks: | 89256, 108128, 111029 | ||||||||||||||||||||
Attachments: |
|
Description
Bear Travis
2013-02-27 11:10:18 PST
Created attachment 190560 [details]
Initial Patch
Comment on attachment 190560 [details] Initial Patch Attachment 190560 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16833090 Comment on attachment 190560 [details] Initial Patch Attachment 190560 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16675197 Created attachment 190565 [details]
Update
Wrapping some code in compiler guards for the non-enabled case.
Comment on attachment 190565 [details] Update Attachment 190565 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16761077 Comment on attachment 190565 [details] Update Attachment 190565 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16783379 Comment on attachment 190565 [details] Update Attachment 190565 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/16753075 Created attachment 190570 [details]
Update
Removing unnecessary initialization in RenderBlockRareData.
Comment on attachment 190570 [details] Update Attachment 190570 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16776589 Comment on attachment 190570 [details] Update Attachment 190570 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16758172 Comment on attachment 190570 [details] Update Attachment 190570 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16753083 Comment on attachment 190570 [details] Update Attachment 190570 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16807301 Created attachment 190614 [details]
Removing 0 for null pointer
Comment on attachment 190614 [details] Removing 0 for null pointer View in context: https://bugs.webkit.org/attachment.cgi?id=190614&action=review The change is going in the right direction but I think we can do better. > Source/WebCore/rendering/RenderBlock.cpp:1394 > + return m_rareData && m_rareData->m_shapeInsideInfo && ExclusionShapeInsideInfo::isEnabledFor(this) ? static_cast<ExclusionShapeInsideInfo*>(m_rareData->m_shapeInsideInfo.get()) : 0; This is a downcast thus very nasty, especially if you don't check what you are doing casting. > Source/WebCore/rendering/RenderBlock.h:442 > ExclusionShapeInsideInfo* exclusionShapeInsideInfo() const; > + ExclusionShapeInsideInfo* rawExclusionShapeInsideInfo() const; Those 2 functions are extremely similar in name and functionality. It looks like the only place where the new function is used is updateExclusionShapeInsideInfoAfterStyleChange. How about using a pattern in the code where you have a function called: ensureExclusionShapeInsideInfo(); That would do the checks and create the required classes if needed. > Source/WebCore/rendering/RenderBlock.h:1232 > + OwnPtr<ExclusionShapeInfo<RenderBlock, &RenderStyle::resolvedShapeInside> > m_shapeInsideInfo; Couldn't you store an ExclusionShapeInsideInfo here to avoid the cast when getting the value back? Created attachment 190895 [details]
Incorporating feedback
Based on feedback from jchaffraix, I have moved to remove the static casting by getting rid of the cirular depencency on InlineIterator that made it necessary. I have also changed rawExclusionShapeInsideInfo to ensureExclusionShapeInsideInfo.
Comment on attachment 190895 [details] Incorporating feedback View in context: https://bugs.webkit.org/attachment.cgi?id=190895&action=review > Source/WebCore/rendering/ExclusionShapeInsideInfo.cpp:43 > + { } AFAICT this should be on 2 lines. Though I don't think it's a style violation, just the more usual way so it's probably a 'nit'. > Source/WebCore/rendering/ExclusionShapeInsideInfo.h:45 > +struct LineSegmentIterator { Not a super fan of the new Iterator to break a circular dependency but I couldn't think of something smarter. > Source/WebCore/rendering/ExclusionShapeInsideInfo.h:53 > + { } Same comment about this being on 2 lines here. > Source/WebCore/rendering/RenderBlock.cpp:1404 > - ExclusionShapeInsideInfo::removeInfo(this); > + setExclusionShapeInsideInfo(PassOwnPtr<ExclusionShapeInsideInfo>()); The way you instantiate the PassOwnPtr is weird. setExclusionShapeInsideInfo(0); should work (using the PassOwnPtr(T*) constructor). > Source/WebCore/rendering/RenderBlock.h:454 > + if (!m_rareData) > + m_rareData = adoptPtr(new RenderBlockRareData(this)); > + if (!m_rareData->m_shapeInsideInfo) > + m_rareData->m_shapeInsideInfo = ExclusionShapeInsideInfo::createInfo(this); if (!m_rareData || !m_rareData->m_shapeInsideInfo) setExclusionShapeInsideInfo(ExclusionShapeInsideInfo::createInfo(this)); Created attachment 191062 [details]
Making final adjustments
Comment on attachment 191062 [details] Making final adjustments Attachment 191062 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16796263 Comment on attachment 191062 [details] Making final adjustments Attachment 191062 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16687593 Comment on attachment 191062 [details] Making final adjustments Attachment 191062 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/16794285 Comment on attachment 191062 [details] Making final adjustments Attachment 191062 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16687597 Created attachment 191069 [details]
Changing 0 to nullptr
Comment on attachment 191069 [details] Changing 0 to nullptr Clearing flags on attachment: 191069 Committed r144520: <http://trac.webkit.org/changeset/144520> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 111261 Comment #2 From Adam Barth 2013-03-03 00:22:53 PST (-) [reply] 179>C:\b\build\slave\webkit-win-latest-rel\build\src\third_party\WebKit\Source\WTF\wtf/OwnPtrCommon.h(63):error C2248: 'WebCore::RenderBlock::RenderBlockRareData' : cannot access protected struct declared in class 'WebCore::RenderBlock' 179> c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webcore\rendering\RenderBlock.h(1214) : see declaration of 'WebCore::RenderBlock::RenderBlockRareData' 179> c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webcore\rendering\RenderBlock.h(83) : see declaration of 'WebCore::RenderBlock' 179> C:\b\build\slave\webkit-win-latest-rel\build\src\third_party\WebKit\Source\WTF\wtf/PassOwnPtr.h(56) : see reference to function template instantiation 'void WTF::deleteOwnedPtr<WebCore::RenderBlock::RenderBlockRareData>(T *)' being compiled 179> with 179> [ 179> T=WebCore::RenderBlock::RenderBlockRareData 179> ] 179> C:\b\build\slave\webkit-win-latest-rel\build\src\third_party\WebKit\Source\WTF\wtf/PassOwnPtr.h(56) : while compiling class template member function 'WTF::PassOwnPtr<T>::~PassOwnPtr(void)' 179> with 179> [ 179> T=WebCore::RenderBlock::RenderBlockRareData 179> ] 179> c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webcore\rendering\RenderBlock.h(336) : see reference to class template instantiation 'WTF::PassOwnPtr<T>' being compiled 179> with 179> [ 179> T=WebCore::RenderBlock::RenderBlockRareData 179> ] Comment #3 From Adam Barth 2013-03-03 00:23:19 PST (-) [reply] See. for example, http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Builder/builds/36652/steps/compile/logs/stdio#error1 Comment #4 From Adam Barth 2013-03-03 00:35:33 PST (-) [reply] (From update of attachment 191129 [details]) Clearing flags on attachment: 191129 Committed r144561: <http://trac.webkit.org/changeset/144561> Created attachment 192593 [details]
Making RenderBlockRareData public
The compiler for cr-win requires a public struct definition in order to properly instantiate OwnPtr<RenderBlockRareData>.
Comment on attachment 192593 [details]
Making RenderBlockRareData public
Moving back to the commit queue. Should fix the cr-win build failure.
Comment on attachment 192593 [details] Making RenderBlockRareData public Clearing flags on attachment: 192593 Committed r145610: <http://trac.webkit.org/changeset/145610> All reviewed patches have been landed. Closing bug. |