[New Multicolumn] Create RenderMultiColumnSet during renderer creation, not during layout.

[New Multicolumn] Create RenderMultiColumnSet during renderer creation, not during layout.

This allowed for removal of 2 parameters to regionAtBlockOffset(), and
we can also make it const now.

Also make some preparations for multiple RenderMultiColumnSet
objects. This is required by column-span:all.

Add some generic multicol documentation in front of the class definition
of RenderMultiColumnFlowThread, since that's the main class for multicol.
Also improved RenderMultiColumnSet documentation somewhat.

All this is just code improvements; no behavioral changes intended.

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

[ojan, 2014-04-24 21:38:42]

lgtm

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

https://codereview.chromium.org/246403015/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:82:
multiColumnBlockFlow()->RenderBlock::addChild(newSet);
Why do we need the RenderBlock version? Can the multiColumnBlockFlow() have a
flowThread? In either case, this might deserve a comment.

https://codereview.chromium.org/246403015/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:101: while
(RenderMultiColumnSet* columnSet = firstMultiColumnSet())
How do we have multiple sets? It looks like addChild will only ever create one.
From your comment above it sounds like maybe in the future we'll have multiple,
but if that's the case, maybe we could add the loops (here and below) then?

[mstensho (USE GERRIT), 2014-04-24 22:22:21]

Let me know if I understood and addressed the issues correctly.

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

https://codereview.chromium.org/246403015/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:82:
multiColumnBlockFlow()->RenderBlock::addChild(newSet);
On 2014/04/24 21:38:43, ojan wrote:
> Why do we need the RenderBlock version? Can the multiColumnBlockFlow() have a
> flowThread? In either case, this might deserve a comment.

Yes, multiColumnBlockFlow() is the one that has a flow thread, so if we use
RenderBlockFlow::addChild(), we'd get redirected right back here. Would cause an
infinite recursion.

Anyway, worth a comment. Done.

https://codereview.chromium.org/246403015/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:101: while
(RenderMultiColumnSet* columnSet = firstMultiColumnSet())
On 2014/04/24 21:38:43, ojan wrote:
> How do we have multiple sets? It looks like addChild will only ever create
one.
> From your comment above it sounds like maybe in the future we'll have
multiple,
> but if that's the case, maybe we could add the loops (here and below) then?

For now, only one set. In the (near) future, we can have multiple sets, because
of spanners. This is already a while-loop, so I'm not sure I understand what you
mean.

If by "below" you mean the for-loop in layoutColumns(), I agree. I missed
updating that one. Will fix.

[ojan, 2014-04-24 22:46:58]

lgtm

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

https://codereview.chromium.org/246403015/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:101: while
(RenderMultiColumnSet* columnSet = firstMultiColumnSet())
On 2014/04/24 22:22:22, Morten Stenshorne wrote:
> On 2014/04/24 21:38:43, ojan wrote:
> > How do we have multiple sets? It looks like addChild will only ever create
> one.
> > From your comment above it sounds like maybe in the future we'll have
> multiple,
> > but if that's the case, maybe we could add the loops (here and below) then?
> 
> For now, only one set. In the (near) future, we can have multiple sets,
because
> of spanners. This is already a while-loop, so I'm not sure I understand what
you
> mean.

It's just a bit weird to me to have a while loop that never loops. But, it's
true that it was a loop before as well and it will soon loop. So it's fine. If I
were doing this, I'd add the while back in when adding spanners, but I don't
feel strongly about this.

