Convert RenderBlockFlow code to use FloatingObject references

Convert RenderBlockFlow code to use FloatingObject references

RenderBlockFlow has several methods that take FloatingObject pointers
but those methods assumes that the objects will never be NULL. We
should replace them by references. This makes the caller code safer.
Also this CL removes dead code which is never used.

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

[Sunil Ratnu, 2014-11-28 08:42:47]

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

[Sunil Ratnu, 2014-11-28 08:42:47]

Please review this.

Thanks!
Sunil

[Julien - ping for review, 2014-12-01 16:12:47]

It really seem like we should fix the iteration to use reference, that would
make the change more natural IMHO.

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/Floati...
File Source/core/rendering/FloatingObjects.cpp (right):

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/Floati...
Source/core/rendering/FloatingObjects.cpp:215: 
We should use a reference here. The best way is to add a new function to OwnPtr
(getReference?).

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/Floati...
Source/core/rendering/FloatingObjects.cpp:230: 
Ditto.

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

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:962: 
Ditto.

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/line/L...
File Source/core/rendering/line/LineWidth.cpp (right):

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/line/L...
Source/core/rendering/line/LineWidth.cpp:63: void
LineWidth::shrinkAvailableWidthForNewFloatIfNeeded(FloatingObject* newFloat)
We could use a reference here too.

[rwlbuis, 2015-01-27 13:51:09]

On 2014/11/28 08:42:47, Sunil Ratnu wrote:
> Please review this.
> 
> Thanks!
> Sunil

Hi Sunil, do you want to finish this one?

[Sunil Ratnu, 2015-01-27 13:54:15]

Thanks for asking Rob. Have been doing a few other things lately, I will update
this patch soon and then we can land it.

[Sunil Ratnu, 2015-03-16 08:06:30]

Hi Julien & Rob,

Sorry for delay in updating the patch as I was busy with a few other things.
I've made the changes as suggested by Julien. Please review the latest patch.

Thanks & Regards,
Sunil

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/Floati...
File Source/core/rendering/FloatingObjects.cpp (right):

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/Floati...
Source/core/rendering/FloatingObjects.cpp:215: 
On 2014/12/01 16:12:47, Julien Chaffraix - PST wrote:
> We should use a reference here. The best way is to add a new function to
OwnPtr
> (getReference?).

Done. I've added a new method getReference() in OwnPtr.

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/Floati...
Source/core/rendering/FloatingObjects.cpp:230: 
On 2014/12/01 16:12:47, Julien Chaffraix - PST wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:962: 
On 2014/12/01 16:12:47, Julien Chaffraix - PST wrote:
> Ditto.

Done.

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/line/L...
File Source/core/rendering/line/LineWidth.cpp (right):

https://codereview.chromium.org/763173003/diff/1/Source/core/rendering/line/L...
Source/core/rendering/line/LineWidth.cpp:63: void
LineWidth::shrinkAvailableWidthForNewFloatIfNeeded(FloatingObject* newFloat)
On 2014/12/01 16:12:47, Julien Chaffraix - PST wrote:
> We could use a reference here too.

Done.

[Julien - ping for review, 2015-03-16 15:27:54]

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Float...
File Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Float...
Source/core/layout/FloatingObjects.cpp:475: ASSERT(interval.low() ==
m_renderer->pixelSnappedLogicalTopForFloat(*floatingObject));
It seems like |floatingObject| should be a reference.

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:1785: FloatingObject* floatingObject =
it->get();
This should be a const reference.

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:2277: floatLogicalWidth =
std::min(logicalWidthForFloat(*floatingObject), logicalRightOffset -
logicalLeftOffset);
|floatingObject| should be a reference here too. I can sympathize with trying to
keep this patch small but I fear inconsistencies if we don't land all the pieces
together.

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:2337:
setLogicalWidthForFloat(*newObj.get(), logicalWidthForChild(floatBox) +
marginStartForChild(floatBox) + marginEndForChild(floatBox));
*newObj should work and is shorter and safer (there is a nullptr check in
OwnPtr::operator*).

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
File Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlowLine.cpp:917:
appendFloatingObjectToLastLine(*f);
|f| should be a reference (and probably renamed to |floatingObject|).

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlowLine.cpp:2007: LayoutUnit floatLogicalTop =
logicalTopForFloat(*newFloat);
Again |newFloat| looks like it should be a reference here too.

