Rendering text-justify:distribute for 8 bit characters.

Rendering text-justify:distribute for 8 bit characters.

text-justify:distribute provides the ability to justify a line as inter-character level.
This patch takes care of only 8 bit characters.

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

[dw.im, 2013-11-01 06:45:40]

I wanted to use reference test, 
but I don't know how I can make the expected result without 'text-justify:
ditribute'.

Any idea?

[leviw_travelin_and_unemployed, 2013-11-01 20:39:13]

I don't think you can do this test as a ref test.

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/Font.h:159: static unsigned
expansionOpportunityCount(const LChar*, size_t length, TextDirection, bool&
isAfterExpansion, bool distributeJustification = false);
Yech, boolean params! Why not just use the TextJustify enum?

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/WidthIterator.cpp (right):

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/WidthIterator.cpp:216: bool needExpansion =
Font::treatAsSpace(character) || m_distributeJustification;
how about something like isExpansionOpportunity instead of needExpansion?

https://codereview.chromium.org/54743004/diff/1/Source/platform/graphics/Text...
File Source/platform/graphics/TextRun.h (right):

https://codereview.chromium.org/54743004/diff/1/Source/platform/graphics/Text...
Source/platform/graphics/TextRun.h:250: bool m_distributeJustification;
Pack this along with the other bits below, otherwise you'll increase the size of
TextRun.

[dw.im, 2013-11-02 03:13:01]

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/Font.h:159: static unsigned
expansionOpportunityCount(const LChar*, size_t length, TextDirection, bool&
isAfterExpansion, bool distributeJustification = false);
On 2013/11/01 20:39:14, Levi wrote:
> Yech, boolean params! Why not just use the TextJustify enum?

Because.... I only need to check text-justify is distribute or not in the
function.
Textjustify enum could be one choice here.

Will try and see.

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/WidthIterator.cpp (right):

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/WidthIterator.cpp:216: bool needExpansion =
Font::treatAsSpace(character) || m_distributeJustification;
On 2013/11/01 20:39:14, Levi wrote:
> how about something like isExpansionOpportunity instead of needExpansion?

could be better name.. :)

https://codereview.chromium.org/54743004/diff/1/Source/platform/graphics/Text...
File Source/platform/graphics/TextRun.h (right):

https://codereview.chromium.org/54743004/diff/1/Source/platform/graphics/Text...
Source/platform/graphics/TextRun.h:250: bool m_distributeJustification;
On 2013/11/01 20:39:14, Levi wrote:
> Pack this along with the other bits below, otherwise you'll increase the size
of
> TextRun.

will do like this.
+ unsigned m_distributeJustification : 2;

[dw.im, 2013-11-04 02:11:08]

I've uploaded a new patchset for review.

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/Font.h:159: static unsigned
expansionOpportunityCount(const LChar*, size_t length, TextDirection, bool&
isAfterExpansion, bool distributeJustification = false);
On 2013/11/02 03:13:01, dw.im wrote:
> On 2013/11/01 20:39:14, Levi wrote:
> > Yech, boolean params! Why not just use the TextJustify enum?
> 
> Because.... I only need to check text-justify is distribute or not in the
> function.
> Textjustify enum could be one choice here.
> 
> Will try and see.

I used boolean parameter here again, 
because including "core/rendering/style/RenderStyleConstants.h" to use the
TextJustify enum is illegal for this file.
presubmit detect this as Illegal include.

Anyway, I only need to check the value is distribute or not in this function, 
so, how about leaving the boolean parameter with its name so that readers can
know the meaning of the parameter?

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/WidthIterator.cpp (right):

https://codereview.chromium.org/54743004/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/WidthIterator.cpp:216: bool needExpansion =
Font::treatAsSpace(character) || m_distributeJustification;
On 2013/11/02 03:13:01, dw.im wrote:
> On 2013/11/01 20:39:14, Levi wrote:
> > how about something like isExpansionOpportunity instead of needExpansion?
> 
> could be better name.. :)

Done.

https://codereview.chromium.org/54743004/diff/1/Source/platform/graphics/Text...
File Source/platform/graphics/TextRun.h (right):

