[CSS3] Parsing the property, text-justify.

[CSS3] Parsing the property, text-justify.

This patch implements the parsing side of the "text-justify" property specified
in CSS3 working draft.
Specification link : http://dev.w3.org/csswg/css-text/#text-justify

Tests: fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify-inherited.html
       fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html

BUG=250762
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159634

[dw.im, 2013-06-18 12:11:18]

I check the test result with enabling the CSS3_TEXT flag.
If anyone need, I will upload it as well.

[pdr., 2013-06-18 13:06:21]

This change looks good but I'm not an expert how our CSS system works. I've
added dstockwell for the final review.

@dstockwell, do you mind looking this over?

https://codereview.chromium.org/17155007/diff/1/LayoutTests/fast/css3-text/cs...
File
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/script-tests/getComputedStyle-text-justify.js
(right):

https://codereview.chromium.org/17155007/diff/1/LayoutTests/fast/css3-text/cs...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/script-tests/getComputedStyle-text-justify.js:47:
debug('');
Are these empty debug lines needed?

https://codereview.chromium.org/17155007/diff/1/Source/core/css/DeprecatedSty...
File Source/core/css/DeprecatedStyleBuilder.cpp (right):

https://codereview.chromium.org/17155007/diff/1/Source/core/css/DeprecatedSty...
Source/core/css/DeprecatedStyleBuilder.cpp:1669:
setPropertyHandler(CSSPropertyWebkitTextJustify,
ApplyPropertyDefault<TextJustify, &RenderStyle::textJustify, TextJustify,
&RenderStyle::setTextJustify, TextJustify,
&RenderStyle::initialTextJustify>::createHandler());
Should we be adding new code to the DeprecatedStyleBuilder? I'm not an expert in
this area, but it feels like we may need to add this somewhere else.

[Mike Lawther (Google), 2013-06-19 11:09:11]

What is the plan for removing the prefix from the CSS3 text properties? I know
they are behind a compile flag, but Blink shouldn't really be shipping anything
new with a prefix.

You'll need to add the new property to
http://src.chromium.org/viewvc/blink/trunk/Source/core/page/UseCounter.cpp,
otherwise you'll get a compile fail on the clang bots (Mac and Linux). If you're
developing on Linux, you should switch to using clang so you can catch this
locally :)

[dw.im, 2013-06-24 08:11:07]

On 2013/06/19 11:09:11, Mike Lawther (Google) wrote:
> What is the plan for removing the prefix from the CSS3 text properties? I know
> they are behind a compile flag, but Blink shouldn't really be shipping
anything
> new with a prefix.
> 
Hmm.. Then, I need to make new patch without the prefix.

And, another properties behind same flag, 
I will try with another bug.

> You'll need to add the new property to
> http://src.chromium.org/viewvc/blink/trunk/Source/core/page/UseCounter.cpp,
> otherwise you'll get a compile fail on the clang bots (Mac and Linux). If
you're
> developing on Linux, you should switch to using clang so you can catch this
> locally :)
>
Oh! I see. I will do it.
Thanks!

And, clang.. I will try~ ;)

[Julien - ping for review, 2013-07-15 22:05:46]

https://codereview.chromium.org/17155007/diff/18001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify-inherited.html
(right):

https://codereview.chromium.org/17155007/diff/18001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify-inherited.html:8:
<script src="script-tests/getComputedStyle-text-justify-inherited.js"></script>
It's much better to inline scripts for such simple tests.

https://codereview.chromium.org/17155007/diff/18001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/script-tests/getComputedStyle-text-justify-inherited.js
(right):

https://codereview.chromium.org/17155007/diff/18001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/script-tests/getComputedStyle-text-justify-inherited.js:31:
inheritanceTest("distribute");
Do we really need separate tests for inherited vs non-inherited? It makes it
hard to know what's tested in your change.

https://codereview.chromium.org/17155007/diff/18001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/script-tests/getComputedStyle-text-justify.js
(right):

