Bug 99439 – [CSS3] Parsing the property, text-align-last.

• Component: CSS
• Status: RESOLVED FIXED

[Dongwoo Joshua Im (dwim)]

This is the first step to support "text-align-last" property in WebKit.
Implement to parse the property, 
and test that with 'getComputedStyle'.

[Dongwoo Joshua Im (dwim)]

Created attachment 168912 [details]
Patch

[Build Bot]

Comment on attachment 168912 [details]
Patch

Attachment 168912 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14394084

[Dongwoo Joshua Im (dwim)]

Created attachment 168922 [details]
mac port build fix

[Build Bot]

Comment on attachment 168922 [details]
mac port build fix

Attachment 168922 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14387125

[WebKit Review Bot]

Comment on attachment 168922 [details]
mac port build fix

Attachment 168922 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14398204

New failing tests:
fast/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last.html

[WebKit Review Bot]

Comment on attachment 168922 [details]
mac port build fix

Attachment 168922 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14395219

New failing tests:
fast/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last.html

[Dongwoo Joshua Im (dwim)]

(In reply to comment #6)
> (From update of attachment 168922 [details])
> Attachment 168922 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14395219
> 
> New failing tests:
> fast/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last.html

Ok.. I think I need to skip this test as Missing.

[Dongwoo Joshua Im (dwim)]

Created attachment 169098 [details]
Patch

[Julien Chaffraix]

Comment on attachment 169098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169098&action=review

Please don't copy and paste code without looking at what it is doing.

> Source/JavaScriptCore/ChangeLog:8
> +        Regarding the CSS specification below, text-align-last property is needed to be implemented in WebKit.

Not a very helpful comment, anything mandated by the spec for compliance should be implemented.

> Source/WebCore/ChangeLog:13
> +        * Configurations/FeatureDefines.xcconfig: Add disabled CSS3_TEXT_ALIGN_LAST flag.

I really don't think another flag for a *single* feature is going to cut. I would rather see that you repurpose the existing CSS3_TEXT_DECORATION to be the umbrella flag for all CSS3 Text features.

> Source/WebCore/css/CSSParser.cpp:1864
> +        // left || right || center || justify || start || end || auto

Let's copy this from the spec:

// auto | start | end | left | right | center | justify

|| has a different meaning from |, see CSS 2.1.

> Source/WebCore/css/CSSParser.cpp:1866
> +            || value->unit == CSSPrimitiveValue::CSS_STRING)

I don't think this is right to allow CSS_STRING here: the specification doesn't mention it nor do I see special treatment for that value anywhere else.

> Source/WebCore/css/StyleBuilder.cpp:1144
> +        if (primitiveValue->getIdent() != CSSValueWebkitMatchParent)
> +            styleResolver->style()->setTextAlignLast(*primitiveValue);

This is obviously from text-align and the spec never says you should expect match-parent.

> Source/WebCore/rendering/style/RenderStyle.h:264
> +#if ENABLE(CSS3_TEXT_ALIGN_LAST)
> +        unsigned m_textAlignLast : 4; // ETextAlignLast
> +#endif // CSS3_TEXT_ALIGN_LAST

I don't think it's fine to bloat this structure with a new property (on top of that you should have updated the bit count below): we have a StyleRareInheritedData that handles properties we don't expect to see much in the wild.

