Issue 1303413004: Convert some call sites to use explicit LayoutRect->FloatRect conversion.

Issue 1303413004: Convert some call sites to use explicit LayoutRect->FloatRect conversion. (Closed)

Created:
5 years, 4 months ago by chrishtr

Modified:
5 years, 4 months ago

Reviewers:
eae

CC:
apavlov+blink_chromium.org, blink-layers+watch_chromium.org, blink-reviews, blink-reviews-paint_chromium.org, blink-reviews-rendering, caseq+blink_chromium.org, devtools-reviews_chromium.org, dshwang, krit, eae+blinkwatch, f(malita), fs, gyuyoung2, jchaffraix+rendering, kenneth.christiansen, kouhei+svg_chromium.org, kozyatinskiy+blink_chromium.org, leviw+renderwatch, lushnikov+blink_chromium.org, pdr+svgwatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, pfeldman+blink_chromium.org, rwlbuis, Stephen Chennney, sergeyv+blink_chromium.org, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, yurys+blink_chromium.org, zoltan1

Base URL:
svn://svn.chromium.org/blink/trunk

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Convert some call sites to use explicit LayoutRect->FloatRect conversion. This is in preparation for making the FloatRect constructor from LayoutRect explicit. BUG=524259 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201168

Patch Set 1 #

Total comments: 8

Patch Set 2 : #

Patch Set 3 : #

Created: 5 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+43 lines, -37 lines)			Patch
M	Source/core/editing/Caret.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/editing/Editor.cpp	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/editing/markers/DocumentMarkerControllerTest.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/frame/RootFrameViewport.cpp	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/core/inspector/InspectorTraceEvents.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/HitTestLocation.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/LayoutBlock.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/LayoutBoxModelObject.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/LayoutInline.cpp	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/layout/LayoutObject.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/LayoutPart.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/compositing/CompositingInputsUpdater.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/line/AbstractInlineTextBox.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/line/RootInlineBox.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/svg/LayoutSVGText.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/page/TouchAdjustment.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/page/scrolling/ScrollingCoordinator.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/paint/BlockFlowPainter.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/paint/BoxBorderPainter.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/paint/BoxPainter.cpp	View		2 chunks	+5 lines, -3 lines	0 comments	Download
M	Source/core/paint/DeprecatedPaintLayerClipper.cpp	View	2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/paint/HTMLCanvasPainter.cpp	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	Source/core/paint/ImagePainter.cpp	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	Source/core/paint/InlineFlowBoxPainter.cpp	View		3 chunks	+6 lines, -3 lines	0 comments	Download
M	Source/core/paint/InlineTextBoxPainter.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/testing/Internals.cpp	View		2 chunks	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 17 (6 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

eae

https://codereview.chromium.org/1303413004/diff/1/Source/core/editing/markers/DocumentMarkerControllerTest.cpp File Source/core/editing/markers/DocumentMarkerControllerTest.cpp (right): https://codereview.chromium.org/1303413004/diff/1/Source/core/editing/markers/DocumentMarkerControllerTest.cpp#newcode204 Source/core/editing/markers/DocumentMarkerControllerTest.cpp:204: IntRect invalidRect(RenderedDocumentMarker::create(DocumentMarker(0, 0, false))->renderedRect()); renderRect() returns a LayoutRect, this ...

5 years, 4 months ago (2015-08-25 17:47:17 UTC) #3

chrishtr

5 years, 4 months ago (2015-08-25 18:30:44 UTC) #4

eae

LGTM On Tue, Aug 25, 2015 at 11:30 AM, <chrishtr@chromium.org> wrote: > > https://codereview.chromium.org/1303413004/diff/1/Source/core/editing/markers/DocumentMarkerControllerTest.cpp > ...

5 years, 4 months ago (2015-08-25 18:35:58 UTC) #5

LGTM

On Tue, Aug 25, 2015 at 11:30 AM,  <chrishtr@chromium.org> wrote:
>
>
https://codereview.chromium.org/1303413004/diff/1/Source/core/editing/markers...
> File Source/core/editing/markers/DocumentMarkerControllerTest.cpp
> (right):
>
>
https://codereview.chromium.org/1303413004/diff/1/Source/core/editing/markers...
> Source/core/editing/markers/DocumentMarkerControllerTest.cpp:204:
> IntRect invalidRect(RenderedDocumentMarker::create(DocumentMarker(0, 0,
> false))->renderedRect());
> On 2015/08/25 at 17:47:16, eae wrote:
>>
>> renderRect() returns a LayoutRect, this implicitly down converts that
>
> to an IntRect which doesn't seem right.
>
>> Explicitly calling pixelSnappedIntRect or enclosingIntRect would be
>
> better.
>
> It's implicit in the same way that renderedRectsForMarkers is implicit,
> because it builds a vector of IntRects that are constructed by calling
> results.append() with a LayoutRect:

Ok

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
>
> Should there be a followup task to remove the IntRect constructor that
> takes in a LayoutRect?

Seems like a reasonable change given that LayoutRect->IntRect not only
looses precision but is ambiguous (does it truncate, snap or expand?)

>
>
https://codereview.chromium.org/1303413004/diff/1/Source/core/layout/LayoutIn...
> File Source/core/layout/LayoutInline.cpp (right):
>
>
https://codereview.chromium.org/1303413004/diff/1/Source/core/layout/LayoutIn...
> Source/core/layout/LayoutInline.cpp:1029: FloatRect
> rect(curr->clippedOverflowRectForPaintInvalidation(view()));
> On 2015/08/25 at 17:47:16, eae wrote:
>>
>> Why are we using a FloatRect here given that the clippedOverflorRect
>
> is a LayoutRect and we return a LayoutRect?
>
> The context() function on line 1030 requires it. I changed to convert
> just for that call.

Much better, thanks!

>
https://codereview.chromium.org/1303413004/diff/1/Source/core/layout/LayoutPa...
> File Source/core/layout/LayoutPart.cpp (right):
>
>
https://codereview.chromium.org/1303413004/diff/1/Source/core/layout/LayoutPa...
> Source/core/layout/LayoutPart.cpp:300: LayoutRect
>
absoluteContentBox(localToAbsoluteQuad(FloatQuad(FloatRect(contentBox))).boundingBox());
> On 2015/08/25 at 17:47:17, eae wrote:
>>
>> Might be easier to add a FloatQuad constructor that takes a
>
> LayoutRect? I'll leave that up to you though.
>
> I'd prefer not to, to make the performance more obvious to future
> developers.

Ok, that makes sense.

>
>
https://codereview.chromium.org/1303413004/diff/1/Source/core/paint/Deprecate...
> File Source/core/paint/DeprecatedPaintLayerClipper.cpp (right):
>
>
https://codereview.chromium.org/1303413004/diff/1/Source/core/paint/Deprecate...
> Source/core/paint/DeprecatedPaintLayerClipper.cpp:98: if
> (IntRect(clipRect) == LayoutRect::infiniteIntRect())
> On 2015/08/25 at 17:47:17, eae wrote:
>>
>> This looks odd, why would we compare a LayoutRect against
>
> InfinitIntRect? Could you file a bug and/or add a TODO comment here?
>
> The presence of this weird code is already tracked in crbug.com/440143.
> See the definition of LayoutRect::infiniteRect().

Great, thanks!

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.