Fix unexpected erasing textQuad

Fix unexpected erasing textQuad

LayoutText::absoluteQuadsForRange erases quads which given by the
argument. This is unexpected behavior. To fix this issue when given
quads is not empty, just skip adding bounds of box which is not in
range offset without clearing previous quads.

BUG=707627
Review-Url: https://codereview.chromium.org/2792953002
Cr-Commit-Position: refs/heads/master@{#461681}
Committed: https://chromium.googlesource.com/chromium/src/+/7aab680a20f88c816877a06dcfee33eadb732e1c

[wanchang, 2017-04-03 10:16:26]

wanchang.ryu@lge.com changed reviewers:
+ kojii@chromium.org

[wanchang, 2017-04-03 10:16:26]

PTAL.

[mstensho (USE GERRIT), 2017-04-03 11:16:35]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2017-04-03 11:16:35]

Needs a test.

As for the correctness of the code changes, I cannot really judge. @kojii can,
hopefully. :)

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

https://codereview.chromium.org/2792953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutText.cpp:506: bool
hasCheckedBoxInRange = quads.isEmpty() ? false : true;
bool hasCheckedBoxInRange = !quads.isEmpty();

[wanchang, 2017-04-03 11:43:36]

I also added missing code. I will prepare a test case.
Thanks.

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

https://codereview.chromium.org/2792953002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutText.cpp:506: bool
hasCheckedBoxInRange = quads.isEmpty() ? false : true;
On 2017/04/03 11:16:35, mstensho wrote:
> bool hasCheckedBoxInRange = !quads.isEmpty();

Done.

[kojii, 2017-04-03 12:39:45]

Hmm...I see, this will fix the problem, but this also undoes what you fixed if
|quad| is not empty, correct?

Maybe what you want is to keep the initial size, then use
|rects.resize(initialSize)| rather than |rects.clear()|?

For this level of tests, gtest (the one the other CL was using) might be good.
Are you familiar with it?

[kojii, 2017-04-03 13:03:05]

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

[commit-bot: I haz the power, 2017-04-03 13:03:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2017-04-03 13:07:36]

Oh, you submitted new patch before I replied. This test looks good, so the gtest
I mentioned in #6 is optional, only if you're interested in learning.

And I found that this function is called only from Range::textRects(), which
always call LayoutText::absoluteRectsForRange() in a sequence, so your fix
works.

You could try the fix I mentioned in #6, or add a comment saying this function
is always called in sequence that this check should work. I mildly prefer the
former, WDYT?

[wanchang, 2017-04-03 13:21:25]

On 2017/04/03 12:39:45, kojii wrote:
> Hmm...I see, this will fix the problem, but this also undoes what you fixed if
> |quad| is not empty, correct?
> 
> Maybe what you want is to keep the initial size, then use
> |rects.resize(initialSize)| rather than |rects.clear()|?
> 
> For this level of tests, gtest (the one the other CL was using) might be good.
> Are you familiar with it?

Thanks for comment kojii.
This patch doesn't undo the original issue when |quad| is not empty, just
ignoring range offsets which are not created as inlinebox. This may drop empty
rect of whitespace.

And the idea of keeping the initail size sounds good idea! I will try to
refactor code.

Regarding gtest, where I can create a test for the LayoutText.cpp function ? I
am new for gtest, but I think I can create it.

Thank you.

[kojii, 2017-04-03 13:31:33]

On 2017/04/03 at 13:21:25, wanchang.ryu wrote:
> Thanks for comment kojii.
> This patch doesn't undo the original issue when |quad| is not empty, just
ignoring range offsets which are not created as inlinebox. This may drop empty
rect of whitespace.

I meant, if input |rects| isn't empty, and if the |start| is outside of the
range, we will add quad for outside of the range, but since this function is
only called from |Range::textRects()|, this is not a real concern.

> And the idea of keeping the initail size sounds good idea! I will try to
refactor code.

Sounds good, thank you for keep working on this.

> Regarding gtest, where I can create a test for the LayoutText.cpp function ? I
am new for gtest, but I think I can create it.

We have
Source/core/layout/LayoutTextTest.cpp
this would be ideal place to add the test.

[commit-bot: I haz the power, 2017-04-03 14:38:24]

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

[commit-bot: I haz the power, 2017-04-03 14:38:25]

Dry run: This issue passed the CQ dry run.

[wanchang, 2017-04-04 07:41:00]

Thanks kind comments.

On 2017/04/03 13:31:33, kojii wrote:
> On 2017/04/03 at 13:21:25, wanchang.ryu wrote:
> > Thanks for comment kojii.
> > This patch doesn't undo the original issue when |quad| is not empty, just
> ignoring range offsets which are not created as inlinebox. This may drop empty
> rect of whitespace.
> 
> I meant, if input |rects| isn't empty, and if the |start| is outside of the
> range, we will add quad for outside of the range, but since this function is
> only called from |Range::textRects()|, this is not a real concern.
> 
I just understood what you are mentioned. I think to fix this issue, It needs a
little different approach.
I upload new CL, I think this way it can be solved. This implementation needs a
change on existing layout test
'get-bounding-client-rect-empty-and-non-empty.html'
This test getClientRects with only one character, and len(rects) should be 1 but
this TC assume len(rects) can be greater than 1.