https://codereview.chromium.org/17155007/diff/18001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/script-tests/getComputedStyle-text-justify.js:32:
function computedValueSettingTest(value, defaultValue)
This function is testing nothing if defaultValue = 'auto' which is the only way
you call it.

If we reject the value because we don't handle it (and I am not sure we do),
text-justify would be the default 'auto'.

To be sure that we do handle it, you need to use non-default values for the
parent element ('inherit') / the element ('initial').

https://codereview.chromium.org/17155007/diff/18001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/17155007/diff/18001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:939: return
RuntimeEnabledFeatures::css3TextEnabled();
I don't think this logic is OK. In debug, we will hit the ASSERT if CSS3 text is
disabled. This should just always return true and the runtime flag check above
would reject the properties.

https://codereview.chromium.org/17155007/diff/18001/Source/core/css/Deprecate...
File Source/core/css/DeprecatedStyleBuilder.cpp (right):

https://codereview.chromium.org/17155007/diff/18001/Source/core/css/Deprecate...
Source/core/css/DeprecatedStyleBuilder.cpp:1378:
setPropertyHandler(CSSPropertyTextJustify, ApplyPropertyDefault<TextJustify,
&RenderStyle::textJustify, TextJustify, &RenderStyle::setTextJustify,
TextJustify, &RenderStyle::initialTextJustify>::createHandler());
It is NOT acceptable to add more code to a deprecated class!

This is handled super easily by a CSSPropertyNames.in entry with type_name.

https://codereview.chromium.org/17155007/diff/18001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/17155007/diff/18001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyleConstants.h:361: TextJustifyAuto,
TextJustifyNone, TextJustifyInterWord, TextJustifyDistribute
You should put a FIXME about the value you don't implement. The specification
also allows: inter-ideograph | inter-cluster (kashida being optional)

It would be better still to actually implement ALL of the values instead of just
implementing a subset.

[dw.im, 2013-07-16 01:13:55]

https://codereview.chromium.org/17155007/diff/18001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/script-tests/getComputedStyle-text-justify.js
(right):

https://codereview.chromium.org/17155007/diff/18001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/script-tests/getComputedStyle-text-justify.js:32:
function computedValueSettingTest(value, defaultValue)
On 2013/07/15 22:05:47, Julien Chaffraix wrote:
> This function is testing nothing if defaultValue = 'auto' which is the only
way
> you call it.
> 
> If we reject the value because we don't handle it (and I am not sure we do),
> text-justify would be the default 'auto'.
> 
> To be sure that we do handle it, you need to use non-default values for the
> parent element ('inherit') / the element ('initial').

I see. 
I'll try it with another value.

https://codereview.chromium.org/17155007/diff/18001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/17155007/diff/18001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:939: return
RuntimeEnabledFeatures::css3TextEnabled();
On 2013/07/15 22:05:47, Julien Chaffraix wrote:
> I don't think this logic is OK. In debug, we will hit the ASSERT if CSS3 text
is
> disabled. This should just always return true and the runtime flag check above
> would reject the properties.

Then, I think TextAlignLast is also needed to return true always, isn't it?

https://codereview.chromium.org/17155007/diff/18001/Source/core/css/Deprecate...
File Source/core/css/DeprecatedStyleBuilder.cpp (right):

https://codereview.chromium.org/17155007/diff/18001/Source/core/css/Deprecate...
Source/core/css/DeprecatedStyleBuilder.cpp:1378:
setPropertyHandler(CSSPropertyTextJustify, ApplyPropertyDefault<TextJustify,
&RenderStyle::textJustify, TextJustify, &RenderStyle::setTextJustify,
TextJustify, &RenderStyle::initialTextJustify>::createHandler());
On 2013/07/15 22:05:47, Julien Chaffraix wrote:
> It is NOT acceptable to add more code to a deprecated class!
> 
> This is handled super easily by a CSSPropertyNames.in entry with type_name.

in the CSSProperties.in file, I see.

