Bug 110995 – [css exclusions] Move ExclusionShapeInsideInfo into RenderBlockRareData

• Component: CSS
• Status: RESOLVED FIXED

[Bear Travis]

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.

[Bear Travis]

Created attachment 190560 [details]
Initial Patch

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Bear Travis]

Created attachment 190565 [details]
Update

Wrapping some code in compiler guards for the non-enabled case.

[Early Warning System Bot]

Comment on attachment 190565 [details]
Update

Attachment 190565 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16761077

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 190565 [details]
Update

Attachment 190565 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/16753075

[Bear Travis]

Created attachment 190570 [details]
Update

Removing unnecessary initialization in RenderBlockRareData.

[Early Warning System Bot]

Comment on attachment 190570 [details]
Update

Attachment 190570 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16776589

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Bear Travis]

Created attachment 190614 [details]
Removing 0 for null pointer

[Julien Chaffraix]

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?

[Bear Travis]

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.

[Julien Chaffraix]

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));

[Bear Travis]

Created attachment 191062 [details]
Making final adjustments

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[EFL EWS Bot]

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

[WebKit Review Bot]

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

[Bear Travis]

Created attachment 191069 [details]
Changing 0 to nullptr

[WebKit Review Bot]

Comment on attachment 191069 [details]
Changing 0 to nullptr

Clearing flags on attachment: 191069

Committed r144520: <http://trac.webkit.org/changeset/144520>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Re-opened since this is blocked by bug 111261

[Ryosuke Niwa]

 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>

[Bear Travis]

Created attachment 192593 [details]
Making RenderBlockRareData public

The compiler for cr-win requires a public struct definition in order to properly instantiate OwnPtr<RenderBlockRareData>.

[Bear Travis]

Comment on attachment 192593 [details]
Making RenderBlockRareData public

Moving back to the commit queue. Should fix the cr-win build failure.

[WebKit Review Bot]

Comment on attachment 192593 [details]
Making RenderBlockRareData public

Clearing flags on attachment: 192593

Committed r145610: <http://trac.webkit.org/changeset/145610>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.