Only synthesize grapheme widths once for surrogate pair characters

Only synthesize grapheme widths once for surrogate pair characters

This patch updates the grapheme width synthesis code to not set
surrogate pairs to be half-width. With this change our rendering
more closely matches Gecko.

There are no pixel differences in svg/text layouttests as a result
of this codechange. This patch does unify the multiple copies of
highlight code which resulted in a small pixel difference in
combining-character-queries.html due to the height of the highlight
text.

BUG=597311
Committed: https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58
Cr-Commit-Position: refs/heads/master@{#384412}

[pdr., 2016-03-31 03:24:31]

pdr@chromium.org changed reviewers:
+ fs@opera.com

[pdr., 2016-03-31 03:26:28]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-31 03:27:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847763002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847763002/1

[pdr., 2016-03-31 03:27:17]

Description was changed from

==========
Do not synthesize grapheme widths for surrogate pair characters

This patch updates the grapheme width synthesis code to not set
surrogate pairs to be half-width. With this change our rendering
more closely matches Gecko.

There are no pixel differences in svg/text layouttests as a result
of this codechange. This patch does unify the multiple copies of
highlight code which resulted in a small pixel difference in
combining-character-queries.html due to the height of the highlight
text.

BUG=597311
==========

to

==========
Only synthesize grapheme widths once for surrogate pair characters

This patch updates the grapheme width synthesis code to not set
surrogate pairs to be half-width. With this change our rendering
more closely matches Gecko.

There are no pixel differences in svg/text layouttests as a result
of this codechange. This patch does unify the multiple copies of
highlight code which resulted in a small pixel difference in
combining-character-queries.html due to the height of the highlight
text.

BUG=597311
==========

[commit-bot: I haz the power, 2016-03-31 07:00:57]

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

[commit-bot: I haz the power, 2016-03-31 07:00:58]

Dry run: This issue passed the CQ dry run.

[fs, 2016-03-31 09:01:42]

LGTM (but also somewhat mindboggling)

https://codereview.chromium.org/1847763002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/svg/text/resources/highlightGlyphs.js
(right):

https://codereview.chromium.org/1847763002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/text/resources/highlightGlyphs.js:7: var
textElements = document.querySelectorAll(".testTextRun");
This shadows the parameter (which is always this anyway...)

https://codereview.chromium.org/1847763002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp
(right):

https://codereview.chromium.org/1847763002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:39: static
bool characterStartsSurrogatePair(const TextRun& run, unsigned index)
Nit: No 'static' needed here (in unnamed namespace). Maybe an 'inline' though?
=)

[pdr., 2016-03-31 18:49:03]

On 2016/03/31 at 09:01:42, fs wrote:
> LGTM (but also somewhat mindboggling)

Hmm, I don't like mindboggling. Do you mean this is different than all of the
approaches we talked about on the bug? If so--I'm hoping this is a bandaid while
our other discussion proceeds on https://github.com/w3c/svgwg/issues/65.

[fs, 2016-03-31 18:53:46]

On 2016/03/31 at 18:49:03, pdr wrote:
> On 2016/03/31 at 09:01:42, fs wrote:
> > LGTM (but also somewhat mindboggling)
> 
> Hmm, I don't like mindboggling. Do you mean this is different than all of the
approaches we talked about on the bug? If so--I'm hoping this is a bandaid while
our other discussion proceeds on https://github.com/w3c/svgwg/issues/65.

I think that was mostly due to the "before coffee" nature of the review =).
(Kept staring at the index manipulations waaay too long.)

[fs, 2016-03-31 18:56:02]

On 2016/03/31 at 18:53:46, fs wrote:
> On 2016/03/31 at 18:49:03, pdr wrote:
> > On 2016/03/31 at 09:01:42, fs wrote:
> > > LGTM (but also somewhat mindboggling)
> > 
> > Hmm, I don't like mindboggling. Do you mean this is different than all of
the approaches we talked about on the bug? If so--I'm hoping this is a bandaid
while our other discussion proceeds on https://github.com/w3c/svgwg/issues/65.
> 
> I think that was mostly due to the "before coffee" nature of the review =).
(Kept staring at the index manipulations waaay too long.)

But, yeah, I think this would be much less complicated not having to care about
splitting into smaller bits...

[pdr., 2016-03-31 20:13:55]

On 2016/03/31 at 18:56:02, fs wrote:
> On 2016/03/31 at 18:53:46, fs wrote:
> > On 2016/03/31 at 18:49:03, pdr wrote:
> > > On 2016/03/31 at 09:01:42, fs wrote:
> > > > LGTM (but also somewhat mindboggling)
> > > 
> > > Hmm, I don't like mindboggling. Do you mean this is different than all of
the approaches we talked about on the bug? If so--I'm hoping this is a bandaid
while our other discussion proceeds on https://github.com/w3c/svgwg/issues/65.
> > 
> > I think that was mostly due to the "before coffee" nature of the review =).
(Kept staring at the index manipulations waaay too long.)
> 
> But, yeah, I think this would be much less complicated not having to care
about splitting into smaller bits...

Tentatively CQing with high hopes of removing this code very soon (but not the
tests!).

[pdr., 2016-03-31 20:14:01]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-03-31 20:14:02]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1847763002/#ps20001
(title: "Address reviewer comments")

[commit-bot: I haz the power, 2016-03-31 20:14:25]

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

[commit-bot: I haz the power, 2016-03-31 22:04:07]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-31 22:05:08]

Description was changed from

==========
Only synthesize grapheme widths once for surrogate pair characters

This patch updates the grapheme width synthesis code to not set
surrogate pairs to be half-width. With this change our rendering
more closely matches Gecko.

There are no pixel differences in svg/text layouttests as a result
of this codechange. This patch does unify the multiple copies of
highlight code which resulted in a small pixel difference in
combining-character-queries.html due to the height of the highlight
text.

BUG=597311
==========

to

==========
Only synthesize grapheme widths once for surrogate pair characters

This patch updates the grapheme width synthesis code to not set
surrogate pairs to be half-width. With this change our rendering
more closely matches Gecko.

There are no pixel differences in svg/text layouttests as a result
of this codechange. This patch does unify the multiple copies of
highlight code which resulted in a small pixel difference in
combining-character-queries.html due to the height of the highlight
text.

BUG=597311

Committed: https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58
Cr-Commit-Position: refs/heads/master@{#384412}
==========

[commit-bot: I haz the power, 2016-03-31 22:05:09]

Patchset 2 (id:??) landed as
https://crrev.com/ec8e7dc866c0edc91cb9c79cd82ea40d2b5e5b58
Cr-Commit-Position: refs/heads/master@{#384412}