If we move the shape info into the rare data construct, we can avoid creating and checking the map. This will also help with bug 108128, which relies on keeping shape info around for one more layout pass after the style has been changed.
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>