Bug 100058

Summary: [CSS3] Parsing the property, text-justify.
Product: WebKit Reporter: Dongwoo Joshua Im (dwim) <dw.im>
Component: CSSAssignee: Dongwoo Joshua Im (dwim) <dw.im>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, bruno.abinader, cmarcelo, commit-queue, donggwan.kim, ebrahim, eric, esprehn+autocc, jchaffraix, lamarque, macpherson, menard, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 99945, 99584    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch only for EWS
none
patch for landing
none
Patch none

Description Dongwoo Joshua Im (dwim) 2012-10-22 17:38:53 PDT
This is the first step to support "text-justify" property in WebKit.
Implement to parse the property, and test that with 'getComputedStyle'.
Comment 1 Dongwoo Joshua Im (dwim) 2012-11-14 00:02:30 PST
Created attachment 174090 [details]
Patch
Comment 2 Alexis Menard (darktears) 2012-11-14 03:39:45 PST
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.
Comment 3 Dongwoo Joshua Im (dwim) 2012-11-14 16:48:14 PST
Created attachment 174287 [details]
Patch
Comment 4 Dongwoo Joshua Im (dwim) 2012-11-14 16:50:45 PST
(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.
Comment 5 Alexis Menard (darktears) 2012-12-11 03:44:27 PST
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).
Comment 6 Alexis Menard (darktears) 2012-12-11 03:50:55 PST
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.
Comment 7 Dongwoo Joshua Im (dwim) 2013-01-11 05:43:29 PST
(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 ]"
Comment 8 Dongwoo Joshua Im (dwim) 2013-01-11 05:46:23 PST
(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.
Comment 9 Alexis Menard (darktears) 2013-01-11 05:48:22 PST
(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.
Comment 10 Dongwoo Joshua Im (dwim) 2013-01-11 06:46:17 PST
Created attachment 182335 [details]
Patch
Comment 11 Julien Chaffraix 2013-01-11 12:59:57 PST
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.
Comment 12 Dongwoo Joshua Im (dwim) 2013-01-12 06:03:54 PST
(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?
Comment 13 Julien Chaffraix 2013-01-14 10:24:35 PST
> 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.
Comment 14 Dongwoo Joshua Im (dwim) 2013-01-15 02:01:29 PST
(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.
Comment 15 Dongwoo Joshua Im (dwim) 2013-01-15 03:40:48 PST
Created attachment 182732 [details]
Patch
Comment 16 Bruno Abinader (history only) 2013-01-15 05:03:44 PST
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).
Comment 17 Dongwoo Joshua Im (dwim) 2013-01-15 05:11:33 PST
(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?
Comment 18 Bruno Abinader (history only) 2013-01-15 05:30:23 PST
(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.
Comment 19 Dongwoo Joshua Im (dwim) 2013-01-17 06:09:04 PST
(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?
Comment 20 Dongwoo Joshua Im (dwim) 2013-01-17 07:58:57 PST
Created attachment 183185 [details]
Patch only for EWS
Comment 21 Dongwoo Joshua Im (dwim) 2013-01-18 06:46:01 PST
Created attachment 183441 [details]
patch for landing
Comment 22 Andreas Kling 2013-04-09 02:59:06 PDT
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.
Comment 23 Dongwoo Joshua Im (dwim) 2013-04-09 03:06:24 PDT
(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.
Comment 24 Dongwoo Joshua Im (dwim) 2013-04-09 04:05:54 PDT
Created attachment 197023 [details]
Patch
Comment 25 WebKit Commit Bot 2013-04-09 17:31:09 PDT
Comment on attachment 197023 [details]
Patch

Clearing flags on attachment: 197023

Committed r148070: <http://trac.webkit.org/changeset/148070>
Comment 26 WebKit Commit Bot 2013-04-09 17:31:13 PDT
All reviewed patches have been landed.  Closing bug.