[New Multicolumn] Add RenderMultiColumnFlowThread::findSetRendering().

[New Multicolumn] Add RenderMultiColumnFlowThread::findSetRendering().

This is a preparatory patch for proper handling of dynamic insertion and
removal of elements inside multicol containers.

This new method will be needed when dynamically inserting spanners into the
middle of column content, since we will then need to split a column set in two
(and we need to figure out exactly which set to split). All of that will be
dealt with in a separate CL. For now, only call this method from some unit
tests, so see that it works.

BUG=347325
R=jchaffraix@chromium.org

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

[mstensho (USE GERRIT), 2015-01-21 12:52:36]

Julien, you may remember this method from an old and bigger (and canceled) CL. I
decided to break it out from the actual upcoming patch that adds support for
dynamic insertion and removal of content inside multicol containers.

[Julien - ping for review, 2015-01-22 10:23:09]

A couple of question, I don't have any gripe with this change and once they are
cleared we should be able to land that.

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

https://codereview.chromium.org/856383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:83:
ASSERT(sibling->isRenderMultiColumnSpannerPlaceholder());
Note that this ASSERT is already covered by the
toRenderMultiColumnSpannerPlaceholder, which does it in release too. In this
case, it's nice to be explicit.

https://codereview.chromium.org/856383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:91: return 0;
Shouldn't this return multicolSet->flowThread()->lastChild() to match
firstRendererInSet? /me has a sense of deja-vu on this question but has to ask
again

https://codereview.chromium.org/856383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:101: ASSERT(renderer !=
this && renderer->isDescendantOf(this));
It's better to split && into different ASSERTs. The reason is that when it
fails, you don't know which part of the predicate failed.

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

https://codereview.chromium.org/856383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:113:
EXPECT_EQ(flowThread->findSetRendering(document().getElementById("block")->renderer()),
columnSet);
Unfortunately by piggy-backing on these tests, you're making them more fragile
as they can break due to several reasons.

Usually it's better to follow a single responsibility approach for testing so
that there is only one reason for the test to break (and thus it's obvious what
went wrong).

[mstensho (USE GERRIT), 2015-01-22 10:39:57]

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

https://codereview.chromium.org/856383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:83:
ASSERT(sibling->isRenderMultiColumnSpannerPlaceholder());
On 2015/01/22 10:23:08, Julien Chaffraix - CET wrote:
> Note that this ASSERT is already covered by the
> toRenderMultiColumnSpannerPlaceholder, which does it in release too. In this
> case, it's nice to be explicit.

Yeah, since I want to explain that adjacent sets is verboten, I might as well
throw in this extra assert, I thought.

https://codereview.chromium.org/856383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:91: return 0;
On 2015/01/22 10:23:08, Julien Chaffraix - CET wrote:
> Shouldn't this return multicolSet->flowThread()->lastChild() to match
> firstRendererInSet? /me has a sense of deja-vu on this question but has to ask
> again

I could use *lastLeafChild()*. The only caller of this function doesn't really
care, though, so I thought it was just waste of time and binary code size. Then
you could say that it's not very nice behavior to return 0 here... That's why I
initially (in an earlier CL that got dropped) actually didn't have
firstRendererInSet() and lastRendererInSet(), but just had the code inside
findSetRendering(), to allow for tighter coupling, without writing mildly
"misbehaving" functions.

Let me know if I should change anything.

https://codereview.chromium.org/856383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:101: ASSERT(renderer !=
this && renderer->isDescendantOf(this));
On 2015/01/22 10:23:08, Julien Chaffraix - CET wrote:
> It's better to split && into different ASSERTs. The reason is that when it
> fails, you don't know which part of the predicate failed.

Indeed.
Done.

[Julien - ping for review, 2015-01-22 11:45:44]

lgtm

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

https://codereview.chromium.org/856383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:91: return 0;
On 2015/01/22 10:39:57, mstensho wrote:
> On 2015/01/22 10:23:08, Julien Chaffraix - CET wrote:
> > Shouldn't this return multicolSet->flowThread()->lastChild() to match
> > firstRendererInSet? /me has a sense of deja-vu on this question but has to
ask
> > again
> 
> I could use *lastLeafChild()*. The only caller of this function doesn't really
> care, though, so I thought it was just waste of time and binary code size.
Then
> you could say that it's not very nice behavior to return 0 here... That's why
I
> initially (in an earlier CL that got dropped) actually didn't have
> firstRendererInSet() and lastRendererInSet(), but just had the code inside
> findSetRendering(), to allow for tighter coupling, without writing mildly
> "misbehaving" functions.
> 
> Let me know if I should change anything.

Let's add a comment about this in the code to document that it's OK to do that.

[mstensho (USE GERRIT), 2015-01-22 11:53:40]

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

https://codereview.chromium.org/856383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:91: return 0;
On 2015/01/22 11:45:44, Julien Chaffraix - CET wrote:
> On 2015/01/22 10:39:57, mstensho wrote:
> > On 2015/01/22 10:23:08, Julien Chaffraix - CET wrote:
> > > Shouldn't this return multicolSet->flowThread()->lastChild() to match
> > > firstRendererInSet? /me has a sense of deja-vu on this question but has to
> ask
> > > again
> > 
> > I could use *lastLeafChild()*. The only caller of this function doesn't
really
> > care, though, so I thought it was just waste of time and binary code size.
> Then
> > you could say that it's not very nice behavior to return 0 here... That's
why
> I
> > initially (in an earlier CL that got dropped) actually didn't have
> > firstRendererInSet() and lastRendererInSet(), but just had the code inside
> > findSetRendering(), to allow for tighter coupling, without writing mildly
> > "misbehaving" functions.
> > 
> > Let me know if I should change anything.
> 
> Let's add a comment about this in the code to document that it's OK to do
that.

Done.

[mstensho (USE GERRIT), 2015-01-22 11:53:53]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-01-22 11:54:09]

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

[commit-bot: I haz the power, 2015-01-22 13:07:48]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188810