Issue 178473024: Convert some Shape code to use references

Issue 178473024: Convert some Shape code to use references (Closed)

Created:
6 years, 9 months ago by maheshkk

Modified:
6 years, 9 months ago

Reviewers:
adamk, rwlbuis

CC:
blink-reviews, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr.

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Convert some Shape code to use references This change makes shapes implementation inline with webkit's. Also using references is lot safe for caller method and also removes extra null checks. Insprired by https://bugs.webkit.org/show_bug.cgi?id=124876 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168585

Patch Set 1 #

Patch Set 2 : Some more reference code #

Total comments: 2

Patch Set 3 : style changes #

Patch Set 4 : rebase with 185393015 #

Total comments: 6

Patch Set 5 : rebased patch #

Created: 6 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+93 lines, -95 lines)			Patch
M	Source/core/rendering/FloatingObjects.cpp	View	1 2 3 4	3 chunks	+8 lines, -8 lines	0 comments	Download
M	Source/core/rendering/LayoutState.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderBlock.h	View	1 2 3 4	1 chunk	+4 lines, -4 lines	0 comments	Download
M	Source/core/rendering/RenderBlock.cpp	View	1 2 3 4	2 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/core/rendering/RenderBlockLineLayout.cpp	View	1 2 3 4	3 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/core/rendering/RenderBox.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderBox.cpp	View	1 2 3 4	3 chunks	+4 lines, -4 lines	0 comments	Download
M	Source/core/rendering/RenderView.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/line/BreakingContextInlineHeaders.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/line/LineWidth.cpp	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/rendering/shapes/ShapeInfo.h	View	1 2 3 4	6 chunks	+21 lines, -21 lines	0 comments	Download
M	Source/core/rendering/shapes/ShapeInfo.cpp	View	1 2 3 4	3 chunks	+9 lines, -9 lines	0 comments	Download
M	Source/core/rendering/shapes/ShapeInsideInfo.h	View	1 2 3 4	3 chunks	+6 lines, -7 lines	0 comments	Download
M	Source/core/rendering/shapes/ShapeInsideInfo.cpp	View	1 2 3 4	4 chunks	+9 lines, -9 lines	0 comments	Download
M	Source/core/rendering/shapes/ShapeOutsideInfo.h	View	1 2 3 4	2 chunks	+7 lines, -8 lines	0 comments	Download
M	Source/core/rendering/shapes/ShapeOutsideInfo.cpp	View	1 2 3 4	4 chunks	+10 lines, -10 lines	0 comments	Download

Messages

Total messages: 14 (0 generated)

Expand Messages | Collapse Messages

rwlbuis

lgtm Have a look at the nits though, also you may need a rebase. https://codereview.chromium.org/178473024/diff/20001/Source/core/rendering/RenderBlock.h ...

6 years, 9 months ago (2014-03-04 14:21:13 UTC) #3

maheshkk

On 2014/03/04 14:21:13, rwlbuis wrote: > lgtm > > Have a look at the nits ...

6 years, 9 months ago (2014-03-04 16:28:33 UTC) #4

adamk

I didn't look at the whole patch, but I noticed a variety of places where ...

6 years, 9 months ago (2014-03-04 19:45:05 UTC) #5

maheshkk

On 2014/03/04 19:45:05, adamk wrote: Thanks Adam for comments. > I didn't look at the ...

6 years, 9 months ago (2014-03-04 19:58:23 UTC) #6

On 2014/03/04 19:45:05, adamk wrote:

Thanks Adam for comments. 

> I didn't look at the whole patch, but I noticed a variety of places where
you've
> changed function or member names, or added new functions. Given that the
> intended purpose of this patch is just to use references instead of pointers,
> those seem like changes that should be in a separate (or several separate)
> patches.
> 
> In general, it's much easier to review patches that have a single aim.

I did not intent to rename/add any new functions with this patch. 
I made a mistake with "patch set 4", My intention was to merge/rebase my changes
with https://codereview.chromium.org/185393015/
and commit my patch when the above patch is committed. But I dint do that right
and endedup including changes from 185393015 in this CL.


> 
>
https://codereview.chromium.org/178473024/diff/60001/Source/core/rendering/Fl...
> File Source/core/rendering/FloatingObjects.cpp (right):
> 
>
https://codereview.chromium.org/178473024/diff/60001/Source/core/rendering/Fl...
> Source/core/rendering/FloatingObjects.cpp:504: if (ShapeOutsideInfo*
> shapeOutside = shapeInfoForFloat(*floatingObject, *m_renderer, m_lineTop,
> m_lineBottom)) {
> It's not clear from this function why you know it's safe to dereference
> floatingObject here. I see from code inspection that logicalRightForFloat()
> already depends on it being non-null, though, so adding an
> ASSERT(floatingObject) is safe and will make this dereference OK.
> 
>
https://codereview.chromium.org/178473024/diff/60001/Source/core/rendering/Fl...
> Source/core/rendering/FloatingObjects.cpp:522: if (ShapeOutsideInfo*
> shapeOutside = shapeInfoForFloat(*floatingObject, *m_renderer, m_lineTop,
> m_lineBottom)) {
> Same comment here as above, an ASSERT(floatingObject) should be added.
> 

Agreed. Will add one.


>
https://codereview.chromium.org/178473024/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderBlock.h (right):
> 
>
https://codereview.chromium.org/178473024/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderBlock.h:230: LayoutSize logicalSizeForChild(const
> RenderBox* child) const { return isHorizontalWritingMode() ? child->size() :
> child->size().transposedSize(); }
> This change seems out of place in this patch, as it has nothing to do with
> changing pointers to references.
> 
>
https://codereview.chromium.org/178473024/diff/60001/Source/core/rendering/sh...
> File Source/core/rendering/shapes/ShapeInfo.h (right):
> 
>
https://codereview.chromium.org/178473024/diff/60001/Source/core/rendering/sh...
> Source/core/rendering/shapes/ShapeInfo.h:71: void
> setReferenceBoxLogicalSize(LayoutSize newReferenceBoxLogicalSize)
> Changing this to take a size seems out of scope for this patch
> 
>
https://codereview.chromium.org/178473024/diff/60001/Source/core/rendering/sh...
> Source/core/rendering/shapes/ShapeInfo.h:112: void markShapeAsDirty() {
> m_shape.clear(); }
> This rename, too, seems out of scope
> 
>
https://codereview.chromium.org/178473024/diff/60001/Source/core/rendering/sh...
> Source/core/rendering/shapes/ShapeInfo.h:122: virtual LayoutBox referenceBox()
> const = 0;
> And this one...

These changes are not part of this CL (refer patch set 3 for reference), I will
get rid of these changes.