https://codereview.chromium.org/763173003/diff/20001/Source/wtf/OwnPtr.h
File Source/wtf/OwnPtr.h (right):

https://codereview.chromium.org/763173003/diff/20001/Source/wtf/OwnPtr.h#newc...
Source/wtf/OwnPtr.h:59: ValueType& getReference() const { ASSERT(m_ptr); return
*m_ptr; };
Note that this is basically operator* so it seems like using it in the callers
above would solve our problem without the need for a new function.

[Sunil Ratnu, 2015-03-17 11:31:19]

Hi Julien,

Thanks for the review. I've made changes suggested by you. Please have a look at
the latest patchset.

Thanks!
Sunil

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Float...
File Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Float...
Source/core/layout/FloatingObjects.cpp:475: ASSERT(interval.low() ==
m_renderer->pixelSnappedLogicalTopForFloat(*floatingObject));
On 2015/03/16 15:27:54, Julien Chaffraix - PST wrote:
> It seems like |floatingObject| should be a reference.

Done.

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:1785: FloatingObject* floatingObject =
it->get();
On 2015/03/16 15:27:54, Julien Chaffraix - PST wrote:
> This should be a const reference.

Done.

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:2277: floatLogicalWidth =
std::min(logicalWidthForFloat(*floatingObject), logicalRightOffset -
logicalLeftOffset);
On 2015/03/16 15:27:54, Julien Chaffraix - PST wrote:
> |floatingObject| should be a reference here too. I can sympathize with trying
to
> keep this patch small but I fear inconsistencies if we don't land all the
pieces
> together.

Done.

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:2337:
setLogicalWidthForFloat(*newObj.get(), logicalWidthForChild(floatBox) +
marginStartForChild(floatBox) + marginEndForChild(floatBox));
On 2015/03/16 15:27:54, Julien Chaffraix - PST wrote:
> *newObj should work and is shorter and safer (there is a nullptr check in
> OwnPtr::operator*).

Done.

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
File Source/core/layout/LayoutBlockFlowLine.cpp (right):

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlowLine.cpp:917:
appendFloatingObjectToLastLine(*f);
On 2015/03/16 15:27:54, Julien Chaffraix - PST wrote:
> |f| should be a reference (and probably renamed to |floatingObject|).

Done.

https://codereview.chromium.org/763173003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlowLine.cpp:2007: LayoutUnit floatLogicalTop =
logicalTopForFloat(*newFloat);
On 2015/03/16 15:27:54, Julien Chaffraix - PST wrote:
> Again |newFloat| looks like it should be a reference here too.

Done.

https://codereview.chromium.org/763173003/diff/20001/Source/wtf/OwnPtr.h
File Source/wtf/OwnPtr.h (right):

https://codereview.chromium.org/763173003/diff/20001/Source/wtf/OwnPtr.h#newc...
Source/wtf/OwnPtr.h:59: ValueType& getReference() const { ASSERT(m_ptr); return
*m_ptr; };
On 2015/03/16 15:27:54, Julien Chaffraix - PST wrote:
> Note that this is basically operator* so it seems like using it in the callers
> above would solve our problem without the need for a new function.

Done. Now using operator* itself instead of the new function.

[Sunil Ratnu, 2015-03-17 11:40:47]

Patchset #3 (id:40001) has been deleted

