Elements whose contents start with an astral Unicode symbol disappear when CSS `::first-letter` is …

Elements whose contents start with an astral Unicode symbol disappear when CSS `::first-letter` is applied to them.

This CL is inspired by http://trac.webkit.org/changeset/172513
written by Myles C. Maxfield.
The previous code assumed that all "characters" are exactly 1 16-bit code unit wide.
Instead, use numCharactersInGraphemeClusters().

For blink, we add countCharactersAndGraphemesInCluster to bring a similar
functionality to blink. This api is based on countGraphemesInCluster and improved
to figure out the number of characters and grapheme at the same time in the given clusters.

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

[changseok, 2015-05-14 17:13:05]

shivamidow@gmail.com changed reviewers:
+ eae@chromium.org, esprehn@chromium.org, leviw@chromium.org

[changseok, 2015-05-14 17:13:06]

Please review this.

[eae, 2015-05-14 23:52:48]

https://codereview.chromium.org/1133853006/diff/1/Source/platform/text/TextBr...
File Source/platform/text/TextBreakIterator.cpp (right):

https://codereview.chromium.org/1133853006/diff/1/Source/platform/text/TextBr...
Source/platform/text/TextBreakIterator.cpp:53: unsigned
numCharactersInGraphemeClusters(const String& string, unsigned
numGraphemeClusters)
We already have an implementation of this in HarfBuzzShaper,
countGraphemesInCluster. How about we move that implementation into
TextBreakIterator and make it a static function? That way the implementation can
be shared.

[changseok, 2015-05-15 17:08:56]

https://codereview.chromium.org/1133853006/diff/1/Source/platform/text/TextBr...
File Source/platform/text/TextBreakIterator.cpp (right):

https://codereview.chromium.org/1133853006/diff/1/Source/platform/text/TextBr...
Source/platform/text/TextBreakIterator.cpp:53: unsigned
numCharactersInGraphemeClusters(const String& string, unsigned
numGraphemeClusters)
On 2015/05/14 23:52:48, eae wrote:
> We already have an implementation of this in HarfBuzzShaper,
> countGraphemesInCluster. How about we move that implementation into
> TextBreakIterator and make it a static function? That way the implementation
can
> be shared.

Sorry, I can't follow this meaning. Do you want anything like..
static unsigned countGraphemesInCluster()
{
    ....
}

unsigned numCharactersInGraphemeClutters()
{
    ....
    return countGraphemesInClusters();
}

??

[changseok, 2015-05-18 14:52:55]

On 2015/05/14 23:52:48, eae wrote:
>
https://codereview.chromium.org/1133853006/diff/1/Source/platform/text/TextBr...
> File Source/platform/text/TextBreakIterator.cpp (right):
> 
>
https://codereview.chromium.org/1133853006/diff/1/Source/platform/text/TextBr...
> Source/platform/text/TextBreakIterator.cpp:53: unsigned
> numCharactersInGraphemeClusters(const String& string, unsigned
> numGraphemeClusters)
> We already have an implementation of this in HarfBuzzShaper,
> countGraphemesInCluster. How about we move that implementation into
> TextBreakIterator and make it a static function? That way the implementation
can
> be shared.

As my understanding, the functionality of countGraphemesInClusters is a little
bit different from that of numCharactersInGraphemeClusters.
countGraphemesInCluster counts how many 'graphemes' are in a cluster, but
numCharactersInGraphemeClusters counts how many 'characters' construct the given
graphemes.
i,g countGraphemesInCluster() for a special character '𝌆' returns 1, but 2 from
numCharactersInGraphemeClusters.

[changseok, 2015-05-25 06:28:20]

On 2015/05/18 14:52:55, changseok wrote:
> On 2015/05/14 23:52:48, eae wrote:
> >
>
https://codereview.chromium.org/1133853006/diff/1/Source/platform/text/TextBr...
> > File Source/platform/text/TextBreakIterator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1133853006/diff/1/Source/platform/text/TextBr...
> > Source/platform/text/TextBreakIterator.cpp:53: unsigned
> > numCharactersInGraphemeClusters(const String& string, unsigned
> > numGraphemeClusters)
> > We already have an implementation of this in HarfBuzzShaper,
> > countGraphemesInCluster. How about we move that implementation into
> > TextBreakIterator and make it a static function? That way the implementation
> can
> > be shared.
> 
> As my understanding, the functionality of countGraphemesInClusters is a little
> bit different from that of numCharactersInGraphemeClusters.
> countGraphemesInCluster counts how many 'graphemes' are in a cluster, but
> numCharactersInGraphemeClusters counts how many 'characters' construct the
given
> graphemes.
> i,g countGraphemesInCluster() for a special character '𝌆' returns 1, but 2
from
> numCharactersInGraphemeClusters.

