Use LayoutRect during addFocusRingRects to avoid loss of precision

Use LayoutRect during addFocusRingRects to avoid loss of precision

Previously we use IntRect to store focus ring rects during
addFocusRingRects(). This causes 1-pixel offset or size error when
the result is offsetted again (e.g. when the focused element contains
layers).

Use LayoutRect to avoid errors. Also avoid pixel-snapping when passing
additionalOffset.

BUG=405422
TEST=fast/sub-pixel/focus-ring-around-sub-pixel-layer.html

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

[Julien - ping for review, 2014-08-20 22:12:31]

Some early comments, Levi would probably have more on the general direction (I
wasn't totally convinced this was the best route).

https://codereview.chromium.org/492053002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderInline.cpp (right):

https://codereview.chromium.org/492053002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderInline.cpp:1401: :
toRenderBox(continuation())->location() - containingBlock()->location());
I found the previous structure more readable.

We shouldn't move this function, else we lose the history and get a bigger (more
difficult to review) diff.

https://codereview.chromium.org/492053002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/492053002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1225:
focusRingIntRects.append(pixelSnappedIntRect(focusRingRects[i]));
We talked about this O(N) loop and while it shouldn't be terrible in the common
(N should be 1 most of the time), there is no guarantee that it is.

Nit: Ideally we would want to avoid it (or at least remove the nested one to
compensate).

[Xianzhu, 2014-09-04 00:28:34]

Patchset #2 (id:20001) has been deleted

[Xianzhu, 2014-09-04 00:29:12]

https://codereview.chromium.org/492053002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderInline.cpp (right):

https://codereview.chromium.org/492053002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderInline.cpp:1401: :
toRenderBox(continuation())->location() - containingBlock()->location());
On 2014/08/20 22:12:31, Julien Chaffraix - PST wrote:
> I found the previous structure more readable.
> 
Done.

> We shouldn't move this function, else we lose the history and get a bigger
(more
> difficult to review) diff.

Unfortunately, actually I moved 'class AbsoluteLayoutRectsGeneratorContext' up
because addFocusRingRects uses it.

Fortunately, in the new patch set, because of previous changes, the diff shows
the moving as we expect :)

https://codereview.chromium.org/492053002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/492053002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1225:
focusRingIntRects.append(pixelSnappedIntRect(focusRingRects[i]));
On 2014/08/20 22:12:31, Julien Chaffraix - PST wrote:
> We talked about this O(N) loop and while it shouldn't be terrible in the
common
> (N should be 1 most of the time), there is no guarantee that it is.
> 
> Nit: Ideally we would want to avoid it (or at least remove the nested one to
> compensate).

Previously the computation of the loop was distributed in addFocusRingRects().
Considering the layered path combines rects, the new N <= the old N, so the
total computation won't increase.

[leviw_travelin_and_unemployed, 2014-09-05 20:17:19]

lgtm

https://codereview.chromium.org/492053002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/492053002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1226:
focusRingIntRects.append(pixelSnappedIntRect(focusRingRects[i]));
It's unfortunate we're making an extra data structure just to hold the pixel
snapped versions of these rects, but I understand we don't want to move pixel
snapping logic down into the Graphics Context (at least not yet/with this
patch).

[Xianzhu, 2014-09-05 21:25:07]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-09-05 21:25:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/492053002/40001

[Julien - ping for review, 2014-09-05 21:27:01]

lgtm2

[Xianzhu, 2014-09-05 22:44:09]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-09-05 22:44:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/492053002/60001

[commit-bot: I haz the power, 2014-09-06 04:45:33]

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

[commit-bot: I haz the power, 2014-09-06 04:45:34]

Exceeded time limit waiting for builds to trigger.

[Xianzhu, 2014-09-08 16:03:13]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-09-08 16:04:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/492053002/60001

[commit-bot: I haz the power, 2014-09-08 16:05:30]

Committed patchset #3 (id:60001) as 181567