> Source/WebCore/rendering/style/RenderStyleConstants.h:332
>  enum ETextAlign {
>      LEFT, RIGHT, CENTER, JUSTIFY, WEBKIT_LEFT, WEBKIT_RIGHT, WEBKIT_CENTER, TASTART, TAEND,
> +#if ENABLE(CSS3_TEXT_ALIGN_LAST)
> +    TAAUTO
> +#endif // CSS3_TEXT_ALIGN_LAST

I don't really like that. text-align and text-align-last have slightly different arguments, yet you piggy-back on this enum which makes it very likely that a programming mistake allows an error to slip through. On top of that, you get the legacy values straight into your new property (WEBKIT_LEFT, ...) which doesn't make much sense.

All in all, it's probably better to have a separate enum. We can always merge the logic later if we see that the need support for the other modes.

> LayoutTests/ChangeLog:11
> +        * fast/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last-expected.txt: Added.

It's not OK to land the bad baseline because the bots don't run with the feature enabled. Just skip the whole fast/css3-text-align-last directory on all platforms pending enabling the flag (you can open a bug for that or just point at the meta bug).

> LayoutTests/fast/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last.js:69
> +

This is an *inherited* property so I would expect a test for that. How about the extensions you have mistakenly allowed by copying and pasting from ' text-align'? You should now test that we *don't* support them.

> LayoutTests/platform/chromium/TestExpectations:201
> +# Chromium doesn't support the css3 text-align-last yet.
> +webkit.org/b/99439 fast/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last.html [ Missing ]

This entry doesn't make any sense: you have an expected file above. The test is failing because the feature is disabled on the EWS, it should be SKIPPED until enabled (on all platforms).

[Dongwoo Joshua Im (dwim)]

(In reply to comment #9)
> (From update of attachment 169098 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169098&action=review
> 
Thanks for the review, jchaffraix!

This is my first step into CSS world, so I don't really understand well yet.
So, I'm still study about CSS features and implementations.

Working for this patch will help me to understand little bit more about CSS, hopefully.


> Please don't copy and paste code without looking at what it is doing.
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Regarding the CSS specification below, text-align-last property is needed to be implemented in WebKit.
> 
> Not a very helpful comment, anything mandated by the spec for compliance should be implemented.
> 
Yes, I will.

> > Source/WebCore/ChangeLog:13
> > +        * Configurations/FeatureDefines.xcconfig: Add disabled CSS3_TEXT_ALIGN_LAST flag.
> 
> I really don't think another flag for a *single* feature is going to cut. I would rather see that you repurpose the existing CSS3_TEXT_DECORATION to be the umbrella flag for all CSS3 Text features.
> 
But, as you know, text decoration and text alignment are in different chapter in the spec, and it seems they are really different feature.

I don't think "CSS3_TEXT_DECORATION" can be a kind of comprehensive flag of text-align-last and text decoration.


How about this?

1. Let's make "CSS3_TEXT_ALIGNMENT", and we can implement 'text-align-last' and 'text-justify' under the new flag.

2. Or, Let's make "CSS3_TEXT", and let text-align-last and text decoration be in the flag.


What do you think?


> > Source/WebCore/css/CSSParser.cpp:1864
> > +        // left || right || center || justify || start || end || auto
> 
> Let's copy this from the spec:
> 
> // auto | start | end | left | right | center | justify
> 
> || has a different meaning from |, see CSS 2.1.
> 
Yes, I'll fix this.

> > Source/WebCore/css/CSSParser.cpp:1866
> > +            || value->unit == CSSPrimitiveValue::CSS_STRING)
> 
> I don't think this is right to allow CSS_STRING here: the specification doesn't mention it nor do I see special treatment for that value anywhere else.
> 
You are right.

> > Source/WebCore/css/StyleBuilder.cpp:1144
> > +        if (primitiveValue->getIdent() != CSSValueWebkitMatchParent)
> > +            styleResolver->style()->setTextAlignLast(*primitiveValue);
> 
> This is obviously from text-align and the spec never says you should expect match-parent.
> 
You are right.

> > Source/WebCore/rendering/style/RenderStyle.h:264
> > +#if ENABLE(CSS3_TEXT_ALIGN_LAST)
> > +        unsigned m_textAlignLast : 4; // ETextAlignLast
> > +#endif // CSS3_TEXT_ALIGN_LAST
> 
> I don't think it's fine to bloat this structure with a new property (on top of that you should have updated the bit count below): we have a StyleRareInheritedData that handles properties we don't expect to see much in the wild.
> 
Oh, ok. I'll move this into StyleRareInheritedData.
I expected this feature could be used "not rare", though. ;)

> > Source/WebCore/rendering/style/RenderStyleConstants.h:332
> >  enum ETextAlign {
> >      LEFT, RIGHT, CENTER, JUSTIFY, WEBKIT_LEFT, WEBKIT_RIGHT, WEBKIT_CENTER, TASTART, TAEND,
> > +#if ENABLE(CSS3_TEXT_ALIGN_LAST)
> > +    TAAUTO
> > +#endif // CSS3_TEXT_ALIGN_LAST
> 
> I don't really like that. text-align and text-align-last have slightly different arguments, yet you piggy-back on this enum which makes it very likely that a programming mistake allows an error to slip through. On top of that, you get the legacy values straight into your new property (WEBKIT_LEFT, ...) which doesn't make much sense.
> 
> All in all, it's probably better to have a separate enum. We can always merge the logic later if we see that the need support for the other modes.
> 
This is the one I've most worried about.
I will make another enum, then use TALLEFT, TALRIGHT, TALSTART and so on.

> > LayoutTests/ChangeLog:11
> > +        * fast/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last-expected.txt: Added.
> 
> It's not OK to land the bad baseline because the bots don't run with the feature enabled. Just skip the whole fast/css3-text-align-last directory on all platforms pending enabling the flag (you can open a bug for that or just point at the meta bug).
> 
Ok. I will create a meta bug, and try to support all the port, in near future. ;-)

> > LayoutTests/fast/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last.js:69
> > +
> 
> This is an *inherited* property so I would expect a test for that. How about the extensions you have mistakenly allowed by copying and pasting from ' text-align'? You should now test that we *don't* support them.
> 
I will try to add such test case.

> > LayoutTests/platform/chromium/TestExpectations:201
> > +# Chromium doesn't support the css3 text-align-last yet.
> > +webkit.org/b/99439 fast/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last.html [ Missing ]
> 
> This entry doesn't make any sense: you have an expected file above. The test is failing because the feature is disabled on the EWS, it should be SKIPPED until enabled (on all platforms).
> 
Ok. Skip & meta bug which we mentioned previously.

[Julien Chaffraix]

Comment on attachment 169098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169098&action=review

>>> Source/WebCore/ChangeLog:13
>>> +        * Configurations/FeatureDefines.xcconfig: Add disabled CSS3_TEXT_ALIGN_LAST flag.
>> 
>> I really don't think another flag for a *single* feature is going to cut. I would rather see that you repurpose the existing CSS3_TEXT_DECORATION to be the umbrella flag for all CSS3 Text features.
> 
> But, as you know, text decoration and text alignment are in different chapter in the spec, and it seems they are really different feature.
> 
> I don't think "CSS3_TEXT_DECORATION" can be a kind of comprehensive flag of text-align-last and text decoration.
> 
> 
> How about this?
> 
> 1. Let's make "CSS3_TEXT_ALIGNMENT", and we can implement 'text-align-last' and 'text-justify' under the new flag.
> 
> 2. Or, Let's make "CSS3_TEXT", and let text-align-last and text decoration be in the flag.
> 
> 
> What do you think?

Option 2, I should have requested a broader flag upfront instead of settling for CSS3_TEXT_DECORATION. Please do the rename in a separate bug and CC me.

[Dongwoo Joshua Im (dwim)]

Created attachment 169843 [details]
Patch

[Dongwoo Joshua Im (dwim)]

Created attachment 170036 [details]
Rebased patch

[WebKit Review Bot]

Attachment 170036 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:180:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/mac/TestExpectations:976:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/qt-mac/TestExpectations:5809:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:45:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/qt/TestExpectations:472:  Path does not exist.  [test/expectations] [5]
Total errors found: 5 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dongwoo Joshua Im (dwim)]

