[New Multicolumn] Faster and more direct RenderMultiColumnSet::multiColumnFlowThread().

[New Multicolumn] Faster and more direct RenderMultiColumnSet::multiColumnFlowThread().

No need to go via the parent block to get the flow thread.

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

[mstensho (USE GERRIT), 2014-04-29 13:06:18]

I could have used the DEFINE_RENDER_OBJECT_TYPE_CASTS() macro instead of a
manual static_cast, but then I would have to add an
isRenderMultiColumnFlowThread() to RenderObject, which felt like vtable bloat,
seeing that this is only used in debug.

[mstensho (USE GERRIT), 2014-04-29 13:14:50]

Also call multiColumnFlowThread() in favor of
multicolBlock->multiColumnFlowThread()

[rune, 2014-04-30 11:00:12]

Rubber stamp with nits.

https://codereview.chromium.org/254883003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/254883003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFlowThread.h:63: // This one is only used in
assertions at the moment.
"one" -> "method"

https://codereview.chromium.org/254883003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThread.h (right):

https://codereview.chromium.org/254883003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.h:60: // This one is only used
in assertions at the moment.
"one" -> "method"?

[mstensho (USE GERRIT), 2014-04-30 11:03:16]

Rebase master

[mstensho (USE GERRIT), 2014-04-30 11:04:28]

Code review

[mstensho (USE GERRIT), 2014-04-30 11:05:05]

https://codereview.chromium.org/254883003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/254883003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderFlowThread.h:63: // This one is only used in
assertions at the moment.
On 2014/04/30 11:00:12, rune wrote:
> "one" -> "method"

Done.

https://codereview.chromium.org/254883003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThread.h (right):

https://codereview.chromium.org/254883003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.h:60: // This one is only used
in assertions at the moment.
On 2014/04/30 11:00:12, rune wrote:
> "one" -> "method"?

Done.

[ojan, 2014-04-30 17:31:28]

lgtm

https://codereview.chromium.org/254883003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnSet.h (right):

https://codereview.chromium.org/254883003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnSet.h:63:
ASSERT_WITH_SECURITY_IMPLICATION(!flowThread() ||
flowThread()->isRenderMultiColumnFlowThread());
Nit: I don't think this does anything different than plain ASSERT in debug
builds. The point of ASSERT_WITH_SECURITY_IMPLICATION is that we can run release
builds with these asserts turned on for clusterfuzz so that we find these
asserts in release builds.

I guess there's no harm in making it an ASSERT_WITH_SECURITY_IMPLICATION. If
this really has security implications, then I'd say you should remove the NDEBUG
check and do this in release builds. The code should all get stripped in
non-clusterfuzz release builds anyways. That way clusterfuzz will find code that
hits this assert.

[mstensho (USE GERRIT), 2014-04-30 19:32:41]

On 2014/04/30 17:31:28, ojan wrote:
> lgtm
> 
>
https://codereview.chromium.org/254883003/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderMultiColumnSet.h (right):
> 
>
https://codereview.chromium.org/254883003/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderMultiColumnSet.h:63:
> ASSERT_WITH_SECURITY_IMPLICATION(!flowThread() ||
> flowThread()->isRenderMultiColumnFlowThread());
> Nit: I don't think this does anything different than plain ASSERT in debug
> builds. The point of ASSERT_WITH_SECURITY_IMPLICATION is that we can run
release
> builds with these asserts turned on for clusterfuzz so that we find these
> asserts in release builds.
> 
> I guess there's no harm in making it an ASSERT_WITH_SECURITY_IMPLICATION. If
> this really has security implications, then I'd say you should remove the
NDEBUG
> check and do this in release builds. The code should all get stripped in
> non-clusterfuzz release builds anyways. That way clusterfuzz will find code
that
> hits this assert.

A bad down-cast here will let you access memory that's not part of the object. I
wasn't aware of how ASSERT_WITH_SECURITY_IMPLICATION works and that it's enabled
even in semi-release builds. I just chose it because that's what
DEFINE_RENDER_OBJECT_TYPE_CASTS expands to. Enabling this for all (removing all
#ifndef NDEBUG from my patch) seems like the most reasonable thing to me now.
I'll do that.

[mstensho (USE GERRIT), 2014-04-30 19:35:31]

Code review #2

[mstensho (USE GERRIT), 2014-04-30 19:36:34]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-04-30 19:37:08]

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

[commit-bot: I haz the power, 2014-04-30 20:38:13]

Change committed as 173025