Bug 100058 – [CSS3] Parsing the property, text-justify.

• Component: CSS
• Status: RESOLVED FIXED

[Dongwoo Joshua Im (dwim)]

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

[Dongwoo Joshua Im (dwim)]

Created attachment 174090 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 174090 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:1875
> +    case CSSPropertyWebkitTextJustify:

This could go to is isValidKeywordPropertyAndValue like the same kind of properties. This will help speed wise in JS. In fact CSSPropertyWebkitTextAlignLast should be there too. If you make a separate patch for the latter I can review it.

[Dongwoo Joshua Im (dwim)]

Created attachment 174287 [details]
Patch

[Dongwoo Joshua Im (dwim)]

(In reply to comment #2)
> (From update of attachment 174090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174090&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:1875
> > +    case CSSPropertyWebkitTextJustify:
> 
> This could go to is isValidKeywordPropertyAndValue like the same kind of properties. This will help speed wise in JS. In fact CSSPropertyWebkitTextAlignLast should be there too. If you make a separate patch for the latter I can review it.

The lines you mentioned have been moved into isValidKeywordPropertyAndValue function.

And, I will create a separate bug, and let you know.

Thanks.

[Alexis Menard (darktears)]

Comment on attachment 174287 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        (WebCore::CSSComputedStyleDeclaration::getPropertyCSSValue): Get the value of the text-justify property.

Remove this comment it doesn't bring much.

> Source/WebCore/ChangeLog:19
> +        (WebCore::isValidKeywordPropertyAndValue): Check whether it is a proper value which text-justify can have.

Ditto here.

> Source/WebCore/ChangeLog:28
> +        * css/CSSPropertyNames.in: Add '-webkit-text-justify' property.

Ditto.

> Source/WebCore/ChangeLog:29
> +        * css/CSSValueKeywords.in: Add the possible values of the '-webkit-text-justify' property.

Ditto.

> Source/WebCore/ChangeLog:42
> +        (StyleRareInheritedData): Add m_textJustify.

Same, we can deduct by reading the patch. It usually better to comment here the approach for example, complicated cases,why you refactored, or something special to take into account.

> Source/WebCore/rendering/style/RenderStyleConstants.h:354
> +enum ETextJustify {

I don't think you want the E here. I know ETextAlignLast have it but it seems that we should not use it (only if it the enum clash with with another type). This file is a bit messy when it comes to the coding style. But it seems that recent addition tend to land without the E prefix. ETextAlignLast went through by mistake apparently (you can double check with jchaffraix).

[Alexis Menard (darktears)]

Comment on attachment 174287 [details]
Patch

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

> LayoutTests/ChangeLog:25
> +        * platform/chromium/TestExpectations: Skip these new test cases.

Comment not very useful

> LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/script-tests/getComputedStyle-text-justify.js:50
> +invalidValueSettingTest('-webkit-left', defaultValue);

I would test also some other invalid value such as 'solid' or something else. Also could you cover inherit and initial as values?

> LayoutTests/platform/chromium/TestExpectations:206
> +webkit.org/b/99945 fast/css3-text/css3-text-justify

You could add [TEXT] here as the test will be run (so you can catch crasher) but ignore in the results. Same for below.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #6)
> (From update of attachment 174287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174287&action=review
> 
> > LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/script-tests/getComputedStyle-text-justify.js:50
> > +invalidValueSettingTest('-webkit-left', defaultValue);
> 
> I would test also some other invalid value such as 'solid' or something else. Also could you cover inherit and initial as values?
> 
I will add some more keywords, such as inherit, initial, solid, normal, bold, ltr, and so on.

> > LayoutTests/platform/chromium/TestExpectations:206
> > +webkit.org/b/99945 fast/css3-text/css3-text-justify
> 
> You could add [TEXT] here as the test will be run (so you can catch crasher) but ignore in the results. Same for below.
>
like this? - "[ Failure ]"

[Dongwoo Joshua Im (dwim)]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 174287 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=174287&action=review
> > 
> > > LayoutTests/platform/chromium/TestExpectations:206
> > > +webkit.org/b/99945 fast/css3-text/css3-text-justify
> > 
> > You could add [TEXT] here as the test will be run (so you can catch crasher) but ignore in the results. Same for below.
> >
> like this? - "[ Failure ]"

[ Skip ] would be right here, I think.

[Alexis Menard (darktears)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 174287 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=174287&action=review
> > > 
> > > > LayoutTests/platform/chromium/TestExpectations:206
> > > > +webkit.org/b/99945 fast/css3-text/css3-text-justify
> > > 
> > > You could add [TEXT] here as the test will be run (so you can catch crasher) but ignore in the results. Same for below.
> > >
> > like this? - "[ Failure ]"
> 
> [ Skip ] would be right here, I think.

It's an alternative sure but [Text] as like this http://trac.webkit.org/changeset/135632/trunk/LayoutTests/platform/qt/TestExpectations run the test but ignore the result so it is skipped but it runs the test. I really don't mind it's a suggestion.

[Dongwoo Joshua Im (dwim)]

Created attachment 182335 [details]
Patch

[Julien Chaffraix]

Comment on attachment 174287 [details]
Patch

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

>>>>> LayoutTests/platform/chromium/TestExpectations:206
>>>>> +webkit.org/b/99945 fast/css3-text/css3-text-justify
>>>> 
>>>> You could add [TEXT] here as the test will be run (so you can catch crasher) but ignore in the results. Same for below.
>>> 
>>> like this? - "[ Failure ]"
>> 
>> [ Skip ] would be right here, I think.
> 
> It's an alternative sure but [Text] as like this http://trac.webkit.org/changeset/135632/trunk/LayoutTests/platform/qt/TestExpectations run the test but ignore the result so it is skipped but it runs the test. I really don't mind it's a suggestion.

I was under the impression - maybe wrong - that you couldn't put a [ Text ] entry on a directory, which is why the other entries don't have it. I agree with Alexis that it should be [ Text ] if possible.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #11)
> (From update of attachment 174287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174287&action=review
> 
> >>>>> LayoutTests/platform/chromium/TestExpectations:206
> >>>>> +webkit.org/b/99945 fast/css3-text/css3-text-justify
> >>>> 
> >>>> You could add [TEXT] here as the test will be run (so you can catch crasher) but ignore in the results. Same for below.
> >>> 
> >>> like this? - "[ Failure ]"
> >> 
> >> [ Skip ] would be right here, I think.
> > 
> > It's an alternative sure but [Text] as like this http://trac.webkit.org/changeset/135632/trunk/LayoutTests/platform/qt/TestExpectations run the test but ignore the result so it is skipped but it runs the test. I really don't mind it's a suggestion.
> 
> I was under the impression - maybe wrong - that you couldn't put a [ Text ] entry on a directory, which is why the other entries don't have it. I agree with Alexis that it should be [ Text ] if possible.

I can see some directories have [ Expectation ] at the TestExpectations file in the mac, chromium, and the other ports as well.
I might think directory could have one, and there is no mention about that at the http://trac.webkit.org/wiki/TestExpectations.
But, if you don't prefer this way, I could remove those, because I don't have any preference about that.

BTW, Julien, what do you think about the "E" prefix at the name of the enum?
Do you think I can (or, need) remove the "E" from the ETextAlignLast, as Alexis said at the comment #5 below?

[Julien Chaffraix]

> I can see some directories have [ Expectation ] at the TestExpectations file in the mac, chromium, and the other ports as well.
> I might think directory could have one, and there is no mention about that at the http://trac.webkit.org/wiki/TestExpectations.
> But, if you don't prefer this way, I could remove those, because I don't have any preference about that.

Keeping the expectation is better as the tests are at least run and not just skipped.

> BTW, Julien, what do you think about the "E" prefix at the name of the enum?
> Do you think I can (or, need) remove the "E" from the ETextAlignLast, as Alexis said at the comment #5 below?

The E-prefix shouldn't be added to new enums as this doesn't follow the WebKit style (and slowly rolled into old enums but that's another topic). It's my oversight that this new enum went through with the E-prefix so yes it should be changed. Please put that in a follow-up refactoring though.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #13)

> Keeping the expectation is better as the tests are at least run and not just skipped.

I see. 
Then, I'll use "[ Failure ]" here, because the test cases will fail with text diff.


> The E-prefix shouldn't be added to new enums as this doesn't follow the WebKit style (and slowly rolled into old enums but that's another topic). It's my oversight that this new enum went through with the E-prefix so yes it should be changed. Please put that in a follow-up refactoring though.

ok. Then, I'll file it as another bug.

[Dongwoo Joshua Im (dwim)]

Created attachment 182732 [details]
Patch

[Bruno Abinader (history only)]

As you are using "CSS3_TEXT" feature flag, Julien suggested me while ago that we also upload a patch with this feature flag enabled by default, so the code inside ifdef's could also be tested on all EWS bots. I've been doing this together with Lamarque on several bugs pending review. You can find an updated sample of this patch in bug 102795. We usually upload two versions of the patch, one prepared for commit, and the other merged with this 'EWS-run-only' patch (see bug 91638 as example).

[Dongwoo Joshua Im (dwim)]

(In reply to comment #16)
> As you are using "CSS3_TEXT" feature flag, Julien suggested me while ago that we also upload a patch with this feature flag enabled by default, so the code inside ifdef's could also be tested on all EWS bots. I've been doing this together with Lamarque on several bugs pending review. You can find an updated sample of this patch in bug 102795. We usually upload two versions of the patch, one prepared for commit, and the other merged with this 'EWS-run-only' patch (see bug 91638 as example).

This flag is enabled at GTK and EFL port by default.
IMO, we can refer these two ports to see this patch occur build failure or not, can't we?
What do you think?

[Bruno Abinader (history only)]

(In reply to comment #17)
> (In reply to comment #16)
> > As you are using "CSS3_TEXT" feature flag, Julien suggested me while ago that we also upload a patch with this feature flag enabled by default, so the code inside ifdef's could also be tested on all EWS bots. I've been doing this together with Lamarque on several bugs pending review. You can find an updated sample of this patch in bug 102795. We usually upload two versions of the patch, one prepared for commit, and the other merged with this 'EWS-run-only' patch (see bug 91638 as example).
> 
> This flag is enabled at GTK and EFL port by default.
> IMO, we can refer these two ports to see this patch occur build failure or not, can't we?
> What do you think?

Sounds good, however AFAIK we do not run layout tests on these ports. As I remember, the reason to run on all ports, specially Chromium/Mac ones, is that they do run the layout tests on EWS bots. If layout test results are not a priority, then yes, I agree with you we can skip the 'EWS-run-only' patches for now.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > As you are using "CSS3_TEXT" feature flag, Julien suggested me while ago that we also upload a patch with this feature flag enabled by default, so the code inside ifdef's could also be tested on all EWS bots. I've been doing this together with Lamarque on several bugs pending review. You can find an updated sample of this patch in bug 102795. We usually upload two versions of the patch, one prepared for commit, and the other merged with this 'EWS-run-only' patch (see bug 91638 as example).
> > 
> > This flag is enabled at GTK and EFL port by default.
> > IMO, we can refer these two ports to see this patch occur build failure or not, can't we?
> > What do you think?
> 
> Sounds good, however AFAIK we do not run layout tests on these ports. As I remember, the reason to run on all ports, specially Chromium/Mac ones, is that they do run the layout tests on EWS bots. If layout test results are not a priority, then yes, I agree with you we can skip the 'EWS-run-only' patches for now.

But, that means I need to make port-specific expected result of the ports, even though they don't support this feature yet.

Actually, for this bug, port-specific expected results are not needed, but for the rendering patch which will follow this bug, port-specific expected results would be needed.
I really can't agree with the idea - "Make port-specific expected result only for the ews even if the feature is not enabled."


BTW, for this bug, what I need to do is only one thing - enabling the CSS3_TEXT at chromium port by default.

If Julien really want to see this, I will make the "EWS only patch".

Julien, what do you think?

[Dongwoo Joshua Im (dwim)]

Created attachment 183185 [details]
Patch only for EWS

[Dongwoo Joshua Im (dwim)]

Created attachment 183441 [details]
patch for landing

[Andreas Kling]

Comment on attachment 183441 [details]
patch for landing

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

Looks like a good start! r=me

> LayoutTests/ChangeLog:25
> +        * platform/chromium/TestExpectations: Skip these new test cases, because CSS3_TEXT is not enabled yet.

The Chromium part of this patch is no longer relevant, obviously.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #22)
> (From update of attachment 183441 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183441&action=review
> 
> Looks like a good start! r=me
> 
> > LayoutTests/ChangeLog:25
> > +        * platform/chromium/TestExpectations: Skip these new test cases, because CSS3_TEXT is not enabled yet.
> 
> The Chromium part of this patch is no longer relevant, obviously.

Oh, OK.
I will land this patch after removing that part.

[Dongwoo Joshua Im (dwim)]

Created attachment 197023 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 197023 [details]
Patch

Clearing flags on attachment: 197023

Committed r148070: <http://trac.webkit.org/changeset/148070>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.