Use references in RenderBlock and RenderBlockFlow methods

Use references in RenderBlock and RenderBlockFlow methods

RenderBlock and RenderBlockFlow has several methods that take pointers
but those methods assumes that the objects will never be NULL. We
should replace them by references. This makes the caller code safer.

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

[Sunil Ratnu, 2014-11-26 03:43:02]

sunil.ratnu@samsung.com changed reviewers:
+ jchaffraix@chromium.org, rob.buis@samsung.com

[Sunil Ratnu, 2014-11-26 03:43:03]

Please review this.

Thanks!
Sunil

[rwlbuis, 2014-11-26 17:07:57]

On 2014/11/26 03:43:03, Sunil Ratnu wrote:
> Please review this.
> 
> Thanks!
> Sunil

Is it based on a WebKit change?

[Sunil Ratnu, 2014-11-26 17:24:55]

On 2014/11/26 17:07:57, rwlbuis wrote:
> On 2014/11/26 03:43:03, Sunil Ratnu wrote:
> > Please review this.
> > 
> > Thanks!
> > Sunil
> 
> Is it based on a WebKit change?

No, its not based on any webkit change. I just observed that the pointers were
not null anywhere, so better to use references instead.

[Julien - ping for review, 2014-11-26 18:52:36]

The change lgtm but the line drawn seems arbitrary: a lot of the callers to the
function need to be converted to reference too. I would prefer if that should
happen as part of this change to avoid versioning churn but could live with
separate patches to ease landing.

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

https://codereview.chromium.org/752723004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:504: void
RenderBlockFlow::determineLogicalLeftPositionForChild(RenderBox* child)
This function could also take a reference.

https://codereview.chromium.org/752723004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:512: LayoutUnit childMarginStart =
marginStartForChild(*child);
It seems like the function could also use a reference.

https://codereview.chromium.org/752723004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:565: LayoutUnit oldLogicalTop =
logicalTopForChild(*child);
Same comment.

https://codereview.chromium.org/752723004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:1142: afterMargin =
marginAfterForChild(*child);
Ditto.

https://codereview.chromium.org/752723004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:1273: setLogicalHeight(logicalHeight()
+ separateMargin + marginBeforeForChild(*child));
Ditto.

https://codereview.chromium.org/752723004/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlockFlow.h (right):

https://codereview.chromium.org/752723004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.h:215: return child->x() +
marginBeforeForChild(*(child->renderer()));
We should make FloatingObject return a reference too.

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

https://codereview.chromium.org/752723004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderTable.cpp:351: void
RenderTable::layoutCaption(RenderTableCaption* caption)
Same comment.

[Sunil Ratnu, 2014-11-27 12:02:55]

Thanks for the review Julien.

I had thought of only making the changes in RenderBlock for simplicity but now
I've included RenderBlockFlow as well which now covers almost all your suggested
points.

https://codereview.chromium.org/752723004/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlockFlow.h (right):

https://codereview.chromium.org/752723004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.h:215: return child->x() +
marginBeforeForChild(*(child->renderer()));
On 2014/11/26 18:52:35, Julien Chaffraix - PST wrote:
> We should make FloatingObject return a reference too.

Lets keep FloatingObject conversion in a separate CL as it involves a lot of
caller sites.

[Sunil Ratnu, 2014-11-27 12:11:30]

The CQ bit was checked by sunil.ratnu@samsung.com

[commit-bot: I haz the power, 2014-11-27 12:11:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/752723004/20001

[commit-bot: I haz the power, 2014-11-27 13:13:05]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186114