[webkit-reviews] review denied: [Bug 52994] WebKit should support -webkit-tab-size. : [Attachment 84730] Took feedback and caught up to ToT

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 52994: WebKit should support -webkit-tab-size.
https://bugs.webkit.org/show_bug.cgi?id=52994

Attachment 84730: Took feedback and caught up to ToT
https://bugs.webkit.org/attachment.cgi?id=84730&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84730&action=review

> LayoutTests/fast/css/tab-size.html:8
> +    window.setTimeout(function() {

Why do we need setTimeout here?

> LayoutTests/fast/css/tab-size.html:19
> +<pre>	x</pre>

Sad that there is no named entity for tab... or is there?

> LayoutTests/fast/css/tab-size.html:44
> +<div style="-webkit-tab-size: 2;">
> +<pre>	x</pre>
> +<pre>		x</pre>
> +<pre>	x	x</pre>
> +<pre>x		x</pre>
> +<pre>x	x	x</pre>
> +<pre>xx	xx	x</pre>
> +<pre>xxx	xxx	x</pre>
> +</div>
> +<div id="dynamic">
> +<pre>	x</pre>
> +<pre>		x</pre>
> +<pre>	x	x</pre>
> +<pre>x		x</pre>
> +<pre>x	x	x</pre>
> +<pre>xx	xx	x</pre>
> +<pre>xxx	xxx	x</pre>
> +</div>

Seems like a lot of copy/paste in this test.  Can't we just generate more of
this from JS to make it easier to read?

I would also like us to test negative tab sizes.  What does the spec say for
those?

> LayoutTests/platform/mac/fast/css/tab-size-expected.txt:4
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600

Does this need to be a render dump test?

> Source/WebCore/css/CSSStyleSelector.cpp:5593
> +	   HANDLE_INHERIT_AND_INITIAL(tabSize, TabSize);
> +	   if (primitiveValue)
> +	       m_style->setTabSize(std::max(0, primitiveValue->getIntValue()));


Luke may have comment on if this is "modern" style.

> Source/WebCore/platform/graphics/TextRun.h:41
> +    TextRun(const UChar* c, int len, int tabSize = 0, float xpos = 0, float
expansion = 0, TrailingExpansionBehavior trailingExpansionBehavior =
AllowTrailingExpansion, bool rtl = false, bool directionalOverride = false)

Is 0 the right default?

> Source/WebCore/platform/graphics/TextRun.h:95
> +    bool allowTabs() const { return m_tabSize; }

This is a bit odd, honestly.  Should at least have a comment to explain why
we're using m_tabSize == 0 to mean "disallow tabs" (whatever that even means?)

> Source/WebCore/platform/graphics/TextRun.h:130
> +    int m_tabSize;

do we want to allow negative tab sizes?  If so, we should test them.

> Source/WebCore/rendering/RenderText.cpp:577
> +	   float tabWidth = allowTabs() ? monospaceCharacterWidth *
style()->tabSize() : 0;

Seems this "allowTabs" check isn't needed if we're using 0 to mean
"disallowed"?  Or I guess it is since we're checking against the style?

> Source/WebCore/rendering/RenderText.h:121
>      bool allowTabs() const { return !style()->collapseWhiteSpace(); }

Seems we should just rename allowTabs to be collapseWhiteSpace!  Since that
makes a lot more sense with tabSize == 0. Or at least collapseTabs.

> Source/WebCore/rendering/style/RenderStyle.cpp:413
> +	       || rareInheritedData->tabSize !=
other->rareInheritedData->tabSize)

Indent 4 spaces.

> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:55
> +    , tabSize(RenderStyle::initialTabSize())

Please use m_ for new members.