Created attachment 170040 [details]
Rebased patch

[Pravin D]

Comment on attachment 170040 [details]
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170040&action=review

> Source/WebCore/rendering/style/StyleRareInheritedData.h:114
> +    unsigned m_textAlignLast : 4; // ETextAlignLast

Drive-by comment
Nit: m_textAlignLast seems to take only 7 values. Can it use 3bits instead of 4bits ? 
IMHO Its better to be conservative of the number of bits being used by an object as it might have significant effect for memory aligned classes.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #16)
> (From update of attachment 170040 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170040&action=review
> 
> > Source/WebCore/rendering/style/StyleRareInheritedData.h:114
> > +    unsigned m_textAlignLast : 4; // ETextAlignLast
> 
> Drive-by comment
> Nit: m_textAlignLast seems to take only 7 values. Can it use 3bits instead of 4bits ? 
> IMHO Its better to be conservative of the number of bits being used by an object as it might have significant effect for memory aligned classes.

Hmm.. I think 3 bits would be enough.
I'll modify it!

[Dongwoo Joshua Im (dwim)]

Created attachment 171577 [details]
Patch

[Julien Chaffraix]

Comment on attachment 171577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171577&action=review

It will need another round but it is way better.

> Source/WebCore/css/StyleBuilder.cpp:1949
> +    setPropertyHandler(CSSPropertyWebkitTextAlignLast, ApplyPropertyTextAlignLast::createHandler());

I don't think a custom property handler is needed. You should get the same result with:

setPropertyHandler(CSSPropertyWebkitTextAlignLast, ApplyPropertyDefault<ETextAlignLast, &RenderStyle::textAlignLast, EBorderStyle, &RenderStyle::setTextAlignLast, ETextAlignLast, &RenderStyle::initialTextAlignLast>::createHandler()));

