Rendering API cleanup

Rendering API cleanup

These methods are not in use anymore. Also I moved some
to private since only the class itself was using them.

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

[rwlbuis, 2014-10-01 21:56:29]

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

[rwlbuis, 2014-10-01 21:56:30]

PTAL. I hope it is not too much to review!

[mstensho (USE GERRIT), 2014-10-01 22:21:36]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2014-10-01 22:21:38]

I only went through the multicol files, and didn't look at the rest.

I don't know if this big patch is too big. I'll leave that for someone else to
decide. :)

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderFlowThread.cpp (left):

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlowThread.cpp:371: void
RenderFlowThread::RegionSearchAdapter::collectIfNeeded(const
MultiColumnSetInterval& interval)
See issue that I raised in the .h file. We're going to need this.

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderFlowThread.h (left):

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlowThread.h:146: RenderMultiColumnSet*
startColumnSet() const { return m_startColumnSet; }
What purpose would this class (RenderMultiColumnSetRange) serve without those
methods? I'm pretty sure the right thing to do here is to remove the entire
class.

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlowThread.h:157: class RegionSearchAdapter {
I suggest that you keep this. I'm about to start using it again; see
https://codereview.chromium.org/584033002/patch/60001/70176

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderMultiColumnSet.h (left):

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.h:77: LayoutUnit
logicalHeightInFlowThread() const { return isHorizontalWritingMode() ?
flowThreadPortionRect().height() : flowThreadPortionRect().width(); }
Please keep this, and kill logicalHeightOfAllFlowThreadContent() instead.

[rwlbuis, 2014-10-01 22:36:15]

On 2014/10/01 22:21:38, mstensho wrote:
> I only went through the multicol files, and didn't look at the rest.
> 
> I don't know if this big patch is too big. I'll leave that for someone else to
> decide. :)

Thanks for looking at this, should be fixed no in patch set #2.

[mstensho (USE GERRIT), 2014-10-02 07:15:17]

https://codereview.chromium.org/620753006/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlowThread.cpp (left):

https://codereview.chromium.org/620753006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFlowThread.cpp:371: void
RenderFlowThread::RegionSearchAdapter::collectIfNeeded(const
MultiColumnSetInterval& interval)
Still need this.

[mstensho (USE GERRIT), 2014-10-02 10:51:46]

One more thing I think we should keep (review still incomplete; haven't looked
at anything else).

https://codereview.chromium.org/620753006/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (left):

https://codereview.chromium.org/620753006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1456: void
RenderLayer::convertToLayerCoords(const RenderLayer* ancestorLayer, LayoutRect&
rect) const
Can we keep this? https://codereview.chromium.org/612323011/patch/1/10003 wants
to use it.

[rwlbuis, 2014-10-02 14:00:47]

On 2014/10/02 10:51:46, mstensho wrote:
> One more thing I think we should keep (review still incomplete; haven't looked
> at anything else).
> 
>
https://codereview.chromium.org/620753006/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayer.cpp (left):
> 
>
https://codereview.chromium.org/620753006/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayer.cpp:1456: void
> RenderLayer::convertToLayerCoords(const RenderLayer* ancestorLayer,
LayoutRect&
> rect) const
> Can we keep this? https://codereview.chromium.org/612323011/patch/1/10003
wants
> to use it.

Sure I kept it in the latest patch set. Also fixed the other thing, I was to
quick last night with the upload...

[Julien - ping for review, 2014-10-02 16:47:49]

I have mixed feelings about this patch. Half of the removals are not really
warranted as it makes our API weird (we have firstChild but not lastChild or a
getter but no setter). The other half are great as they remove really dead code.

I don't think this patch is too big to review as it's very mechanical.

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

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlowThread.h:148: RenderMultiColumnSet*
m_endColumnSet;
Those 2 variables are unused in the code and could be removed.

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (left):

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:3443: bool
RenderLayer::isVisuallyNonEmpty() const
I swore I had seen a patch use this function, but maybe it didn't make the cut.

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayerReflectionInfo.cpp (left):

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayerReflectionInfo.cpp:148: String
RenderLayerReflectionInfo::debugName() const
Isn't this used in debug?

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderTable.h (left):

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderTable.h:155: RenderTableSection* footer() const {
return m_foot; }
I would keep this one for consistency.

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

https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderTableSection.h:53: unsigned& end() { return m_end; }
This API seems deeply dangerous.

[rwlbuis, 2014-10-02 19:56:16]

Thanks for the review!

On 2014/10/02 16:47:49, Julien Chaffraix - PST wrote:
> I have mixed feelings about this patch. Half of the removals are not really
> warranted as it makes our API weird (we have firstChild but not lastChild or a
> getter but no setter). The other half are great as they remove really dead
code.

Understood. I am ok with just shrinking to the "other half".

> I don't think this patch is too big to review as it's very mechanical.

Good.

>
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderFlowThread.h (right):
> 
>
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderFlowThread.h:148: RenderMultiColumnSet*
> m_endColumnSet;
> Those 2 variables are unused in the code and could be removed.

I think the whole class has been removed in a later patch set.

>
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderLayer.cpp (left):
> 
>
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderLayer.cpp:3443: bool
> RenderLayer::isVisuallyNonEmpty() const
> I swore I had seen a patch use this function, but maybe it didn't make the
cut.

There is some similarly named API in FrameView, that is the only thing I could
find.

>
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderLayerReflectionInfo.cpp (left):
> 
>
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderLayerReflectionInfo.cpp:148: String
> RenderLayerReflectionInfo::debugName() const
> Isn't this used in debug?