https://codereview.chromium.org/17155007/diff/18001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/17155007/diff/18001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyleConstants.h:361: TextJustifyAuto,
TextJustifyNone, TextJustifyInterWord, TextJustifyDistribute
On 2013/07/15 22:05:47, Julien Chaffraix wrote:
> You should put a FIXME about the value you don't implement. The specification
> also allows: inter-ideograph | inter-cluster (kashida being optional)
> 
> It would be better still to actually implement ALL of the values instead of
just
> implementing a subset.

As you can see the very current editors draft of the spec, inter-ideograph,
inter-cluster, and kashida are disappeared.
 - http://dev.w3.org/csswg/css-text/#text-justify

That's why I didn't add those values here.

[Julien - ping for review, 2013-07-16 19:40:08]

https://codereview.chromium.org/17155007/diff/18001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/17155007/diff/18001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:939: return
RuntimeEnabledFeatures::css3TextEnabled();
On 2013/07/16 01:13:56, dw.im wrote:
> On 2013/07/15 22:05:47, Julien Chaffraix wrote:
> > I don't think this logic is OK. In debug, we will hit the ASSERT if CSS3
text
> is
> > disabled. This should just always return true and the runtime flag check
above
> > would reject the properties.
> 
> Then, I think TextAlignLast is also needed to return true always, isn't it?

Right, you need to return false when the flag is off but that's a small change.
You can always check how the other runtime enabled features work (e.g. grid
layout or region).

https://codereview.chromium.org/17155007/diff/18001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/17155007/diff/18001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyleConstants.h:361: TextJustifyAuto,
TextJustifyNone, TextJustifyInterWord, TextJustifyDistribute
On 2013/07/16 01:13:56, dw.im wrote:
> On 2013/07/15 22:05:47, Julien Chaffraix wrote:
> > You should put a FIXME about the value you don't implement. The
specification
> > also allows: inter-ideograph | inter-cluster (kashida being optional)
> > 
> > It would be better still to actually implement ALL of the values instead of
> just
> > implementing a subset.
> 
> As you can see the very current editors draft of the spec, inter-ideograph,
> inter-cluster, and kashida are disappeared.
>  - http://dev.w3.org/csswg/css-text/#text-justify
> 
> That's why I didn't add those values here.

I stand corrected, my bad.

[Julien - ping for review, 2013-07-17 01:51:09]

The code change is fine, the test would need another round.

https://chromiumcodereview.appspot.com/17155007/diff/32001/LayoutTests/fast/c...
File
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html
(right):

https://chromiumcodereview.appspot.com/17155007/diff/32001/LayoutTests/fast/c...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:32:
function invalidValueSettingTest(value, defaultValue)
Do we really need |defaultValue| as it's always auto?

https://chromiumcodereview.appspot.com/17155007/diff/32001/LayoutTests/fast/c...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:50:
shouldBe("window.getComputedStyle(child).getPropertyCSSValue('text-justify').cssText",
 "'" + c_value + "'");
Any reason this doesn't check also test e.style? As a rule you should be
consistent across your tests.

https://chromiumcodereview.appspot.com/17155007/diff/32001/LayoutTests/fast/c...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:54:
function ownValueTest(a_value, c_value)
let's use WebKit style for variable instead of
c_style_variable_that_do_not_match_our_style. Also don't shorten the names,
a_value is not very clear, ancestorValue is way better.

https://chromiumcodereview.appspot.com/17155007/diff/32001/LayoutTests/fast/c...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:66:
testComputedStyle_inherit(a_value, a_value);
Why 2 arguments when the function only takes one?

https://chromiumcodereview.appspot.com/17155007/diff/32001/LayoutTests/fast/c...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:86:
testComputedStyle('auto');
Why is this only testing getComputedStyle and not style?

https://chromiumcodereview.appspot.com/17155007/diff/32001/LayoutTests/fast/c...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:89:
valueSettingTest('auto');
This tests nothing as your default value is 'auto'.

