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=186099

[leviw_travelin_and_unemployed, 2014-05-12 10:25:21]

This is a re-try from the previous patch, no? It'd be nice if you uploaded the
old patch first so I could see the inter-patch diff. What has changed?

Can you add a link to the old codereview in the description?

[leviw_travelin_and_unemployed, 2014-05-12 10:26:14]

On 2014/05/12 10:25:21, leviw wrote:
> This is a re-try from the previous patch, no? It'd be nice if you uploaded the
> old patch first so I could see the inter-patch diff. What has changed?
> 
> Can you add a link to the old codereview in the description?

I'd also expect a minimized test case from the old use-after-free issue that got
the old patch reverted to be included.

[dw.im, 2014-05-21 01:46:18]

On 2014/05/12 10:26:14, leviw wrote:
> On 2014/05/12 10:25:21, leviw wrote:
> > This is a re-try from the previous patch, no? It'd be nice if you uploaded
the
> > old patch first so I could see the inter-patch diff. What has changed?
> > 
> > Can you add a link to the old codereview in the description?
> 
> I'd also expect a minimized test case from the old use-after-free issue that
got
> the old patch reverted to be included.

sure.
so, I will upload the old patch which was landed as patchset #3,
and will mention about the story and link in the description.

the minimized test case from clusterfuzz? sure, I will.

[dw.im, 2014-06-03 10:16:45]

On 2014/05/12 10:26:14, leviw wrote:
> On 2014/05/12 10:25:21, leviw wrote:
> > This is a re-try from the previous patch, no? It'd be nice if you uploaded
the
> > old patch first so I could see the inter-patch diff. What has changed?
> > 
> > Can you add a link to the old codereview in the description?
> 
> I'd also expect a minimized test case from the old use-after-free issue that
got
> the old patch reverted to be included.

1. Previous patch which has been reverted :
https://codereview.chromium.org/54743004/

2. clusterFuzz issue :
https://cluster-fuzz.appspot.com/testcase?key=5294657378451456
 * Minimized test case 
   * link :
https://cluster-fuzz.appspot.com/viewer?blob_key=AMIfv940qiRUAiLLKYn37iF3pAFs...
   * download :
https://cluster-fuzz.appspot.com/download/AMIfv940qiRUAiLLKYn37iF3pAFsHKoj8l6...

3. Related patch which has been landed :
https://codereview.chromium.org/254803009/

[leviw_travelin_and_unemployed, 2014-06-03 16:08:36]

On 2014/06/03 10:16:45, dw.im wrote:
> On 2014/05/12 10:26:14, leviw wrote:
> > On 2014/05/12 10:25:21, leviw wrote:
> > > This is a re-try from the previous patch, no? It'd be nice if you uploaded
> the
> > > old patch first so I could see the inter-patch diff. What has changed?
> > > 
> > > Can you add a link to the old codereview in the description?
> > 
> > I'd also expect a minimized test case from the old use-after-free issue that
> got
> > the old patch reverted to be included.
> 
> 1. Previous patch which has been reverted :
> https://codereview.chromium.org/54743004/
> 
> 2. clusterFuzz issue :
> https://cluster-fuzz.appspot.com/testcase?key=5294657378451456
>  * Minimized test case 
>    * link :
>
https://cluster-fuzz.appspot.com/viewer?blob_key=AMIfv940qiRUAiLLKYn37iF3pAFs...
>    * download :
>
https://cluster-fuzz.appspot.com/download/AMIfv940qiRUAiLLKYn37iF3pAFsHKoj8l6...
> 
> 3. Related patch which has been landed :
> https://codereview.chromium.org/254803009/

I'm not sure you understand what I'm asking for. If you first uploaded the old
patch, then uploaded the new patch on the same issue, I could diff the two
patches against each other to see what you've changed. I'm not looking to
download the old test case, I'm looking for you to minimize it and upload it as
a LayoutTest for this issue to ensure this patchset doesn't have the same issue
as the last one.

[dw.im, 2014-06-09 02:25:23]

On 2014/06/03 16:08:36, leviw wrote:

> I'm not sure you understand what I'm asking for. If you first uploaded the old
> patch, then uploaded the new patch on the same issue, I could diff the two
> patches against each other to see what you've changed. I'm not looking to
> download the old test case, I'm looking for you to minimize it and upload it
as
> a LayoutTest for this issue to ensure this patchset doesn't have the same
issue
> as the last one.

Yes, I did understand, but previous patch couldn't be applied on current source
tree due to the new files - e.g. Character.*

Anyway, I know the diff - there is only one diff I mentioned above, and one
patch.
So, will create patches with/without the diff, and upload respective order, so
that you can see the diff.