[Julien - ping for review, 2015-03-18 01:36:08]

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
File Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
Source/core/layout/FloatingObjects.cpp:215: FloatingObject& floatingObject =
*it->get();
It seems like this could be a const FloatingObject&.

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
Source/core/layout/FloatingObjects.cpp:230: FloatingObject& floatingObject =
*it->get();
Ditto.

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
Source/core/layout/FloatingObjects.cpp:368:
m_placedFloatsTree.add(intervalForFloatingObject(&floatingObject));
It seems like intervalForFloatingObject should take a reference. Unfortunately I
don't think we can change the data structure to use references instead of
pointers.

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:894: FloatingObject& floatingObject =
*it->get();
const FloatingObject here too.

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:959: FloatingObject& floatingObject =
*it->get();
Can this one be const FloatingObject&?

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:2348: FloatingObject& floatingObject =
*it->get();
const FloatingObject&? (I will not repeat this comment but it's good to evaluate
all our use of FloatingObject's for constness)

[Sunil Ratnu, 2015-06-04 08:40:47]

Hi Julien,

I've made the changes suggested by you. Apologies for being late while following
up with the comments. This latest patch makes sure that we use const with
floatingObjects wherever possible. Please review this when you have time.

Thanks!
Sunil

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
File Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
Source/core/layout/FloatingObjects.cpp:215: FloatingObject& floatingObject =
*it->get();
On 2015/03/18 01:36:07, Julien Chaffraix - PST wrote:
> It seems like this could be a const FloatingObject&.

Done.

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
Source/core/layout/FloatingObjects.cpp:230: FloatingObject& floatingObject =
*it->get();
On 2015/03/18 01:36:07, Julien Chaffraix - PST wrote:
> Ditto.

Done.

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
Source/core/layout/FloatingObjects.cpp:368:
m_placedFloatsTree.add(intervalForFloatingObject(&floatingObject));
On 2015/03/18 01:36:07, Julien Chaffraix - PST wrote:
> It seems like intervalForFloatingObject should take a reference. Unfortunately
I
> don't think we can change the data structure to use references instead of
> pointers.

Done.

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
File Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:894: FloatingObject& floatingObject =
*it->get();
On 2015/03/18 01:36:08, Julien Chaffraix - PST wrote:
> const FloatingObject here too.

Done.

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:959: FloatingObject& floatingObject =
*it->get();
On 2015/03/18 01:36:08, Julien Chaffraix - PST wrote:
> Can this one be const FloatingObject&?

Yes, it can be. Made it a const.

https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.cpp:2348: FloatingObject& floatingObject =
*it->get();
On 2015/03/18 01:36:07, Julien Chaffraix - PST wrote:
> const FloatingObject&? (I will not repeat this comment but it's good to
evaluate
> all our use of FloatingObject's for constness)

We cant have const here because of the remove() method at the end which takes
FLoatingObject* as argument.

[Sunil Ratnu, 2015-06-10 09:07:21]

On 2015/06/04 08:40:47, Sunil Ratnu wrote:
> Hi Julien,
> 
> I've made the changes suggested by you. Apologies for being late while
following
> up with the comments. This latest patch makes sure that we use const with
> floatingObjects wherever possible. Please review this when you have time.
> 
> Thanks!
> Sunil
> 
>
https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
> File Source/core/layout/FloatingObjects.cpp (right):
> 
>
https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
> Source/core/layout/FloatingObjects.cpp:215: FloatingObject& floatingObject =
> *it->get();
> On 2015/03/18 01:36:07, Julien Chaffraix - PST wrote:
> > It seems like this could be a const FloatingObject&.
> 
> Done.
> 
>
https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
> Source/core/layout/FloatingObjects.cpp:230: FloatingObject& floatingObject =
> *it->get();
> On 2015/03/18 01:36:07, Julien Chaffraix - PST wrote:
> > Ditto.
> 
> Done.
> 
>
https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Float...
> Source/core/layout/FloatingObjects.cpp:368:
> m_placedFloatsTree.add(intervalForFloatingObject(&floatingObject));
> On 2015/03/18 01:36:07, Julien Chaffraix - PST wrote:
> > It seems like intervalForFloatingObject should take a reference.
Unfortunately
> I
> > don't think we can change the data structure to use references instead of
> > pointers.
> 
> Done.
> 
>
https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
> File Source/core/layout/LayoutBlockFlow.cpp (right):
> 
>
https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
> Source/core/layout/LayoutBlockFlow.cpp:894: FloatingObject& floatingObject =
> *it->get();
> On 2015/03/18 01:36:08, Julien Chaffraix - PST wrote:
> > const FloatingObject here too.
> 
> Done.
> 
>
https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
> Source/core/layout/LayoutBlockFlow.cpp:959: FloatingObject& floatingObject =
> *it->get();
> On 2015/03/18 01:36:08, Julien Chaffraix - PST wrote:
> > Can this one be const FloatingObject&?
> 
> Yes, it can be. Made it a const.
> 
>
https://codereview.chromium.org/763173003/diff/50001/Source/core/layout/Layou...
> Source/core/layout/LayoutBlockFlow.cpp:2348: FloatingObject& floatingObject =
> *it->get();
> On 2015/03/18 01:36:07, Julien Chaffraix - PST wrote:
> > const FloatingObject&? (I will not repeat this comment but it's good to
> evaluate
> > all our use of FloatingObject's for constness)
> 
> We cant have const here because of the remove() method at the end which takes
> FLoatingObject* as argument.

ping!

[Julien - ping for review, 2015-06-10 15:15:45]

Sorry about the delay. lgtm

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/Float...
File Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/Float...
Source/core/layout/FloatingObjects.cpp:463: return this->m_outermostFloat ?
this->m_layoutObject->logicalBottomForFloat(*(this->m_outermostFloat)) -
this->m_lineTop : LayoutUnit(1);
I don't think the parentheses are needed here due to operator precedence.

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/Layou...
File Source/core/layout/LayoutBlockFlow.h (right):

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.h:231: child.y() +
marginBeforeForChild(*(child.layoutObject())));
Same comment about parentheses.

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.h:234: return LayoutSize(child.x() +
marginBeforeForChild(*(child.layoutObject())),
Ditto.

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/line/...
File Source/core/layout/line/LineBreaker.cpp (right):

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/line/...
Source/core/layout/line/LineBreaker.cpp:42:
m_block->positionNewFloatOnLine(*(m_block->insertFloatingObject(*toLayoutBox(object))),
lastFloatFromPreviousLine, lineInfo, width);
Ditto.

[Sunil Ratnu, 2015-06-11 05:49:44]

Thanks for the review Julien. Made the changes suggested by you.

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/Float...
File Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/Float...
Source/core/layout/FloatingObjects.cpp:463: return this->m_outermostFloat ?
this->m_layoutObject->logicalBottomForFloat(*(this->m_outermostFloat)) -
this->m_lineTop : LayoutUnit(1);
On 2015/06/10 15:15:44, Julien Chaffraix - PST wrote:
> I don't think the parentheses are needed here due to operator precedence.

Done.

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/Layou...
File Source/core/layout/LayoutBlockFlow.h (right):

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.h:231: child.y() +
marginBeforeForChild(*(child.layoutObject())));
On 2015/06/10 15:15:44, Julien Chaffraix - PST wrote:
> Same comment about parentheses.

Done.

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/Layou...
Source/core/layout/LayoutBlockFlow.h:234: return LayoutSize(child.x() +
marginBeforeForChild(*(child.layoutObject())),
On 2015/06/10 15:15:44, Julien Chaffraix - PST wrote:
> Ditto.

Done.

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/line/...
File Source/core/layout/line/LineBreaker.cpp (right):

https://codereview.chromium.org/763173003/diff/70001/Source/core/layout/line/...
Source/core/layout/line/LineBreaker.cpp:42:
m_block->positionNewFloatOnLine(*(m_block->insertFloatingObject(*toLayoutBox(object))),
lastFloatFromPreviousLine, lineInfo, width);
On 2015/06/10 15:15:44, Julien Chaffraix - PST wrote:
> Ditto.

Done.

[Sunil Ratnu, 2015-06-11 05:57:44]

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

[Sunil Ratnu, 2015-06-11 05:57:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/763173003/#ps90001 (title:
"remove parenthesis")

[commit-bot: I haz the power, 2015-06-11 05:57:56]

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

[commit-bot: I haz the power, 2015-06-11 06:00:58]

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

[commit-bot: I haz the power, 2015-06-11 06:01:00]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/58519)

[Sunil Ratnu, 2015-06-11 08:50:29]

Oops. Looks like code has changed. Will rebase to the latest and upload again.

[Sunil Ratnu, 2015-06-11 15:26:26]

Hi Julien,

There was a lot of code to rebase. Could you please take a look one more time
before putting it to the CQ.

Thanks,
Sunil

[Sunil Ratnu, 2015-06-12 06:15:43]

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

[Sunil Ratnu, 2015-06-12 06:15:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/763173003/#ps110001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-12 06:15:52]

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

[commit-bot: I haz the power, 2015-06-12 07:31:23]

Committed patchset #6 (id:110001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197011