> Source/WebCore/rendering/style/RenderStyleConstants.h:351
> +    TALAUTO, TALSTART, TALEND, TALLEFT, TALRIGHT, TALCENTER, TALJUSTIFY

The old properties are using fully capitalized names but the new ones tend to use camel case. I would use camel case as it matches our usual style and probably s/TAL/TextAlignLast/ on all the properties above.

> LayoutTests/ChangeLog:14
> +        * fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last-inherited.js: Added.

Side comment: testing all the inherited value is fine, you only needed to test one to make sure the inheritance works as expected (which could have been folded in the other test).

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last-inherited.js:10
> +testContainer.contentEditable = true;

Is this required?

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last-inherited.js:17
> +debug("Value of ancester is 'start':");

s/ancester/ancestor/ (applies to each debug statement)

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last-inherited.js:23
> +debug("Value of ancester is 'end':");
> +ancestor.style.webkitTextAlignLast = 'end';
> +testComputedStyle("end", "end");

There is a pattern here, you should probably move all this repeated code in a testing function. Also it would be better to have spaces between each values in the output for readability.

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last.js:15
> +    shouldBe("computedStyle.getPropertyCSSValue('" + propertyCSS + "').toString()", "'" + type + "'");

Really not sure what the toString() is testing nor that it is useful information. I would just drop it.

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last.js:19
> +description("Test to make sure -webkit-text-align-last property returns values properly.")

Not a super super helpful description.