Comment please?

[eae, 2015-05-26 22:40:06]

True, they both handle graphnemes in clusters though and I'd like to see the
logic resused. How about it it returns both?

[changseok, 2015-06-01 17:47:47]

On 2015/05/26 22:40:06, eae wrote:
> True, they both handle graphnemes in clusters though and I'd like to see the
> logic resused. How about it it returns both?

I'm not sure if I'm following your thought correctly though.. I get stuck in the
endIndex for countCharactersAndGraphemesInCluster. I need your help.

numCharactersInGraphemeClusters and countGraphemesInCluster are different a
little bit. The main difference is their iterator.
numCharactersInGraphemeClusters uses icu::BreakIterator, on the other hand,
countGraphemesInCluster uses icu::RuleBasedBreakIterator which is a inherited
class from BreakIterator. If we replace the numCharactersInGraphemeClutters()
with countGraphemesInCluster(), then we need to set startIndex and endIndex. But
I don't know here how I can figure out the endIndex for a special character..
Any idea?

https://codereview.chromium.org/1133853006/diff/40001/Source/core/dom/FirstLe...
File Source/core/dom/FirstLetterPseudoElement.cpp (right):

https://codereview.chromium.org/1133853006/diff/40001/Source/core/dom/FirstLe...
Source/core/dom/FirstLetterPseudoElement.cpp:85: length +=
countCharactersAndGraphemesInCluster(substring.characters16(),
substring.length(), 0, 1).characters;
Here the endIndex 1 doesn't work for a special character. It should be 2 but I
don't know how I get it or where I can get it from..

[changseok, 2015-06-08 08:27:12]

On 2015/06/01 17:47:47, changseok wrote:
> On 2015/05/26 22:40:06, eae wrote:
> > True, they both handle graphnemes in clusters though and I'd like to see the
> > logic resused. How about it it returns both?
> 
> I'm not sure if I'm following your thought correctly though.. I get stuck in
the
> endIndex for countCharactersAndGraphemesInCluster. I need your help.
> 
> numCharactersInGraphemeClusters and countGraphemesInCluster are different a
> little bit. The main difference is their iterator.
> numCharactersInGraphemeClusters uses icu::BreakIterator, on the other hand,
> countGraphemesInCluster uses icu::RuleBasedBreakIterator which is a inherited
> class from BreakIterator. If we replace the numCharactersInGraphemeClutters()
> with countGraphemesInCluster(), then we need to set startIndex and endIndex.
But
> I don't know here how I can figure out the endIndex for a special character..
> Any idea?
> 
>
https://codereview.chromium.org/1133853006/diff/40001/Source/core/dom/FirstLe...
> File Source/core/dom/FirstLetterPseudoElement.cpp (right):
> 
>
https://codereview.chromium.org/1133853006/diff/40001/Source/core/dom/FirstLe...
> Source/core/dom/FirstLetterPseudoElement.cpp:85: length +=
> countCharactersAndGraphemesInCluster(substring.characters16(),
> substring.length(), 0, 1).characters;
> Here the endIndex 1 doesn't work for a special character. It should be 2 but I
> don't know how I get it or where I can get it from..

eae@ Could you comment a little bit more for me?

[changseok, 2015-06-16 09:24:24]

The CL is updated. Comment please?

[eae, 2015-06-16 16:30:18]

This creates two extra copies of the string, is that really necessary? Sorry for
being picky but this code is _very_ performance sensitive.

[changseok, 2015-06-17 02:18:11]

On 2015/06/16 16:30:18, eae wrote:
> This creates two extra copies of the string, is that really necessary? Sorry
for
> being picky but this code is _very_ performance sensitive.

I see. I changed the cl to make it re-use the countGraphemesInCluster interface.
With the change, I think the extra copies are avoided.
Feel free to leave your comment. I alway welcome any comment. =)

[changseok, 2015-06-23 06:50:42]

On 2015/06/17 02:18:11, changseok wrote:
> On 2015/06/16 16:30:18, eae wrote:
> > This creates two extra copies of the string, is that really necessary? Sorry
> for
> > being picky but this code is _very_ performance sensitive.
> 
> I see. I changed the cl to make it re-use the countGraphemesInCluster
interface.
> With the change, I think the extra copies are avoided.
> Feel free to leave your comment. I alway welcome any comment. =)

@eae, would you see this again?

[changseok, 2015-08-03 05:26:02]

