Return the right top position for text selections when there are no previous root inline boxes.

Return the right top position for text selections when there are no previous root inline boxes.

The code in RootInlineBox::selectionTop() was doing some extra calculations based on border and
padding to determine the top position of a text selection for a RootInlineBox when there is no
previous box to consider, instead of just returning the m_lineTop value (which is similar to
what is correctly being done already now in RootInlineBox::selectionBottom()).

This caused that the top position for a selection was usually miscalculated, returning to high
values when the line-height value is too big, and too small values when line-height is too
small, causing rendering issues both when drawing cursors and selection rectangles.

BUG=313593
TEST=Try selecting text over lines with too big and too small values for line-height.
R=eae@chromium.org

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

[mario.prada, 2014-01-20 17:19:54]

Hi,

Could you please take a look to this patch? It seems to fix the issue as
described in [1], but I'm not 100% yet whether this is the right approach.

Btw, I also added two new layout tests but some rebaselines might be needed for
other tests if this gets in. This is due to the fact that those other tests are
currently expecting the "wrong output" (too big/small selection rectangle or
cursor), so expectations would need to be updated.

Still, I thought it would be better to leave that potential rebaseline out of
the patch set for now.

Thanks,
Mario

[1] https://code.google.com/p/chromium/issues/detail?id=313593

[eae, 2014-01-21 18:19:20]

On 2014/01/20 17:19:54, mario.prada wrote:
> Hi,
> 
> Could you please take a look to this patch? It seems to fix the issue as
> described in [1], but I'm not 100% yet whether this is the right approach.
> 
> Btw, I also added two new layout tests but some rebaselines might be needed
for
> other tests if this gets in. This is due to the fact that those other tests
are
> currently expecting the "wrong output" (too big/small selection rectangle or
> cursor), so expectations would need to be updated.
> 
> Still, I thought it would be better to leave that potential rebaseline out of
> the patch set for now.
> 
> Thanks,
> Mario
> 
> [1] https://code.google.com/p/chromium/issues/detail?id=313593

Seems entirely reasonable to me.

LGTM

[mario.prada, 2014-01-22 15:16:49]

On 2014/01/21 18:19:20, eae wrote:
> On 2014/01/20 17:19:54, mario.prada wrote:
> > Hi,
> > 
> > Could you please take a look to this patch? It seems to fix the issue as
> > described in [1], but I'm not 100% yet whether this is the right approach.
> > 
> > Btw, I also added two new layout tests but some rebaselines might be needed
> for
> > other tests if this gets in. This is due to the fact that those other tests
> are
> > currently expecting the "wrong output" (too big/small selection rectangle or
> > cursor), so expectations would need to be updated.
> > 
> > Still, I thought it would be better to leave that potential rebaseline out
of
> > the patch set for now.
> > 
> > Thanks,
> > Mario
> > 
> > [1] https://code.google.com/p/chromium/issues/detail?id=313593
> 
> Seems entirely reasonable to me.
> 
> LGTM


Thanks for the review. Unfortunately I haven't got committers right yet (this is
actually my first patch to blink), so I'd appreaciate if you could help as well
with the try bots and the commit-bot thing.

Thanks!

[mario.prada, 2014-01-24 17:33:55]

On 2014/01/22 15:16:49, mario.prada wrote:
> [...]
> Thanks for the review. Unfortunately I haven't got committers right yet (this
is
> actually my first patch to blink), so I'd appreaciate if you could help as
well
> with the try bots and the commit-bot thing.
> 
> Thanks!

Ping reviewers?

[eae, 2014-01-24 18:04:00]

On 2014/01/24 17:33:55, mario.prada wrote:
> On 2014/01/22 15:16:49, mario.prada wrote:
> > [...]
> > Thanks for the review. Unfortunately I haven't got committers right yet
(this
> is
> > actually my first patch to blink), so I'd appreaciate if you could help as
> well
> > with the try bots and the commit-bot thing.
> > 
> > Thanks!
> 
> Ping reviewers?

Are you under an individual or company CLA?

[mario.prada, 2014-01-27 09:31:02]

On 2014/01/24 18:04:00, eae wrote:

Under a Company CLA, included with my <nickname>@samsung.com email address.

It should be listed in the http://goto/cla-signers link that can be found in
[1], I guess. I can't check that myself, though.