description("This test checks that -webkit-text-align-last parses properly the properties from CSS 3 Text."

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last.js:25
> +var testContainer = document.createElement("div");
> +testContainer.contentEditable = true;
> +document.body.appendChild(testContainer);
> +
> +testContainer.innerHTML = '<div id="test" dir="ltr">hello world</div>';

You should probably just replace this with the right HTML markup.

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last.js:27
> +

Where do you test the initial value?

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last.js:69
> +

Where do you test an invalid value?

As an extra protection, I would test against -webkit-left, -webkit-right, -webkit-center, -webkit-match-parent.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #19)
> (From update of attachment 171577 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171577&action=review
> 
> It will need another round but it is way better.

another round, sounds ok.
I will work on this patch on this weekend.

Thanks for the review, Julien!

[Bear Travis]

Comment on attachment 171577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171577&action=review

Just some small additions to Julien's feedback. The text decoration tests do combine testElementStyle and testComputedStyle into a 'testValue' method you might consider using.

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last-inherited.js:3
> +    shouldBe("window.getComputedStyle(ancestor).getPropertyCSSValue('-webkit-text-align-last').cssText",  "'" + a_value + "'");

Nit: An alternative is getPropertyValue if you are just looking for the string value.

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last-inherited.js:7
> +description("Test to make sure inherited -webkit-text-align-last property returns values properly.")

Nit: You could rephrase this to emphasize that you're testing css inheritance: "Test to make sure the -webkit-text-align-last property is properly inherited".

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last.js:26
> +testContainer.innerHTML = '<div id="test" dir="ltr">hello world</div>';
> +e = document.getElementById('test');

Does the 'dir' attribute affect the computed values for text-align-last? If not, I would recommend omitting it. The spec talks mostly about the 'direction' style property as opposed to the 'dir' html attribute, so you may need to test both when actually doing layout.

[Dongwoo Joshua Im (dwim)]

Created attachment 172497 [details]
Patch

[WebKit Review Bot]

Attachment 172497 [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
LayoutTests/platform/qt/TestExpectations:467:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:1313:  Path does not exist.  [test/expectations] [5]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #23)
> Attachment 172497 [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
> LayoutTests/platform/qt/TestExpectations:467:  Path does not exist.  [test/expectations] [5]
> LayoutTests/platform/gtk/TestExpectations:1313:  Path does not exist.  [test/expectations] [5]
> Total errors found: 2 in 26 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

These are not about my patch.
These errors occurred because my patch touch the files..

[Dongwoo Joshua Im (dwim)]

(In reply to comment #24)
> (In reply to comment #23)
> > Attachment 172497 [details] [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
> > LayoutTests/platform/qt/TestExpectations:467:  Path does not exist.  [test/expectations] [5]
> > LayoutTests/platform/gtk/TestExpectations:1313:  Path does not exist.  [test/expectations] [5]
> > Total errors found: 2 in 26 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> These are not about my patch.
> These errors occurred because my patch touch the files..

Style error fixed by http://trac.webkit.org/changeset/133571.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #19)
> (From update of attachment 171577 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171577&action=review
> 

I've fixed patch regarding your comments, Julien.
Please review this again.

[Julien Chaffraix]

Comment on attachment 172497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172497&action=review

Please don't forget to regenerate the ChangeLog after the removal and don't downplay its importance (ie it needs to be filled with important information).

> Source/WebCore/ChangeLog:9
> +        This patch implements the "text-align-last" property specified in CSS3
> +        working draft, with "-webkit-" prefix, under ENABLE_CSS3_TEXT flag.

This is inaccurate: you implement only the parsing side of "text-align-last".

> Source/WebCore/ChangeLog:45
> +        (StyleRareInheritedData):

Please fill these entries too.

> Source/WebCore/css/StyleBuilder.cpp:1158
> +#if ENABLE(CSS3_TEXT)
> +class ApplyPropertyTextAlignLast {
> +public:
> +    static void applyInheritValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        styleResolver->style()->setTextAlignLast(styleResolver->parentStyle()->textAlignLast());
> +    }
> +
> +    static void applyInitialValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        styleResolver->style()->setTextAlignLast(RenderStyle::initialTextAlignLast());
> +    }
> +
> +    static void applyValue(CSSPropertyID, StyleResolver* styleResolver, CSSValue* value)
> +    {
> +        if (!value->isPrimitiveValue())
> +            return;
> +
> +        CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> +        styleResolver->style()->setTextAlignLast(*primitiveValue);
> +    }
> +
> +    static PropertyHandler createHandler() { return PropertyHandler(&applyInheritValue, &applyInitialValue, &applyValue); }
> +
> +};
> +#endif // CSS3_TEXT

This is dead code now. Please remove it.

> Source/WebCore/rendering/style/RenderStyleConstants.h:351
> +    TextAlignLastAUTO, TextAlignLastSTART, TextAlignLastEND, TextAlignLastLEFT, TextAlignLastRIGHT, TextAlignLastCENTER, TextAlignLastJUSTIFY

This is not proper camel-case. Please don't capitalize the last word as it doesn't match WebKit style.

> LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last.js:3
> +    if (type != null) {

This check always passes AFAICT and you never check against null.

> LayoutTests/platform/gtk/TestExpectations:45
> +webkit.org/b/76173 fast/css3-text/css3-text-align-last [ Missing ]

This line is wrong. [Missing] is about missing expected files which you have here. It should be [ Skipped ] or nothing (they are equivalent). It should also be located with the other fast/css3-text entry like the other changes.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #27)
> (From update of attachment 172497 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172497&action=review
> 
> Please don't forget to regenerate the ChangeLog after the removal and don't downplay its importance (ie it needs to be filled with important information).
> 
Yes, I will.

> > Source/WebCore/ChangeLog:9
> > +        This patch implements the "text-align-last" property specified in CSS3
> > +        working draft, with "-webkit-" prefix, under ENABLE_CSS3_TEXT flag.
> 
> This is inaccurate: you implement only the parsing side of "text-align-last".
> 
I modified it.

> > Source/WebCore/ChangeLog:45
> > +        (StyleRareInheritedData):
> 
> Please fill these entries too.
> 
Add comment.

> > Source/WebCore/css/StyleBuilder.cpp:1158
> > +#if ENABLE(CSS3_TEXT)
> > +class ApplyPropertyTextAlignLast {
> > +public:
> > +    static void applyInheritValue(CSSPropertyID, StyleResolver* styleResolver)
> > +    {
> > +        styleResolver->style()->setTextAlignLast(styleResolver->parentStyle()->textAlignLast());
> > +    }
> > +
> > +    static void applyInitialValue(CSSPropertyID, StyleResolver* styleResolver)
> > +    {
> > +        styleResolver->style()->setTextAlignLast(RenderStyle::initialTextAlignLast());
> > +    }
> > +
> > +    static void applyValue(CSSPropertyID, StyleResolver* styleResolver, CSSValue* value)
> > +    {
> > +        if (!value->isPrimitiveValue())
> > +            return;
> > +
> > +        CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> > +        styleResolver->style()->setTextAlignLast(*primitiveValue);
> > +    }
> > +
> > +    static PropertyHandler createHandler() { return PropertyHandler(&applyInheritValue, &applyInitialValue, &applyValue); }
> > +
> > +};
> > +#endif // CSS3_TEXT
> 
> This is dead code now. Please remove it.
> 
Removed.

> > Source/WebCore/rendering/style/RenderStyleConstants.h:351
> > +    TextAlignLastAUTO, TextAlignLastSTART, TextAlignLastEND, TextAlignLastLEFT, TextAlignLastRIGHT, TextAlignLastCENTER, TextAlignLastJUSTIFY
> 
> This is not proper camel-case. Please don't capitalize the last word as it doesn't match WebKit style.
> 
I did.

> > LayoutTests/fast/css3-text/css3-text-align-last/getComputedStyle/script-tests/getComputedStyle-text-align-last.js:3
> > +    if (type != null) {
> 
> This check always passes AFAICT and you never check against null.
> 
Removed.

> > LayoutTests/platform/gtk/TestExpectations:45
> > +webkit.org/b/76173 fast/css3-text/css3-text-align-last [ Missing ]
> 
> This line is wrong. [Missing] is about missing expected files which you have here. It should be [ Skipped ] or nothing (they are equivalent). It should also be located with the other fast/css3-text entry like the other changes.

Actually, no reason to skip this test on GTK port,
so I removed this line.

[Dongwoo Joshua Im (dwim)]

Created attachment 172926 [details]
Patch

[WebKit Review Bot]

Attachment 172926 [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
LayoutTests/platform/qt/TestExpectations:2402:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #30)
> Attachment 172926 [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
> LayoutTests/platform/qt/TestExpectations:2402:  Path does not exist.  [test/expectations] [5]
> Total errors found: 1 in 25 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This is not about my patch...

[WebKit Review Bot]

Comment on attachment 172926 [details]
Patch

Rejecting attachment 172926 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
51>At revision 166379.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...
LayoutTests/platform/qt/TestExpectations:2402:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 5 files

Full output: http://queues.webkit.org/results/14745991

[Dongwoo Joshua Im (dwim)]

(In reply to comment #32)
> (From update of attachment 172926 [details])
> Rejecting attachment 172926 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> -externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> 51>At revision 166379.
> 
> ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> 
> ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> Updating webkit projects from gyp files...
> LayoutTests/platform/qt/TestExpectations:2402:  Path does not exist.  [test/expectations] [5]
> Total errors found: 1 in 5 files
> 
> Full output: http://queues.webkit.org/results/14745991

This will be resolve by https://bugs.webkit.org/show_bug.cgi?id=101562.

[Dongwoo Joshua Im (dwim)]

Created attachment 173527 [details]
Patch

[WebKit Review Bot]

Comment on attachment 173527 [details]
Patch

Clearing flags on attachment: 173527

Committed r134190: <http://trac.webkit.org/changeset/134190>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.