https://codereview.chromium.org/54743004/diff/1/Source/platform/graphics/Text...
Source/platform/graphics/TextRun.h:250: bool m_distributeJustification;
On 2013/11/02 03:13:01, dw.im wrote:
> On 2013/11/01 20:39:14, Levi wrote:
> > Pack this along with the other bits below, otherwise you'll increase the
size
> of
> > TextRun.
> 
> will do like this.
> + unsigned m_distributeJustification : 2; 

Done.. as bool m_distributeJustification : 1;

[leviw_travelin_and_unemployed, 2013-11-04 19:37:58]

https://codereview.chromium.org/54743004/diff/110001/Source/core/platform/gra...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/54743004/diff/110001/Source/core/platform/gra...
Source/core/platform/graphics/Font.h:160: static unsigned
expansionOpportunityCount(const UChar*, size_t length, TextDirection, bool&
isAfterExpansion, bool distributeJustification = false);
Should we ever call this without passing in a value for distributeJustification?
I see calls to expansionOpportunityCount() in ComplexTextController. Will this
be right?

https://codereview.chromium.org/54743004/diff/110001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/54743004/diff/110001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:1060: opportunitiesInRun =
Font::expansionOpportunityCount(rt->characters8() + r->m_start, r->m_stop -
r->m_start, r->m_box->direction(), isAfterExpansion, (textJustify ==
TextJustifyDistribute));
I still think this is really ugly :-/ If the concept is known to Platform, I'd
rather the concept just be defined there, since this seems like a bandaid to
justify the layering violation.

[dw.im, 2013-11-05 10:26:05]

https://codereview.chromium.org/54743004/diff/110001/Source/core/platform/gra...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/54743004/diff/110001/Source/core/platform/gra...
Source/core/platform/graphics/Font.h:160: static unsigned
expansionOpportunityCount(const UChar*, size_t length, TextDirection, bool&
isAfterExpansion, bool distributeJustification = false);
On 2013/11/04 19:37:58, Levi wrote:
> Should we ever call this without passing in a value for
distributeJustification?
> I see calls to expansionOpportunityCount() in ComplexTextController. Will this
> be right?

It seems ComplexTextController is only belong to mac port.

I think we need to handle the mac port as a seperate patch.
I cannot handle the mac port, so I will leave a FIXME in that file.

https://codereview.chromium.org/54743004/diff/110001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/54743004/diff/110001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:1060: opportunitiesInRun =
Font::expansionOpportunityCount(rt->characters8() + r->m_start, r->m_stop -
r->m_start, r->m_box->direction(), isAfterExpansion, (textJustify ==
TextJustifyDistribute));
On 2013/11/04 19:37:58, Levi wrote:
> I still think this is really ugly :-/ If the concept is known to Platform, I'd
> rather the concept just be defined there, since this seems like a bandaid to
> justify the layering violation.

Hmm...
I'm not sure what could I do for this..

1st option is, declare the enum TextJustify at somewhere in core/platform, and
then include the file in core/rendering.
It looks weird...

2nd option is, declare one more enum at somewhere in core/platform, and, in
crde/rendering, convert TextJustify to the new enum value, and pass it as
parameter.
How about this?
such as.. enum TextJustifyValue { TextJustifyValueInterWord = 0x00,
TextJustifyValueDistribute = 0x01 } 
and, provide converting function, as well.

[leviw_travelin_and_unemployed, 2013-11-05 19:07:24]

https://codereview.chromium.org/54743004/diff/110001/Source/core/platform/gra...
File Source/core/platform/graphics/Font.h (right):

https://codereview.chromium.org/54743004/diff/110001/Source/core/platform/gra...
Source/core/platform/graphics/Font.h:160: static unsigned
expansionOpportunityCount(const UChar*, size_t length, TextDirection, bool&
isAfterExpansion, bool distributeJustification = false);
On 2013/11/05 10:26:10, dw.im wrote:
> On 2013/11/04 19:37:58, Levi wrote:
> > Should we ever call this without passing in a value for
> distributeJustification?
> > I see calls to expansionOpportunityCount() in ComplexTextController. Will
this
> > be right?
> 
> It seems ComplexTextController is only belong to mac port.
> 
> I think we need to handle the mac port as a seperate patch.
> I cannot handle the mac port, so I will leave a FIXME in that file.

