[webkit-reviews] review canceled: [Bug 89259] [CSS Exclusions] Enable shape-inside for simple rectangles : [Attachment 159482] Fixing nullptr error on destruction

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Bear Travis
<betravis at adobe.com>'s request for review:
Bug 89259: [CSS Exclusions] Enable shape-inside for simple rectangles
https://bugs.webkit.org/show_bug.cgi?id=89259

Attachment 159482: Fixing nullptr error on destruction
https://bugs.webkit.org/attachment.cgi?id=159482&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=159482&action=review


Ooops review collision, let's have another round to cover Daniel's comments :)

>> Source/WebCore/rendering/WrapShapeInfo.cpp:56
>> +	, m_lineTop(0)
> 
> It's unnecessary to initialize these instance variables with 0 as the default
constructor for FractionalLayoutUnit (*) defaults the value of new instances to
0.
> 
> (*) LayoutUnit is defined to be an alternative name for FractionalLayoutUnit
by
<http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/LayoutTypes.h?re
v=125167#L49>.

Good catch Daniel, the definition of LayoutUnit actually depends if you have
enabled subpixel layout: it's FractionalLayoutUnit if you do, int if you
didn't.

The preferred way is to initialize a LayoutUnit to "0" is ZERO_LAYOUT_UNIT and
not 0.

>> Source/WebCore/rendering/WrapShapeInfo.cpp:65
>> +WrapShapeInfo*
WrapShapeInfo::ensureWrapShapeInfoForRenderBlock(RenderBlock* block)
> 
> Maybe "add", as in WrapShapeInfo::add(RenderBlock*), would be a more
descriptive name for this function? This would make the name and functionality
of this function consistent with other functions named add, including
HashMap::add() (which is called below).

We have a pattern of ensure* method in the dom/, this is just extending it
here. I don't think add makes any sense as it's an implementation detail.

> Source/WebCore/rendering/WrapShapeInfo.h:86
> +    LayoutUnit m_logicalHeight;

Maybe we could get away with using floats here to match the line box units and
not the RenderObject layout.