I think not since the debug builds show no problem. Unless there is another
builder I should run.

>
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderTable.h (left):
> 
>
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderTable.h:155: RenderTableSection* footer() const {
> return m_foot; }
> I would keep this one for consistency.

Done.

>
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderTableSection.h (right):
> 
>
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderTableSection.h:53: unsigned& end() { return m_end;
}
> This API seems deeply dangerous.

I see. I changed the API a bit, hope you like that better.

[Julien - ping for review, 2014-10-03 01:12:46]

On 2014/10/02 at 19:56:16, rob.buis wrote:
> Thanks for the review!
> 
> On 2014/10/02 16:47:49, Julien Chaffraix - PST wrote:
> > I have mixed feelings about this patch. Half of the removals are not really
> > warranted as it makes our API weird (we have firstChild but not lastChild or
a
> > getter but no setter). The other half are great as they remove really dead
code.
> 
> Understood. I am ok with just shrinking to the "other half".

Totally!

> >
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> > File Source/core/rendering/RenderLayerReflectionInfo.cpp (left):
> > 
> >
https://codereview.chromium.org/620753006/diff/1/Source/core/rendering/Render...
> > Source/core/rendering/RenderLayerReflectionInfo.cpp:148: String
> > RenderLayerReflectionInfo::debugName() const
> > Isn't this used in debug?
> 
> I think not since the debug builds show no problem. Unless there is another
builder I should run.

I used the wrong word in the comment, my apologies. I meant to say: "isn't this
used for debugging?"

And I double-checked that it's not as RenderLayer::debugName does this very
check so we're clear.

[Julien - ping for review, 2014-10-03 01:26:43]

lgtm.

Please format your description so that it doesn't go past 80 characters (I use
72 as my limit based on
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/rqGhY7PfkYY )

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Co...
File Source/core/rendering/CounterNode.h (left):

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Co...
Source/core/rendering/CounterNode.h:61: CounterNode* firstChild() const { return
m_firstChild; }
Let's keep this one for consistency.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Hi...
File Source/core/rendering/HitTestLocation.h (left):

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Hi...
Source/core/rendering/HitTestLocation.h:57: bool isRectilinear() const { return
m_isRectilinear; }
Nit: Maybe keep?

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/In...
File Source/core/rendering/InlineBox.h (left):

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/In...
Source/core/rendering/InlineBox.h:208: float bottom() const { return top() +
height(); }
I think we should keep these.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/In...
Source/core/rendering/InlineBox.h:290: void flipForWritingMode(FloatRect&);
This is an inconsistency from my perspective as we have LayoutPoint & FloatPoint
but not FloatRect. We should keep it for consistency.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlockFlow.h (left):

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockFlow.h:123: LayoutSize
logicalSizeForFloat(const FloatingObject* floatingObject) const { return
isHorizontalWritingMode() ? LayoutSize(floatingObject->width(),
floatingObject->height()) : LayoutSize(floatingObject->height(),
floatingObject->width()); }
We should keep logicalHeightForFloat() but on the fence about
logicalSizeForFloat() so either way is fine.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockFlow.h:128: int
pixelSnappedLogicalRightForFloat(const FloatingObject* floatingObject) const {
return isHorizontalWritingMode() ?
floatingObject->frameRect().pixelSnappedMaxX() :
floatingObject->frameRect().pixelSnappedMaxY(); }
We should keep them for consistency too.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.h (left):

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:122: int pixelSnappedLogicalWidth() const {
return style()->isHorizontalWritingMode() ? pixelSnappedWidth() :
pixelSnappedHeight(); }
Let's keep this one for consistency.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.h:293: LayoutUnit marginLogicalRight() const {
return m_marginBox.logicalRight(style()->writingMode()); }
Ditto.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.h (left):

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.h:131: LayoutUnit
borderLogicalHeight() const { return borderBefore() + borderAfter(); }
Ditto (x3).

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.h:134: LayoutUnit
paddingLogicalRight() const { return style()->isHorizontalWritingMode() ?
paddingRight() : paddingBottom(); }
Ditto.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderFrameSet.h (left):

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderFrameSet.h:63: RenderObject* lastChild() const {
ASSERT(children() == virtualChildren()); return children()->lastChild(); }
Ditto.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderFrameSet.h:63: RenderObject* lastChild() const {
ASSERT(children() == virtualChildren()); return children()->lastChild(); }
Ditto.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderMedia.h (left):

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderMedia.h:42: RenderObject* lastChild() const {
ASSERT(children() == virtualChildren()); return children()->lastChild(); }
Those are important and should be kept: RenderObject only implements
slowFirstChild / slowLastChild and renderers that can have descendants have to
implement these 2.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderTextControl.h (left):

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderTextControl.h:53: int textBlockLogicalWidth() const;
Nit: Would probably keep.

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.h (left):

https://codereview.chromium.org/620753006/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.h:102: RenderObject* selectionEnd() const {
return m_selectionEnd; }
Let's keep this one for consistency.

[rwlbuis, 2014-10-03 15:59:44]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-10-03 15:59:54]

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

[rwlbuis, 2014-10-03 16:00:42]

The CQ bit was unchecked by rob.buis@samsung.com

[rwlbuis, 2014-10-03 16:01:27]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-10-03 16:01:58]

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

[commit-bot: I haz the power, 2014-10-03 17:06:21]

Committed patchset #5 (id:80001) as 183203