https://chromiumcodereview.appspot.com/17155007/diff/32001/LayoutTests/fast/c...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:97:
invalidValueSettingTest('-webkit-right', defaultValue);
Can we avoid undocumented, unstandardized and prefixed values?

https://chromiumcodereview.appspot.com/17155007/diff/32001/LayoutTests/fast/c...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:110:
computedValueSettingTest('initial', 'auto');
This testing is very much based on the previous one passing which is a bit
annoying. If we regress and return 'auto' from the previous tests, this will
automatically pass.

That's a general comment about re-using the test fixture for all the tests.

https://chromiumcodereview.appspot.com/17155007/diff/32001/Source/core/css/CS...
File Source/core/css/CSSParser-in.cpp (right):

https://chromiumcodereview.appspot.com/17155007/diff/32001/Source/core/css/CS...
Source/core/css/CSSParser-in.cpp:969: case CSSPropertyTextJustify:
I would change CSSPropertyTextAlignLast above as part of this change too.

https://chromiumcodereview.appspot.com/17155007/diff/32001/Source/core/render...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://chromiumcodereview.appspot.com/17155007/diff/32001/Source/core/render...
Source/core/rendering/style/RenderStyleConstants.h:361: // FIXME: Implement
support for 'auto', 'none', 'inter-word', and 'distribute'
This FIXME is a bit too remote from the code you will touch to be useful
actually. What would have been fine is to mention if we were missing some values
in the enum (which we don't).

[dw.im, 2013-07-17 02:03:15]

One comment about the test case : 
It seems the file is little bit messed up during combine the two test cases.
I will try to make it more readible and clear.

https://codereview.chromium.org/17155007/diff/32001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html
(right):

https://codereview.chromium.org/17155007/diff/32001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:32:
function invalidValueSettingTest(value, defaultValue)
On 2013/07/17 01:51:10, Julien Chaffraix wrote:
> Do we really need |defaultValue| as it's always auto?

hmm... no. we don't.

https://codereview.chromium.org/17155007/diff/32001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:86:
testComputedStyle('auto');
On 2013/07/17 01:51:10, Julien Chaffraix wrote:
> Why is this only testing getComputedStyle and not style?

I will add one line of code to test it.

https://codereview.chromium.org/17155007/diff/32001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:89:
valueSettingTest('auto');
On 2013/07/17 01:51:10, Julien Chaffraix wrote:
> This tests nothing as your default value is 'auto'.

valueSettingTest is set & test, 
so, set as 'auto', and check if the value is setted ad 'auto'.

https://codereview.chromium.org/17155007/diff/32001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:97:
invalidValueSettingTest('-webkit-right', defaultValue);
On 2013/07/17 01:51:10, Julien Chaffraix wrote:
> Can we avoid undocumented, unstandardized and prefixed values?

oh! sure. I will use another one.
I used these values because it make me feel these values are really invalid.

https://codereview.chromium.org/17155007/diff/32001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/17155007/diff/32001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:969: case CSSPropertyTextJustify:
On 2013/07/17 01:51:10, Julien Chaffraix wrote:
> I would change CSSPropertyTextAlignLast above as part of this change too.

I can do that.
I've planned to fix that as part of another patch - "rendering text-align-last"
- which will follow this paptch, thogh.

https://codereview.chromium.org/17155007/diff/32001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/17155007/diff/32001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyleConstants.h:361: // FIXME: Implement
support for 'auto', 'none', 'inter-word', and 'distribute'
On 2013/07/17 01:51:10, Julien Chaffraix wrote:
> This FIXME is a bit too remote from the code you will touch to be useful
> actually. What would have been fine is to mention if we were missing some
values
> in the enum (which we don't).

Yes.. I will try to implement those value in near future..
Then, I will remove this "FIXME", and try to implement those.

[Julien - ping for review, 2013-07-17 17:35:24]

https://codereview.chromium.org/17155007/diff/32001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html
(right):

https://codereview.chromium.org/17155007/diff/32001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:89:
valueSettingTest('auto');
On 2013/07/17 02:03:15, dw.im wrote:
> On 2013/07/17 01:51:10, Julien Chaffraix wrote:
> > This tests nothing as your default value is 'auto'.
> 
> valueSettingTest is set & test, 
> so, set as 'auto', and check if the value is setted ad 'auto'.

Right, this is what the code does but it is actually not testing ANYTHING. You
are missing the fact that the value is already set to 'auto' (from the previous
test / initial value):
- if we don't handle 'auto' properly during parsing, we will carry the original
value of 'auto'
- if we handle it properly, the output would be 'auto'

Now because we have no way to know which of this case applies, this test doesn't
give any meaningful information.

[dw.im, 2013-10-11 06:10:52]

On 2013/07/17 17:35:24, Julien Chaffraix wrote:

Dear Julien,
Please review the new patch set.
Thanks!

[Julien - ping for review, 2013-10-14 12:57:45]

lgtm

https://codereview.chromium.org/17155007/diff/44001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html
(right):

https://codereview.chromium.org/17155007/diff/44001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:7:
<div id="test">Hello World</div>
Nit: Maybe remove the placeholder text.

https://codereview.chromium.org/17155007/diff/44001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:98:

Don't forget to test the special value 'initial'!

https://codereview.chromium.org/17155007/diff/44001/Source/core/css/CSSValueK...
File Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/17155007/diff/44001/Source/core/css/CSSValueK...
Source/core/css/CSSValueKeywords.in:235: //none
Space after comment for both lines.

https://codereview.chromium.org/17155007/diff/44001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/17155007/diff/44001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:425: CSSPropertyTextJustify,
Yosin, this change looks fine by me but you may have a different opinion.

[dw.im, 2013-10-15 01:14:14]

https://codereview.chromium.org/17155007/diff/44001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html
(right):

https://codereview.chromium.org/17155007/diff/44001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:7:
<div id="test">Hello World</div>
On 2013/10/14 12:57:46, Julien Chaffraix wrote:
> Nit: Maybe remove the placeholder text.

Done.

https://codereview.chromium.org/17155007/diff/44001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html:98:

On 2013/10/14 12:57:46, Julien Chaffraix wrote:
> Don't forget to test the special value 'initial'!

Done.

[yosin_UTC9, 2013-10-15 01:23:46]

ACK

https://codereview.chromium.org/17155007/diff/44001/Source/core/css/StyleProp...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/17155007/diff/44001/Source/core/css/StyleProp...
Source/core/css/StylePropertySet.cpp:425: CSSPropertyTextJustify,
On 2013/10/14 12:57:46, Julien Chaffraix wrote:
> Yosin, this change looks fine by me but you may have a different opinion.

Looks fine to me too.

[commit-bot: I haz the power, 2013-10-15 01:24:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dw.im@samsung.com/17155007/58001

[dw.im, 2013-10-15 01:24:54]

On 2013/10/15 01:23:46, Yoshifumi Inoue wrote:
> ACK
> 
>
https://codereview.chromium.org/17155007/diff/44001/Source/core/css/StyleProp...
> File Source/core/css/StylePropertySet.cpp (right):
> 
>
https://codereview.chromium.org/17155007/diff/44001/Source/core/css/StyleProp...
> Source/core/css/StylePropertySet.cpp:425: CSSPropertyTextJustify,
> On 2013/10/14 12:57:46, Julien Chaffraix wrote:
> > Yosin, this change looks fine by me but you may have a different opinion.
> 
> Looks fine to me too.

Thanks, Yosin!

[yosin_UTC9, 2013-10-15 01:28:41]

https://codereview.chromium.org/17155007/diff/58001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/17155007/diff/58001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:715: return true;
nit: Please put brace when conditional expression lays on multiple lines even if
then-clause is a single ine.

[commit-bot: I haz the power, 2013-10-15 02:13:14]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-10-15 02:31:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dw.im@samsung.com/17155007/58001

[commit-bot: I haz the power, 2013-10-15 02:54:28]

Change committed as 159634