[dw.im, 2014-06-09 07:52:11]

On 2014/06/09 02:25:23, dw.im wrote:
> On 2014/06/03 16:08:36, leviw wrote:
> 
> > I'm not sure you understand what I'm asking for. If you first uploaded the
old
> > patch, then uploaded the new patch on the same issue, I could diff the two
> > patches against each other to see what you've changed. I'm not looking to
> > download the old test case, I'm looking for you to minimize it and upload it
> as
> > a LayoutTest for this issue to ensure this patchset doesn't have the same
> issue
> > as the last one.
> 
> Yes, I did understand, but previous patch couldn't be applied on current
source
> tree due to the new files - e.g. Character.*
> 
> Anyway, I know the diff - there is only one diff I mentioned above, and one
> patch.
> So, will create patches with/without the diff, and upload respective order, so
> that you can see the diff.

Done - patchset #3 & #4.
Also, please also refer the related patch -
https://codereview.chromium.org/254803009/ .

PTAL

[ebraminio, 2014-06-26 12:25:04]

On 2014/06/09 07:52:11, dw.im wrote:
> On 2014/06/09 02:25:23, dw.im wrote:
> > On 2014/06/03 16:08:36, leviw wrote:
> > 
> > > I'm not sure you understand what I'm asking for. If you first uploaded the
> old
> > > patch, then uploaded the new patch on the same issue, I could diff the two
> > > patches against each other to see what you've changed. I'm not looking to
> > > download the old test case, I'm looking for you to minimize it and upload
it
> > as
> > > a LayoutTest for this issue to ensure this patchset doesn't have the same
> > issue
> > > as the last one.
> > 
> > Yes, I did understand, but previous patch couldn't be applied on current
> source
> > tree due to the new files - e.g. Character.*
> > 
> > Anyway, I know the diff - there is only one diff I mentioned above, and one
> > patch.
> > So, will create patches with/without the diff, and upload respective order,
so
> > that you can see the diff.
> 
> Done - patchset #3 & #4.
> Also, please also refer the related patch -
> https://codereview.chromium.org/254803009/ .
> 
> PTAL

Just a reminder PTAL :) Apparently whole source-code there is reformatted after
that patch revert and putting old patch first was not possible.

