Find In Page doesn't correctly highlight wrapped text.

Find In Page doesn't correctly highlight wrapped text.

With "white-space: pre-wrap" texts which consist line break,
we get 3 ranges for documentMarker (+ve, +ve) (-ve, +ve) and (-ve, +ve).
So, when we pass -ve offset to SimpleShaper::advance which takes unsigned
value, it converts the -ve offset to very large unsigned value.
Because of this, Font::selectionRectForText "width" comes as "0",
hence no highlight after line break.

Changed SimpleShaper::advance to take int instead of unsigned.

BUG=521047
Please Note: It was regression caused by https://codereview.chromium.org/608413002

[Sk.kumar, 2015-09-04 08:49:24]

sk.kumar@samsung.com changed reviewers:
+ eae@chromium.org, pdr@chromium.org

[Sk.kumar, 2015-09-08 05:18:47]

pdr@, Please Review.

[pdr., 2015-09-08 21:21:25]

https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
File Source/platform/fonts/shaping/SimpleShaper.cpp (right):

https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
Source/platform/fonts/shaping/SimpleShaper.cpp:205: if (m_currentCharacter >=
static_cast<unsigned>(offset))
I think we'll still need to fix this issue which is what eae's patch was trying
to address. For a negative offset, this static_cats will be wrong.

[pdr., 2015-09-08 21:22:36]

On 2015/09/08 at 21:21:25, pdr wrote:
>
https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
> File Source/platform/fonts/shaping/SimpleShaper.cpp (right):
> 
>
https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
> Source/platform/fonts/shaping/SimpleShaper.cpp:205: if (m_currentCharacter >=
static_cast<unsigned>(offset))
> I think we'll still need to fix this issue which is what eae's patch was
trying to address. For a negative offset, this static_cats will be wrong.

err... static_cast