Hello? I'd like to make this happen. Would you comment on the latest patch? ''a

[leviw_travelin_and_unemployed, 2015-08-03 20:05:20]

leviw@chromium.org changed reviewers:
+ drott@chromium.org

[leviw_travelin_and_unemployed, 2015-08-03 20:05:20]

eae has been out on vacation. Adding drott.

[eae, 2015-08-03 20:42:25]

LGTM

[changseok, 2015-08-04 05:33:40]

The CQ bit was checked by shivamidow@gmail.com

[changseok, 2015-08-04 05:33:41]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1133853006/#ps120001
(title: "Rebase and resolve conflicts")

[commit-bot: I haz the power, 2015-08-04 05:33:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133853006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1133853006/120001

[commit-bot: I haz the power, 2015-08-04 05:48:11]

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

[commit-bot: I haz the power, 2015-08-04 05:48:12]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/72464)

[changseok, 2015-08-04 05:58:14]

The CQ bit was checked by shivamidow@gmail.com

[changseok, 2015-08-04 05:58:14]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1133853006/#ps140001
(title: "Change unsinged variable to int to make win compiler happy.")

[commit-bot: I haz the power, 2015-08-04 05:58:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133853006/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1133853006/140001

[commit-bot: I haz the power, 2015-08-04 06:59:17]

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

[commit-bot: I haz the power, 2015-08-04 06:59:18]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/72470)

[changseok, 2015-08-09 05:18:49]

The CQ bit was checked by shivamidow@gmail.com

[changseok, 2015-08-09 05:18:49]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1133853006/#ps160001
(title: "Fixed layout test failures.")

[commit-bot: I haz the power, 2015-08-09 05:18:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1133853006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1133853006/160001

[commit-bot: I haz the power, 2015-08-09 06:39:07]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200227

[sof, 2015-08-09 12:54:45]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1277803004/ by sigbjornf@opera.com.

The reason for reverting is: ASan picking up a number of UAF failures due to
this one,


http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/bu....

[changseok, 2015-08-10 04:12:22]

On 2015/08/09 12:54:45, sof wrote:
> A revert of this CL (patchset #9 id:160001) has been created in
> https://codereview.chromium.org/1277803004/ by mailto:sigbjornf@opera.com.
> 
> The reason for reverting is: ASan picking up a number of UAF failures due to
> this one,
> 
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/bu....

Sorry for the inconvenience. I think the UAF failures are fixed, but I don't
know how I can land an updated patch again.
Is it enough to submit the patch here, or do I need to create a new ticket for
it?

[drott, 2015-09-02 07:50:37]

On 2015/08/10 04:12:22, changseok wrote:
> On 2015/08/09 12:54:45, sof wrote:
> > A revert of this CL (patchset #9 id:160001) has been created in
> > https://codereview.chromium.org/1277803004/ by mailto:sigbjornf@opera.com.
> > 
> > The reason for reverting is: ASan picking up a number of UAF failures due to
> > this one,
> > 
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/bu....
> 
> Sorry for the inconvenience. I think the UAF failures are fixed, but I don't
> know how I can land an updated patch again.
> Is it enough to submit the patch here, or do I need to create a new ticket for
> it?

Sorry for the delay in answering to your question. If you have additional
improvements to this CL, please create a new CL, describe how you addressed the
issues that lead to the revert, and call it something like "Reland of: Fix CSS
::first-letter for UTF-16 surrogate pairs" and please ask for review again.

[changseok, 2015-09-02 07:59:11]

On 2015/09/02 07:50:37, drott wrote:
> On 2015/08/10 04:12:22, changseok wrote:
> > On 2015/08/09 12:54:45, sof wrote:
> > > A revert of this CL (patchset #9 id:160001) has been created in
> > > https://codereview.chromium.org/1277803004/ by mailto:sigbjornf@opera.com.
> > > 
> > > The reason for reverting is: ASan picking up a number of UAF failures due
to
> > > this one,
> > > 
> > > 
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/bu....
> > 
> > Sorry for the inconvenience. I think the UAF failures are fixed, but I don't
> > know how I can land an updated patch again.
> > Is it enough to submit the patch here, or do I need to create a new ticket
for
> > it?
> 
> Sorry for the delay in answering to your question. If you have additional
> improvements to this CL, please create a new CL, describe how you addressed
the
> issues that lead to the revert, and call it something like "Reland of: Fix CSS
> ::first-letter for UTF-16 surrogate pairs" and please ask for review again.

Thanks for the answer. I'll do it soon.

[drott, 2016-02-02 15:36:39]

drott@chromium.org changed reviewers:
- drott@chromium.org