Bug 110995

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.