[Webkit-unassigned] [Bug 52994] WebKit should support -webkit-tab-size.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=52994


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #84730|review?                     |review-
               Flag|                            |




--- Comment #14 from Eric Seidel <eric at webkit.org>  2011-05-23 14:40:34 PST ---
(From update of attachment 84730)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.