Please add a complex text test case and we'll run it through the bots. I want an
example with Arabic to see how we handle connected characters. We'll definitely
need a FIXME and a tracking bug.

https://codereview.chromium.org/54743004/diff/110001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/54743004/diff/110001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:1060: opportunitiesInRun =
Font::expansionOpportunityCount(rt->characters8() + r->m_start, r->m_stop -
r->m_start, r->m_box->direction(), isAfterExpansion, (textJustify ==
TextJustifyDistribute));
On 2013/11/05 10:26:10, dw.im wrote:
> On 2013/11/04 19:37:58, Levi wrote:
> > I still think this is really ugly :-/ If the concept is known to Platform,
I'd
> > rather the concept just be defined there, since this seems like a bandaid to
> > justify the layering violation.
> 
> Hmm...
> I'm not sure what could I do for this..
> 
> 1st option is, declare the enum TextJustify at somewhere in core/platform, and
> then include the file in core/rendering.
> It looks weird...
> 
> 2nd option is, declare one more enum at somewhere in core/platform, and, in
> crde/rendering, convert TextJustify to the new enum value, and pass it as
> parameter.
> How about this?
> such as.. enum TextJustifyValue { TextJustifyValueInterWord = 0x00,
> TextJustifyValueDistribute = 0x01 } 
> and, provide converting function, as well.

I won't block this patch for this, I just think it's ugly. You also don't need
the extra parens.

[dw.im, 2014-01-03 05:56:48]

Dear Levi,
Please check the new patchset.
Thanks!!

[leviw_travelin_and_unemployed, 2014-01-03 19:26:36]

On 2014/01/03 05:56:48, dw.im wrote:
> Dear Levi,
> Please check the new patchset.
> Thanks!!

Hi!

Your patch doesn't seem to compile on Windows, and seems to be failing a lot of
tests, at least on Linux.

[dw.im, 2014-01-07 09:30:35]

So, last thing I need to do is, 
  1. Add the expectation result for mac and windows, or
  2. Modify the TestExpectation so that we can pass for the two ports.

Which way I can choose?
I don't have environment to make the expectations for the two system now, BTW.

[leviw_travelin_and_unemployed, 2014-01-07 18:55:06]

On 2014/01/07 09:30:35, dw.im wrote:
> So, last thing I need to do is, 
>   1. Add the expectation result for mac and windows, or
>   2. Modify the TestExpectation so that we can pass for the two ports.
> 
> Which way I can choose?
> I don't have environment to make the expectations for the two system now, BTW.

There's a NeedsRebaseline keyword in TestExpectations which ignores the current
result. Ojan wrote a bot that automatically grabs the results from the bots and
commits them later, removing the TestExpectations lines you added.

[dw.im, 2014-01-08 23:02:04]

On 2014/01/07 18:55:06, Levi wrote:
> There's a NeedsRebaseline keyword in TestExpectations which ignores the
current
> result. Ojan wrote a bot that automatically grabs the results from the bots
and
> commits them later, removing the TestExpectations lines you added.
>
It is really helpful!
I've revised the patch.

[leviw_travelin_and_unemployed, 2014-01-15 19:17:28]

LGTM.

Typo in change description: "8 bit characters"

https://codereview.chromium.org/54743004/diff/470001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/54743004/diff/470001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:859: # text-justify should be rebaselined
Don't add this comment. The rebaseline bot won't remove it, so it'd just be
orphaned in the file.

[dw.im, 2014-01-16 02:07:45]

On 2014/01/15 19:17:28, Levi wrote:
> LGTM.
> 
> Typo in change description: "8 bit characters"
> 
Done.

>
https://codereview.chromium.org/54743004/diff/470001/LayoutTests/TestExpectat...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/54743004/diff/470001/LayoutTests/TestExpectat...
> LayoutTests/TestExpectations:859: # text-justify should be rebaselined
> Don't add this comment. The rebaseline bot won't remove it, so it'd just be
> orphaned in the file.
Done!

[commit-bot: I haz the power, 2014-01-16 03:47:37]

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

[commit-bot: I haz the power, 2014-01-16 04:02:09]

Change committed as 165189