Change Font::offsetForPositionForComplexText to use CachingWordShapeIterator

Change Font::offsetForPositionForComplexText to use CachingWordShapeIterator

In the discussion at a CL for the tab characters support in complex path[1],
it was desired that HarfBuzzShaper being a lower layer of
CachingWordShapeIterator, and higher layers should not use HarfBuzzShaper
directly. It gives us flexibility to move code between these two classes.

This patch changes Font::offsetForPositionForComplexText to switch from
HarfBuzzShaper to CachingWordShapeIterator. This method is currently the
only direct user of HarfBuzzShaper.

A basic test that tests all integer positions using Ahem is added, and
confirmed no behavior changes from the current complex path code, but this
code path has several dependencies (fonts, shaping, graphics layers etc.)
that this patch needs to rebaseline 7 tests (7 Mac, 2 Linux, 0 Windows.)

Among the 7 tests:
* 4 are editing to move carets in block directions:
  - 2 out of 4 have rooms to improve, but very rare (only Mac & vertical & RTL)
  - 2 look reasonable changes (also only Mac & vertical & RTL)
* 3 are ellipsis for RTL, neither before and after look stable.

[1] https://codereview.chromium.org/1242213002/

BUG=422670
TEST=editing/selection/offset-from-point.html

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

[kojii, 2015-07-22 17:27:32]

kojii@chromium.org changed reviewers:
+ eae@chromium.org

[kojii, 2015-07-22 17:27:58]

PTAL.

[eae, 2015-07-22 18:33:22]

Nice, I like the implementation but would like to have a clear one way
relationship where CashingWordShaper and Iterator depend on HarfBuzzShaper and
not the other way around.

https://codereview.chromium.org/1248453004/diff/180001/Source/platform/fonts/...
File Source/platform/fonts/shaping/HarfBuzzShaper.h (right):

https://codereview.chromium.org/1248453004/diff/180001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzShaper.h:71: static int
offsetForPosition(CachingWordShapeIterator&,
How about changing this to take a vector of shape results just like the other
methods instead of adding a dependency on the iterator and we'd be consistent
across methods.

[kojii, 2015-07-23 05:06:12]

PTAL.

https://codereview.chromium.org/1248453004/diff/180001/Source/platform/fonts/...
File Source/platform/fonts/shaping/HarfBuzzShaper.h (right):

https://codereview.chromium.org/1248453004/diff/180001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzShaper.h:71: static int
offsetForPosition(CachingWordShapeIterator&,
On 2015/07/22 at 18:33:22, eae wrote:
> How about changing this to take a vector of shape results just like the other
methods instead of adding a dependency on the iterator and we'd be consistent
across methods.

Makes sense, done.

[kojii, 2015-07-31 04:13:03]

PTAL.

This CL is likely to conflict with fallback fix:
https://codereview.chromium.org/1244973003

but resolving conflicts should not change much and I can handle that after I'm
back.

[eae, 2015-08-03 20:44:52]

LGTM

This is great, thank you!

[kojii, 2015-08-10 03:45:20]

The CQ bit was checked by kojii@chromium.org

[kojii, 2015-08-10 03:45:20]

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/1248453004/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-10 03:45:31]

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

[commit-bot: I haz the power, 2015-08-10 03:49:36]

Committed patchset #14 (id:260001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200233

[brucedawson, 2015-08-11 19:01:05]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2015-08-11 19:01:06]

/analyze pointed out some odd code. Harmless, but worth cleaning up I think.

https://codereview.chromium.org/1248453004/diff/260001/Source/platform/fonts/...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1248453004/diff/260001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:452: RefPtr<ShapeResult>
wordResult;
This variable is never used and should be deleted.

This was detected, perversely enough, through a variable shadowing warning from
the two declarations of 'wordResult' in inner scopes.