Comment on attachment 80152[details]
Patch
You forgot to patch RenderStyle::diff to return a Layout hint if this property changes. I'd like to see a test that dynamically changes the tab-size, since this was overlooked.

Comment on attachment 80152[details]
Patch
Also, don't you need auto to map back to the initial value? It seems like auto is doing nothing right now. Make a test where body has a tab size of 20 and then make a div inside that body set the tab size to auto. It should go back to 8, but with your code I bet it stays at 20.

Hyatt, Mitz, thank you for take a look!
I updated the patch for addressing your feedback.
Could you take another look?
> (From update of attachment 80152[details])
> You forgot to patch RenderStyle::diff to return a Layout hint if this property changes. I'd like to see a test that dynamically changes the tab-size, since this was overlooked.
Changed RenderStyle::diff() to handle tabSize and added a dynamic change case to the test.
I also removed "auto" value handling because it doesn't make sense to have "auto" for tab-size,
Mozilla also doesn't have that.

Comment on attachment 84730[details]
Took feedback and caught up to ToT
View in context: https://bugs.webkit.org/attachment.cgi?id=84730&action=review
I updated the patch to address Eric's point.
Thank you for taking a look this aged patch!
Tab-size is now a part of the standard and I think we are ready to add it.
>> LayoutTests/fast/css/tab-size.html:8
>> + window.setTimeout(function() {
>
> Why do we need setTimeout here?
I want to ensure to run this after the first full layout
because this tests dynamic property change.
>> LayoutTests/fast/css/tab-size.html:19
>> +<pre>&#9;x</pre>
>
> Sad that there is no named entity for tab... or is there?
Unfortunately, no. We could use actual tab character but it would be worse than the number-based entity.
>> LayoutTests/fast/css/tab-size.html:44
>> +</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?
That makes sense. Changed to use script for generating the cases.
>> LayoutTests/platform/mac/fast/css/tab-size-expected.txt:4
>> + RenderBlock {HTML} at (0,0) size 800x600
>
> Does this need to be a render dump test?
Yes. This change is all about layout,
and tab layout is not simple enough for giving a clear assertion() in the script.
>> Source/WebCore/css/CSSStyleSelector.cpp:5593
>> + m_style->setTabSize(std::max(0, primitiveValue->getIntValue()));
>
> Luke may have comment on if this is "modern" style.
Sure. now removed std::max() because the parser already rejected negative value before here.
>> 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?
Yes. that is equivalent to allowTabs = false, which was the original 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?)
I noticed that we no longer need this. I remove allowTabs().
>> Source/WebCore/platform/graphics/TextRun.h:130
>> + int m_tabSize;
>
> do we want to allow negative tab sizes? If so, we should test them.
Good catch. It's invalid. I changed this to unsigned.
I also added the test case for it.
>> 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?
Good point. I added RenderStyle::collapsedTabSize() for handling these cases.
>> 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.
I removed this since we don't need this anymore.
>> Source/WebCore/rendering/style/RenderStyle.cpp:413
>> + || rareInheritedData->tabSize != other->rareInheritedData->tabSize)
>
> Indent 4 spaces.
Fixed.
>> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:55
>> + , tabSize(RenderStyle::initialTabSize())
>
> Please use m_ for new members.
Sure. Added.

Created attachment 94966[details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #38)
> Should tab-size:0 be allowed?
Yes, according to the draft. Both <integer> and <length> can be (generally) zero, and there is nothing stating that zero is invalid in tab-size, just that negative values are not allowed. Therefore, it's safe to assume zero should be allowed.
I can even remotely imagine some use cases for it.

Created attachment 141129[details]
Archive of layout-test-results from ec2-cq-03
The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Attachment 141140[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/platform/graphics/Font.h:163: The parameter name "fontData" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 30 files
If any of these errors are false positives, please file a bug against check-webkit-style.