[commit-bot: I haz the power, 2014-04-24 22:47:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/246403015/20001

[commit-bot: I haz the power, 2014-04-25 00:01:45]

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

[commit-bot: I haz the power, 2014-04-25 00:01:45]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[mstensho (USE GERRIT), 2014-04-25 09:52:13]

Some recent change on master triggered an assertion failure [1], and luckily it
was discovered by one of the bots.

With this patch, if the flow thread has no children, there will be no
RenderMultiColumnSet created (previously, you'd always end up with one, even if
there was nothing to paint). So RenderFlowThread::mapLocalToContainer() has no
"region" to redirect to. Need to use the super class's implementation instead
then.

[1] ASSERTION FAILED: enclosingIntRect(rendererMappedResult) ==
enclosingIntRect(FloatQuad(result).boundingBox())
../../third_party/WebKit/Source/core/rendering/RenderGeometryMap.cpp(178) :
WebCore::FloatQuad WebCore::RenderGeometryMap::mapToContainer(const
WebCore::FloatRect&, const WebCore::RenderLayerModelObject*) const

[mstensho (USE GERRIT), 2014-04-25 09:52:22]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-04-25 09:52:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/246403015/60001

[commit-bot: I haz the power, 2014-04-25 10:27:14]

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

[commit-bot: I haz the power, 2014-04-25 10:27:15]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[mstensho (USE GERRIT), 2014-04-25 10:37:58]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-04-25 10:38:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/246403015/60001

[commit-bot: I haz the power, 2014-04-25 11:10:05]

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

[commit-bot: I haz the power, 2014-04-25 11:10:06]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[mstensho (USE GERRIT), 2014-04-25 13:48:48]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-04-25 13:49:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/246403015/60001

[commit-bot: I haz the power, 2014-04-25 14:26:03]

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

[commit-bot: I haz the power, 2014-04-25 14:26:03]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[mstensho (USE GERRIT), 2014-04-25 14:34:40]

All right, the bot is clearly having issues. Good thing, actually, because I
forgot one thing: regionAtBlockOffset() can be made const now, and one ugly cast
can therefore go away. So I'm fixing that while waiting for better weather.

[mstensho (USE GERRIT), 2014-04-25 14:35:13]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-04-25 14:35:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/246403015/70001

[commit-bot: I haz the power, 2014-04-25 15:08:16]

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

[commit-bot: I haz the power, 2014-04-25 15:08:17]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[mstensho (USE GERRIT), 2014-04-25 15:09:30]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-04-25 15:10:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/246403015/70001

[commit-bot: I haz the power, 2014-04-25 15:11:40]

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

[commit-bot: I haz the power, 2014-04-25 15:11:40]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[mstensho (USE GERRIT), 2014-04-26 00:26:46]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-04-26 00:27:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/246403015/70001

[commit-bot: I haz the power, 2014-04-26 02:21:17]

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

[commit-bot: I haz the power, 2014-04-26 02:21:18]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[mstensho (USE GERRIT), 2014-04-26 07:29:41]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-04-26 07:30:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/246403015/70001

[commit-bot: I haz the power, 2014-04-26 07:30:14]

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

[commit-bot: I haz the power, 2014-04-26 07:30:15]

Failed to apply patch for Source/core/rendering/RenderBlock.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderBlock.cpp
  Hunk #1 FAILED at 4856.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/rendering/RenderBlock.cpp.rej

Patch:       Source/core/rendering/RenderBlock.cpp
Index: Source/core/rendering/RenderBlock.cpp
diff --git a/Source/core/rendering/RenderBlock.cpp
b/Source/core/rendering/RenderBlock.cpp
index
3493f57328b02b3a860375b41de0813bc6c2f7d8..7836118358e7b77109e533462b85a5d664ea2377
100644
--- a/Source/core/rendering/RenderBlock.cpp
+++ b/Source/core/rendering/RenderBlock.cpp
@@ -4856,7 +4856,7 @@ RenderRegion* RenderBlock::regionAtBlockOffset(LayoutUnit
blockOffset) const
     if (!flowThread || !flowThread->hasValidRegionInfo())
         return 0;
 
-    return flowThread->regionAtBlockOffset(offsetFromLogicalTopOfFirstPage() +
blockOffset, true);
+    return flowThread->regionAtBlockOffset(offsetFromLogicalTopOfFirstPage() +
blockOffset);
 }
 
 LayoutUnit RenderBlock::collapsedMarginBeforeForChild(const RenderBox* child)
const

[mstensho (USE GERRIT), 2014-04-26 07:37:43]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-04-26 07:38:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/246403015/90001

[commit-bot: I haz the power, 2014-04-26 08:18:11]

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

[commit-bot: I haz the power, 2014-04-26 08:18:11]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[mstensho (USE GERRIT), 2014-04-26 08:39:47]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-04-26 08:40:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/246403015/90001

[commit-bot: I haz the power, 2014-04-26 09:30:45]

Change committed as 172698