We are long waiting for `text-align-last', even webkit now has it now and this
patch is prerequisite for it http://crbug.com/248839#c11

[leviw_travelin_and_unemployed, 2014-07-28 23:59:58]

Super close!

https://codereview.chromium.org/255323004/diff/50001/LayoutTests/fast/css3-te...
File LayoutTests/fast/css3-text/css3-text-justify/text-justify-8bits.html
(right):

https://codereview.chromium.org/255323004/diff/50001/LayoutTests/fast/css3-te...
LayoutTests/fast/css3-text/css3-text-justify/text-justify-8bits.html:10: Every
block has been setted as 'text-align: justify' and 'text-align-last: start'.
nit: "set" not "setted"

https://codereview.chromium.org/255323004/diff/50001/LayoutTests/fast/css3-te...
LayoutTests/fast/css3-text/css3-text-justify/text-justify-8bits.html:16: Lorem
ipsum dolor sit amet, consectetuer adipiscing elit. Donec odio. Quisque volutpat
mattis eros. Nullam malesuada erat ut turpis. Suspendisse urna nibh, viverra
non, semper suscipit, posuere a, pede.
It be a good idea to have at least 2 words in the last line.

https://codereview.chromium.org/255323004/diff/50001/Source/platform/fonts/Ch...
File Source/platform/fonts/Character.cpp (right):

https://codereview.chromium.org/255323004/diff/50001/Source/platform/fonts/Ch...
Source/platform/fonts/Character.cpp:278: if (direction == LTR) {
This codepath can be simplified now. We only need one iteration, and just need
to change the index we use to check "isAfterExpansion"

https://codereview.chromium.org/255323004/diff/50001/Source/platform/fonts/Ch...
File Source/platform/fonts/Character.h (right):

https://codereview.chromium.org/255323004/diff/50001/Source/platform/fonts/Ch...
Source/platform/fonts/Character.h:52: static unsigned
expansionOpportunityCount(const UChar*, size_t length, TextDirection, bool&
isAfterExpansion, const bool distributeJustification);
Can we use an enum instead of an extra bool? :-/

https://codereview.chromium.org/255323004/diff/50001/Source/platform/fonts/ma...
File Source/platform/fonts/mac/ComplexTextController.cpp (right):

https://codereview.chromium.org/255323004/diff/50001/Source/platform/fonts/ma...
Source/platform/fonts/mac/ComplexTextController.cpp:79: ,
m_distributeJustification(false)
Why are you making this a member variable if you only pass it on to
Character::expansionOpportunityCount? You only need a local variable.

[dw.im, 2014-07-29 01:54:54]

https://codereview.chromium.org/255323004/diff/50001/Source/platform/fonts/Ch...
File Source/platform/fonts/Character.cpp (right):

https://codereview.chromium.org/255323004/diff/50001/Source/platform/fonts/Ch...
Source/platform/fonts/Character.cpp:278: if (direction == LTR) {
On 2014/07/28 23:59:58, leviw wrote:
> This codepath can be simplified now. We only need one iteration, and just need
> to change the index we use to check "isAfterExpansion"

If the value of text-justify is distribute, then, yes, we can simplify it.
We only need to know how many characters we have, which we already know before
starting the iteration.
If we do, then we can have simple algorithm, and also some performance gain as
well.

I will try to do it.

https://codereview.chromium.org/255323004/diff/50001/Source/platform/fonts/Ch...
File Source/platform/fonts/Character.h (right):

https://codereview.chromium.org/255323004/diff/50001/Source/platform/fonts/Ch...
Source/platform/fonts/Character.h:52: static unsigned
expansionOpportunityCount(const UChar*, size_t length, TextDirection, bool&
isAfterExpansion, const bool distributeJustification);
On 2014/07/28 23:59:58, leviw wrote:
> Can we use an enum instead of an extra bool? :-/

I guess so. And, using enum also can give chance to enhance when we have more
values of text-justify.
I will try to do it.

[dw.im, 2014-08-08 08:54:45]

Hi, Levi,
I tried to use enum, rather than creating a new bool, as you suggested.
PTAL.

[leviw_travelin_and_unemployed, 2014-08-11 17:34:47]

https://codereview.chromium.org/255323004/diff/70001/Source/platform/text/Tex...
File Source/platform/text/TextRun.cpp (right):

https://codereview.chromium.org/255323004/diff/70001/Source/platform/text/Tex...
Source/platform/text/TextRun.cpp:37: uint32_t bitfields : 10;
It looks like we're only actually using 8 right now.

https://codereview.chromium.org/255323004/diff/70001/Source/platform/text/Tex...
File Source/platform/text/TextRun.h (right):

https://codereview.chromium.org/255323004/diff/70001/Source/platform/text/Tex...
Source/platform/text/TextRun.h:249: ETextJustify m_textJustify;
You'll want to pack this into the bits above (there's plenty of room). You can
*store* it as an unsigned.

[dw.im, 2014-08-12 02:47:09]

https://codereview.chromium.org/255323004/diff/70001/Source/platform/text/Tex...
File Source/platform/text/TextRun.cpp (right):

https://codereview.chromium.org/255323004/diff/70001/Source/platform/text/Tex...
Source/platform/text/TextRun.cpp:37: uint32_t bitfields : 10;
On 2014/08/11 17:34:46, leviw wrote:
> It looks like we're only actually using 8 right now.

If we pack m_textJustify into two bits, it become 10 again.

https://codereview.chromium.org/255323004/diff/70001/Source/platform/text/Tex...
File Source/platform/text/TextRun.h (right):

https://codereview.chromium.org/255323004/diff/70001/Source/platform/text/Tex...
Source/platform/text/TextRun.h:249: ETextJustify m_textJustify;
On 2014/08/11 17:34:46, leviw wrote:
> You'll want to pack this into the bits above (there's plenty of room). You can
> *store* it as an unsigned.

Done.

[leviw_travelin_and_unemployed, 2014-08-18 17:55:43]

https://codereview.chromium.org/255323004/diff/90001/Source/platform/fonts/Ch...
File Source/platform/fonts/Character.cpp (right):

https://codereview.chromium.org/255323004/diff/90001/Source/platform/fonts/Ch...
Source/platform/fonts/Character.cpp:283: if (treatAsSpace(characters[length -
1]))
int lastCharacter = (direction == ltr) ? length - 1 : 0
isAfterExpansion = treatAsSpace(characters[lastCharacter);

https://codereview.chromium.org/255323004/diff/90001/Source/platform/text/Tex...
File Source/platform/text/TextRun.h (right):

https://codereview.chromium.org/255323004/diff/90001/Source/platform/text/Tex...
Source/platform/text/TextRun.h:46: JustifyAuto = 0x00,
You don't need 2 digits after the x. 0x1, 0x2, etc.

These aren't exactly flags, either. Can we move TextJustify to platform instead
of doubling it here?

[dw.im, 2014-08-21 00:20:55]

Thanks for the review!

https://codereview.chromium.org/255323004/diff/90001/Source/platform/fonts/Ch...
File Source/platform/fonts/Character.cpp (right):

https://codereview.chromium.org/255323004/diff/90001/Source/platform/fonts/Ch...
Source/platform/fonts/Character.cpp:283: if (treatAsSpace(characters[length -
1]))
On 2014/08/18 17:55:43, leviw wrote:
> int lastCharacter = (direction == ltr) ? length - 1 : 0
> isAfterExpansion = treatAsSpace(characters[lastCharacter);

Cool!

https://codereview.chromium.org/255323004/diff/90001/Source/platform/text/Tex...
File Source/platform/text/TextRun.h (right):

https://codereview.chromium.org/255323004/diff/90001/Source/platform/text/Tex...
Source/platform/text/TextRun.h:46: JustifyAuto = 0x00,
On 2014/08/18 17:55:43, leviw wrote:
> You don't need 2 digits after the x. 0x1, 0x2, etc.
> 
> These aren't exactly flags, either. Can we move TextJustify to platform
instead
> of doubling it here?

I'm not sure we can move it. will try to look into it.

[dw.im, 2014-09-13 15:47:42]

Hi, levi, PTAL!

[leviw_travelin_and_unemployed, 2014-11-05 23:23:39]

Very close.

https://codereview.chromium.org/255323004/diff/150001/Source/core/css/CSSPrim...
File Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/255323004/diff/150001/Source/core/css/CSSPrim...
Source/core/css/CSSPrimitiveValueMappings.h:50: #include
"platform/text/TextRun.h"
This shouldn't be necessary.

https://codereview.chromium.org/255323004/diff/150001/Source/platform/text/Te...
File Source/platform/text/TextRun.h (right):

https://codereview.chromium.org/255323004/diff/150001/Source/platform/text/Te...
Source/platform/text/TextRun.h:46: TextJustifyAuto = 0x0,
You actually don't need to set these values.

https://codereview.chromium.org/255323004/diff/150001/Source/platform/text/Te...
Source/platform/text/TextRun.h:52: typedef unsigned TextJustifyStore;
You don't need this. Look at how RenderObject handles PaintInvalidationReason as
an unsigned in RenderObject, for example.

https://codereview.chromium.org/255323004/diff/150001/Source/platform/text/Te...
Source/platform/text/TextRun.h:213: TextJustifyStore textJustifyStore() const {
return m_textJustifyStore; }
How about just textJustify()/setTextJustify?

https://codereview.chromium.org/255323004/diff/150001/Source/platform/text/Te...
Source/platform/text/TextRun.h:249: unsigned m_textJustifyStore : 2;
How about just "m_textJustify"?

[dw.im, 2014-11-10 10:18:53]

Thanks Levi for the review!
I will upload a new patch soon!

[dw.im, 2014-11-19 02:06:55]

Dear Levi! PTAL!

This patch doesn't include 'LayoutTests/TestExpectations' to rebaseline, because
it always make conflict.
I will add the change after getting review from you.

[leviw_travelin_and_unemployed, 2014-11-26 19:04:13]

lgtm.

[dw.im, 2014-11-27 04:29:41]

The CQ bit was checked by dw.im@samsung.com

[commit-bot: I haz the power, 2014-11-27 04:30:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/255323004/190001

[commit-bot: I haz the power, 2014-11-27 05:41:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-27 05:41:10]

CQ experienced an internal error when committing your CL and the maintainers
were notified. Sorry for the inconvenience.

[commit-bot: I haz the power, 2014-11-27 05:41:15]

The CQ bit was unchecked by commit-bot@chromium.org

[dw.im, 2014-11-27 08:05:59]

The CQ bit was checked by dw.im@samsung.com

[commit-bot: I haz the power, 2014-11-27 08:06:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/255323004/190001

[commit-bot: I haz the power, 2014-11-27 08:06:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-27 08:06:42]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1402.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
b767e5f6a090d7df09b5146859b541c3526a42b3..8cc7bb1e8ec30850eaf4c33824e3369bbee95148
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1402,3 +1402,5 @@ crbug.com/435342 media/video-seeking.html [ Pass Failure ]
 crbug.com/435525 [ Mac Debug ] svg/W3C-SVG-1.1/struct-frag-02-t.svg [
NeedsManualRebaseline ]
 
 crbug.com/436436 fast/dom/HTMLImageElement/image-srcset-w-onerror.html [ Pass
Failure ]
+
+crbug.com/248894 fast/css3-text/css3-text-justify/text-justify-8bits.html [
NeedsRebaseline ]

[dw.im, 2014-11-27 08:21:02]

The CQ bit was checked by dw.im@samsung.com

[commit-bot: I haz the power, 2014-11-27 08:22:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/255323004/210001

[commit-bot: I haz the power, 2014-11-27 09:38:28]

Committed patchset #12 (id:210001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186099