Not static_cats (http://www.sunnyskyz.com/uploads/2014/11/0qt1v-cat-jump.jpg)

[Sk.kumar, 2015-10-06 10:03:06]

On 2015/09/08 21:22:36, pdr wrote:
> On 2015/09/08 at 21:21:25, pdr wrote:
> >
>
https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
> > File Source/platform/fonts/shaping/SimpleShaper.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
> > Source/platform/fonts/shaping/SimpleShaper.cpp:205: if (m_currentCharacter
>=
> static_cast<unsigned>(offset))
> > I think we'll still need to fix this issue which is what eae's patch was
> trying to address. For a negative offset, this static_cats will be wrong.
> 
> err... static_cast
> 
> Not static_cats (http://www.sunnyskyz.com/uploads/2014/11/0qt1v-cat-jump.jpg)

@pdr, Sorry for the late reply.

From C++ Standard 4.7/2, 
If the destination type is unsigned, the resulting value is 
the least unsigned integer congruent to the source integer 
(modulo 2n where n is the number of bits used to represent the unsigned type). 
So, Conversion from int to unsigned is well defined.
for negative numbers pow(2, n) will be added to it to represent it as unsigned.

In the current situation for the wrapped text, we get "-1" value all the time
which 
will be the highest unsigned int value, and i don't see any issue with that. 
So, I feel we can do static_cast on int to make it unsigned here.

Please let me know your opinion.

[pdr., 2015-10-06 17:28:59]

On 2015/10/06 at 10:03:06, sk.kumar wrote:
> On 2015/09/08 21:22:36, pdr wrote:
> > On 2015/09/08 at 21:21:25, pdr wrote:
> > >
> >
https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
> > > File Source/platform/fonts/shaping/SimpleShaper.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
> > > Source/platform/fonts/shaping/SimpleShaper.cpp:205: if (m_currentCharacter
>=
> > static_cast<unsigned>(offset))
> > > I think we'll still need to fix this issue which is what eae's patch was
> > trying to address. For a negative offset, this static_cats will be wrong.
> > 
> > err... static_cast
> > 
> > Not static_cats
(http://www.sunnyskyz.com/uploads/2014/11/0qt1v-cat-jump.jpg)
> 
> @pdr, Sorry for the late reply.
> 
> From C++ Standard 4.7/2, 
> If the destination type is unsigned, the resulting value is 
> the least unsigned integer congruent to the source integer 
> (modulo 2n where n is the number of bits used to represent the unsigned type).

> So, Conversion from int to unsigned is well defined.
> for negative numbers pow(2, n) will be added to it to represent it as
unsigned.
> 
> In the current situation for the wrapped text, we get "-1" value all the time
which 
> will be the highest unsigned int value, and i don't see any issue with that. 
> So, I feel we can do static_cast on int to make it unsigned here.
> 
> Please let me know your opinion.

Wow, I didn't know C++ provides this guarantee.

I think it's okay to go ahead with this change because it's essentially a
revert, but we still do have a bug here because converting -1 to 4294967295 is
not what the code expected, even though it is safe.

LGTM

[wkorman, 2015-10-06 21:50:23]

On 2015/10/06 at 17:28:59, pdr wrote:
> On 2015/10/06 at 10:03:06, sk.kumar wrote:
> > On 2015/09/08 21:22:36, pdr wrote:
> > > On 2015/09/08 at 21:21:25, pdr wrote:
> > > >
> > >
https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
> > > > File Source/platform/fonts/shaping/SimpleShaper.cpp (right):
> > > > 
> > > >
> > >
https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
> > > > Source/platform/fonts/shaping/SimpleShaper.cpp:205: if
(m_currentCharacter >=
> > > static_cast<unsigned>(offset))
> > > > I think we'll still need to fix this issue which is what eae's patch was
> > > trying to address. For a negative offset, this static_cats will be wrong.
> > > 
> > > err... static_cast
> > > 
> > > Not static_cats
(http://www.sunnyskyz.com/uploads/2014/11/0qt1v-cat-jump.jpg)
> > 
> > @pdr, Sorry for the late reply.
> > 
> > From C++ Standard 4.7/2, 
> > If the destination type is unsigned, the resulting value is 
> > the least unsigned integer congruent to the source integer 
> > (modulo 2n where n is the number of bits used to represent the unsigned
type). 
> > So, Conversion from int to unsigned is well defined.
> > for negative numbers pow(2, n) will be added to it to represent it as
unsigned.
> > 
> > In the current situation for the wrapped text, we get "-1" value all the
time which 
> > will be the highest unsigned int value, and i don't see any issue with that.

> > So, I feel we can do static_cast on int to make it unsigned here.
> > 
> > Please let me know your opinion.
> 
> Wow, I didn't know C++ provides this guarantee.
> 
> I think it's okay to go ahead with this change because it's essentially a
revert, but we still do have a bug here because converting -1 to 4294967295 is
not what the code expected, even though it is safe.
> 
> LGTM

See my comment on bug just now -- in my local testing with ToT patched with this
change it does not seem to fix the bug?

[wkorman, 2015-10-06 22:17:40]

On 2015/10/06 at 21:50:23, wkorman wrote:
> See my comment on bug just now -- in my local testing with ToT patched with
this change it does not seem to fix the bug?

I missed a line in applying this patch. I've confirmed this does fix the
regression, but I also agree with pdr@ that presuming eae@'s original api
clarification is accurate re: we should be using unsigned param to advance(), we
should separately do work to restore that.

This test is flaky as written since the text width varies by platform (I had to
change width to 420px I think for it to work on Win7). We should rewrite it to
use Ahem.

[pdr., 2015-10-06 23:07:06]

pdr@chromium.org changed reviewers:
+ wkorman@chromium.org

[pdr., 2015-10-06 23:07:07]

@wkorman, are you okay with filing a separate bug to actually fix the API?

@sk.kumar, can you do one more round of reviews for this one? I agree with
Walter's observation. We just need to update the test to use Ahem and specify a
font size in px to ensure the test works on all platforms.

https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
File Source/platform/fonts/shaping/SimpleShaper.h (right):

https://codereview.chromium.org/1305953003/diff/1/Source/platform/fonts/shapi...
Source/platform/fonts/shaping/SimpleShaper.h:48: unsigned advance(int to,
GlyphBuffer* = 0);
Can you also add a comment that we need to fix this? I went ahead and filed
crbug.com/540047.
// TODO(sk.kumar): This function should be updated to take an unsigned value,
and callers
// should be updated to not pass negative values. See: crbug.com/540047.

(the sk.kumar part doesn't mean you have to fix the bug, it's just a convention
for filing todos)

[wkorman, 2015-10-07 00:45:26]

On 2015/10/06 at 23:07:07, pdr wrote:
> @wkorman, are you okay with filing a separate bug to actually fix the API?
> 
> @sk.kumar, can you do one more round of reviews for this one? I agree with
Walter's observation. We just need to update the test to use Ahem and specify a
font size in px to ensure the test works on all platforms.

Yes, sounds fine. I mailed a patch that has a fixed up test in it, can integrate
into this change.

[Sk.kumar, 2015-10-07 06:10:19]

On 2015/10/07 00:45:26, wkorman wrote:
> On 2015/10/06 at 23:07:07, pdr wrote:
> > @wkorman, are you okay with filing a separate bug to actually fix the API?
> > 
> > @sk.kumar, can you do one more round of reviews for this one? I agree with
> Walter's observation. We just need to update the test to use Ahem and specify
a
> font size in px to ensure the test works on all platforms.
> 
> Yes, sounds fine. I mailed a patch that has a fixed up test in it, can
integrate
> into this change.

@wkorman, I have updated the layout test as per your suggestion.
pdr@, Please review.

[pdr., 2015-10-07 17:50:15]

On 2015/10/07 at 06:10:19, sk.kumar wrote:
> On 2015/10/07 00:45:26, wkorman wrote:
> > On 2015/10/06 at 23:07:07, pdr wrote:
> > > @wkorman, are you okay with filing a separate bug to actually fix the API?
> > > 
> > > @sk.kumar, can you do one more round of reviews for this one? I agree with
> > Walter's observation. We just need to update the test to use Ahem and
specify a
> > font size in px to ensure the test works on all platforms.
> > 
> > Yes, sounds fine. I mailed a patch that has a fixed up test in it, can
integrate
> > into this change.
> 
> @wkorman, I have updated the layout test as per your suggestion.
> pdr@, Please review.

LGTM

[pdr., 2015-10-07 18:39:45]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-10-07 18:40:12]

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

[commit-bot: I haz the power, 2015-10-07 18:41:41]

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

[commit-bot: I haz the power, 2015-10-07 18:41:42]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sk.kumar, 2015-10-07 21:17:19]

The CQ bit was checked by sk.kumar@samsung.com

[commit-bot: I haz the power, 2015-10-07 21:17:28]

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

[commit-bot: I haz the power, 2015-10-07 21:18:29]

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

[commit-bot: I haz the power, 2015-10-07 21:18:30]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)