Fix the x-offset calculation when drawing the text.

Fix the x-offset calculation when drawing the text.

The current calculation assumes positions[0].x() is zero, a run
may start with certain x-offset. It depends on the font settings.
In that case, the entire run might be shifted horizontally.

This is still really hard to be tested in the current code base,
the x-offset value comes from the font settings. Let me leave
the test for the further CL(s).

BUG=456692
R=msw@chromium.org, ckocagil@chromium.org

Committed: https://crrev.com/2bc3c549d9e42787e66f41634c622cf4c7e63f9e
Cr-Commit-Position: refs/heads/master@{#316989}

[msw, 2015-02-14 04:22:34]

https://codereview.chromium.org/923973003/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/923973003/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:963: SkScalar offset_x = preceding_segment_widths
-
Hmm, I wonder if this is actually correct, or if really should just be:
  SkScalar offset_x = preceding_segment_widths;
For single-line, there's one segment per run, so glyphs_range.start() is always
|0|, right? So, this is just adding run.positions[0].x() to the subtraction of
run.positions[0].x()... Can you debug and check if this is ever really untrue?

Otherwise, in the multi-line case, I could see why we'd subtract the position of
the first glyph in the range if the segment began mid-run (it didn't contain the
start of the run), because its run started on a previous line... In that case,
we'd want to keep the subtraction of run.positions[glyphs_range.start()].x(),
but not add the position of the first glyph... so maybe it's:
  SkScalar offset_x = preceding_segment_widths -
    (glyphs_range.start() > 0) ? run.positions[glyphs_range.start()].x() : 0;

Maybe Cem has a better idea of what's going on here...

[Jun Mukai, 2015-02-17 21:24:21]

https://codereview.chromium.org/923973003/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/923973003/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:963: SkScalar offset_x = preceding_segment_widths
-
On 2015/02/14 04:22:34, msw wrote:
> Hmm, I wonder if this is actually correct, or if really should just be:
>   SkScalar offset_x = preceding_segment_widths;
> For single-line, there's one segment per run, so glyphs_range.start() is
always
> |0|, right? So, this is just adding run.positions[0].x() to the subtraction of
> run.positions[0].x()... Can you debug and check if this is ever really untrue?
> 
> Otherwise, in the multi-line case, I could see why we'd subtract the position
of
> the first glyph in the range if the segment began mid-run (it didn't contain
the
> start of the run), because its run started on a previous line... In that case,
> we'd want to keep the subtraction of run.positions[glyphs_range.start()].x(),
> but not add the position of the first glyph... so maybe it's:
>   SkScalar offset_x = preceding_segment_widths -
>     (glyphs_range.start() > 0) ? run.positions[glyphs_range.start()].x() : 0;
> 
> Maybe Cem has a better idea of what's going on here...


Thanks, you are right. My intension was:
- if a segment starts from the beginning of a run, then, we should ignore the x
position because it's canceled below
- if a segment starts from the middle of a run (can happen in multi-line case),
then offset_x should consider the x-posistion from the beginning of the run,
this is essentially run.positions[glyphs_range.start()]

Done as you suggested.

[msw, 2015-02-17 22:28:25]

lgtm if it seems right for RTL (maybe check visually?)

https://codereview.chromium.org/923973003/diff/20001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/923973003/diff/20001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1017: ((glyphs_range.start() != 0) ?
run.positions[glyphs_range.start()].x()
Is this correct for RTL runs (where glyphs_range is reversed?)

[Jun Mukai, 2015-02-17 23:10:21]

New patchsets have been uploaded after l-g-t-m from msw@chromium.org

[Jun Mukai, 2015-02-17 23:10:25]

https://codereview.chromium.org/923973003/diff/20001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/923973003/diff/20001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1017: ((glyphs_range.start() != 0) ?
run.positions[glyphs_range.start()].x()
On 2015/02/17 22:28:25, msw wrote:
> Is this correct for RTL runs (where glyphs_range is reversed?)

Oops, good catch. Used GetMin()

[Jun Mukai, 2015-02-19 02:12:47]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2015-02-19 02:13:10]

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

[commit-bot: I haz the power, 2015-02-19 04:05:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-19 04:06:05]

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