> > And the idea of keeping the initail size sounds good idea! I will try to
> refactor code.
> 
> Sounds good, thank you for keep working on this.
> 
> > Regarding gtest, where I can create a test for the LayoutText.cpp function ?
I
> am new for gtest, but I think I can create it.
> 
> We have
> Source/core/layout/LayoutTextTest.cpp
> this would be ideal place to add the test.
I added a test case on RangeText.cpp and remove layout test, I think, it is more
right place. How do you think ?

PTAL new patchset 4

[kojii, 2017-04-04 08:05:39]

Thank you for working on this, in this depth.

I'd like to separate fixing the broken part from further improvements. That is
safer when new code has an issue, and also we can unblock the patch too.

Could we get back to PS3, land it first, then work on improvements in a separate
CL? Both tests look good, so you can combine the RangeTest from PS4 and the code
change from PS3. Sorry for making this troublesome but I hope you understand to
help other developers.

When you'll be working on further improvements in a separate CL, I'm sorry to
say this but I'm not a big fan of the new approach. This looks like a bit more
complex, will hit the performance by copying to a vector, and will change the
output from visual order to logical order. I don't have clear understanding of
what the problem the new approach is trying to solve, but I hope you find a way
without these side effects.

[wanchang, 2017-04-04 08:18:14]

On 2017/04/04 08:05:39, kojii wrote:
> Thank you for working on this, in this depth.
> 
> I'd like to separate fixing the broken part from further improvements. That is
> safer when new code has an issue, and also we can unblock the patch too.
> 
> Could we get back to PS3, land it first, then work on improvements in a
separate
> CL? Both tests look good, so you can combine the RangeTest from PS4 and the
code
> change from PS3. Sorry for making this troublesome but I hope you understand
to
> help other developers.
> 
> When you'll be working on further improvements in a separate CL, I'm sorry to
> say this but I'm not a big fan of the new approach. This looks like a bit more
> complex, will hit the performance by copying to a vector, and will change the
> output from visual order to logical order. I don't have clear understanding of
> what the problem the new approach is trying to solve, but I hope you find a
way
> without these side effects.

I see, kojii.
I will parepare pachset 3 with RangeTest. Do you prefer to keep layout-test as
well ?
Thanks

[kojii, 2017-04-04 08:26:19]

On 2017/04/04 at 08:18:14, wanchang.ryu wrote:
> I see, kojii.
> I will parepare pachset 3 with RangeTest. Do you prefer to keep layout-test as
well ?

Thank you for your understanding.

Either one of RangeTest or LayoutTest is fine with me, both look great to me.

[wanchang, 2017-04-04 08:31:41]

On 2017/04/04 08:26:19, kojii wrote:
> On 2017/04/04 at 08:18:14, wanchang.ryu wrote:
> > I see, kojii.
> > I will parepare pachset 3 with RangeTest. Do you prefer to keep layout-test
as
> well ?
> 
> Thank you for your understanding.
> 
> Either one of RangeTest or LayoutTest is fine with me, both look great to me.

PTAL

[kojii, 2017-04-04 08:47:00]

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

[commit-bot: I haz the power, 2017-04-04 08:47:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2017-04-04 08:48:50]

lgtm, thank you for your quick work again.

[commit-bot: I haz the power, 2017-04-04 10:30:10]

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

[commit-bot: I haz the power, 2017-04-04 10:30:10]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-04-04 10:56:04]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2017-04-04 10:56:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 11:00:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491303364704740,
"parent_rev": "a75e61beab84fb297e16b2f9cecc9599f61127ab", "commit_rev":
"7aab680a20f88c816877a06dcfee33eadb732e1c"}

[commit-bot: I haz the power, 2017-04-04 11:00:52]

Description was changed from

==========
Fix unexpected erasing textQuad

LayoutText::absoluteQuadsForRange erases quads which given by the
argument. This is unexpected behavior. To fix this issue when given
quads is not empty, just skip adding bounds of box which is not in
range offset without clearing previous quads.

BUG=707627
==========

to

==========
Fix unexpected erasing textQuad

LayoutText::absoluteQuadsForRange erases quads which given by the
argument. This is unexpected behavior. To fix this issue when given
quads is not empty, just skip adding bounds of box which is not in
range offset without clearing previous quads.

BUG=707627

Review-Url: https://codereview.chromium.org/2792953002
Cr-Commit-Position: refs/heads/master@{#461681}
Committed:
https://chromium.googlesource.com/chromium/src/+/7aab680a20f88c816877a06dcfee...
==========

[commit-bot: I haz the power, 2017-04-04 11:00:57]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/7aab680a20f88c816877a06dcfee...