[webkit-reviews] review granted: [Bug 9410] Support for CSS widows and orphans : [Attachment 178297] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dean Jackson <dino at apple.com>'s
request for review:
Bug 9410: Support for CSS widows and orphans
https://bugs.webkit.org/show_bug.cgi?id=9410

Attachment 178297: Patch
https://bugs.webkit.org/attachment.cgi?id=178297&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=178297&action=review


> Source/WebCore/css/CSSParser.cpp:2157
> +    case CSSPropertyOrphans: // <integer> | inherit | auto (We've added
support for auto for backwards compatibility)
> +    case CSSPropertyWidows: // <integer> | inherit | auto (Ditto)

Auto seems like a very strange way to say "do nothing" here. Is there some
better way to say "don't try to stop these"?

> Source/WebCore/rendering/RenderBlock.cpp:6778
> +void RenderBlock::setBreakAtLineToAvoidWidow(RootInlineBox* lineToBreak)

We should assert this is non-zero, since the function to clear is separate.

> Source/WebCore/rendering/RenderBlock.h:310
> +    bool shouldBreakAtLineToAvoidWidow() const { return m_rareData ?
m_rareData->m_shouldBreakAtLineToAvoidWidow : false; }

I like && for expressions like this instead of ? :, what do you think?

> Source/WebCore/rendering/RenderBlock.h:317
> +    void clearShouldBreakAtLineToAvoidWidow() const
> +    {
> +	   if (!m_rareData)
> +	       return;
> +	   m_rareData->m_shouldBreakAtLineToAvoidWidow = false;
> +	   m_rareData->m_lineBreakToAvoidWidow = 0;
> +    }

Even if you want this to be inline, I think it would be better to put this
outside the class definition in the header, to keep things tighter. But also,
does this really need to be inline?