[1]
http://www.chromium.org/developers/contributing-code/external-contributor-che...

[mario.prada, 2014-01-29 17:52:43]

On 2014/01/27 09:31:02, mario.prada wrote:
> On 2014/01/24 18:04:00, eae wrote:
> 
> Under a Company CLA, included with my <mailto:nickname>@samsung.com email
address.
> 
> It should be listed in the http://goto/cla-signers link that can be found in
> [1], I guess. I can't check that myself, though.
> 
> [1]
>
http://www.chromium.org/developers/contributing-code/external-contributor-che...

Sorry for pinging again, but I would appreciate some help moving this forward
:).

I'll be attending FOSDEM from Friday to Sunday and I would love to be able to
work tomorrow on this if possible, otherwise I will have to wait until the next
week. Problem is that I'm not sure I can do much now without knowing the results
in the try bots (I expect some rebaselines will be needed), so please let me
know in case there is something else that is needed from my side to get this
moving forward.

Thanks!

[eae, 2014-01-29 21:12:50]

I'm afraid I cannot find any record of you having signed the SLA, could you tell
me the exact email address it would be under (either here or by e-mailing me
directly)?

Sorry for the red tape but we really do need to keep track of this.

[eae, 2014-01-30 18:05:47]

LGTM, sorry about the delay!

[commit-bot: I haz the power, 2014-01-30 18:43:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/141683003/1

[commit-bot: I haz the power, 2014-01-30 22:01:54]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-30 22:02:02]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 22:02:03]

CQ bit was unchecked on CL. Ignoring.

[mario.prada, 2014-02-06 11:49:57]

Marking tests failing for linux as NeedsRebaseline and updating one reftest

[eae, 2014-02-14 10:38:03]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-02-14 10:38:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/141683003/120001

[commit-bot: I haz the power, 2014-02-14 12:21:58]

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

[commit-bot: I haz the power, 2014-02-14 12:21:59]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[mario.prada, 2014-02-14 15:31:41]

Added more tests as NeedsRebaseline which were not detected on Linux builds

[mario.prada, 2014-02-17 17:23:21]

Added [ NeedsRebaseline ] for foreignObject-text-clipping-bug.xml on Windows

[mario.prada, 2014-02-17 17:24:50]

On 2014/02/17 17:23:21, mario.prada wrote:
> Added [ NeedsRebaseline ] for foreignObject-text-clipping-bug.xml on Windows

I didn't realized that this was needed because it's being marked as WontFix for
Linux in StaleTestExpectations (see http://crbug.com/248065)

[mario.prada, 2014-02-24 17:02:57]

On 2014/02/17 17:24:50, mario.prada wrote:
> On 2014/02/17 17:23:21, mario.prada wrote:
> > Added [ NeedsRebaseline ] for foreignObject-text-clipping-bug.xml on Windows
> 
> I didn't realized that this was needed because it's being marked as WontFix
for
> Linux in StaleTestExpectations (see http://crbug.com/248065)

On 2014/02/14 16:37:25, mario.prada wrote:
> Rebased patch against master

By looking at the output of the faiing bots in different tries, I don't really
think the current failures are related to this patch.

Any suggestion on how to move forward? (this patch has been uploaded here for a
while already)

Thanks

[eae, 2014-02-24 19:55:49]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-02-24 19:56:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/141683003/420001

[commit-bot: I haz the power, 2014-02-24 23:06:32]

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

[commit-bot: I haz the power, 2014-02-24 23:06:33]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[mario.prada, 2014-02-25 14:42:59]

Added [ NeedsRebaseline ] for editing/selection/move-left-right.html on Windows

[mario.prada, 2014-02-25 15:36:16]

The CQ bit was checked by mario.prada@samsung.com

[commit-bot: I haz the power, 2014-02-25 15:38:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/141683003/630001

[mario.prada, 2014-02-25 16:42:43]

The CQ bit was unchecked by mario.prada@samsung.com

[mario.prada, 2014-02-25 16:44:22]

Added [ NeedsRebaseline ] for editing/selection/move-left-right.html

[mario.prada, 2014-02-25 16:48:27]

The CQ bit was checked by mario.prada@samsung.com

[commit-bot: I haz the power, 2014-02-25 16:48:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mario.prada@samsung.com/141683003/650001

[commit-bot: I haz the power, 2014-02-25 19:12:24]

Change committed as 167813