Get rid of false assert from CompositeEditCommand::moveParagraphContentsToNewBlockIfNecessary

Get rid of false assert from CompositeEditCommand::moveParagraphContentsToNewBlockIfNecessary

This patch removes false assumption from CompositeEditCommand::moveParagraphContentsToNewBlockIfNecessary,
which attached test case breaks that.

In the test case attached test case, |upstreamStart| points empty UL elemnt,
but it is visble, and |upstreamEnd| points A element. Thus, |upstreamStart|
isn't descendant of |upstreamEnd|. When removing "style" attribute from UL
element to make it invisible, this test case doesn't cause assertion.

BUG=366068
TEST=LayoutTests/editing/execCommand/justify-center-empty-list-crash.html

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

[yosin_UTC9, 2014-05-27 08:57:29]

Could you review this patch?
Thanks in advance.

[Yuta Kitamura, 2014-05-28 09:14:10]

https://codereview.chromium.org/300023009/diff/1/Source/core/editing/Composit...
File Source/core/editing/CompositeEditCommand.cpp (left):

https://codereview.chromium.org/300023009/diff/1/Source/core/editing/Composit...
Source/core/editing/CompositeEditCommand.cpp:928:
ASSERT(upstreamStart.deprecatedNode()->isDescendantOf(enclosingBlock(upstreamEnd.deprecatedNode())));
I tried to follow the code but couldn't understand why
this deletion is safe... Could you give a bit more context,
please?

[yosin_UTC9, 2014-05-29 04:04:38]

https://codereview.chromium.org/300023009/diff/1/Source/core/editing/Composit...
File Source/core/editing/CompositeEditCommand.cpp (left):

https://codereview.chromium.org/300023009/diff/1/Source/core/editing/Composit...
Source/core/editing/CompositeEditCommand.cpp:928:
ASSERT(upstreamStart.deprecatedNode()->isDescendantOf(enclosingBlock(upstreamEnd.deprecatedNode())));
On 2014/05/28 09:14:10, Yuta Kitamura wrote:
> I tried to follow the code but couldn't understand why
> this deletion is safe... Could you give a bit more context,
> please?

My reasoning is rather simple. We have a case breaking this ASSERT. The author
who add this ASSERT didn't consider empty list but visible.

[Yuta Kitamura, 2014-05-29 09:00:23]

https://codereview.chromium.org/300023009/diff/1/Source/core/editing/Composit...
File Source/core/editing/CompositeEditCommand.cpp (left):

https://codereview.chromium.org/300023009/diff/1/Source/core/editing/Composit...
Source/core/editing/CompositeEditCommand.cpp:928:
ASSERT(upstreamStart.deprecatedNode()->isDescendantOf(enclosingBlock(upstreamEnd.deprecatedNode())));
On 2014/05/29 04:04:39, yosi wrote:
> On 2014/05/28 09:14:10, Yuta Kitamura wrote:
> > I tried to follow the code but couldn't understand why
> > this deletion is safe... Could you give a bit more context,
> > please?
> 
> My reasoning is rather simple. We have a case breaking this ASSERT. The author
> who add this ASSERT didn't consider empty list but visible.

I thought about this for some time but I couldn't reach the conclusion
that this ASSERT is bogus; it looks like a legit assumption to me.

Rather, I think there's something wrong in positions creation logic in
lines 896-900, because it's weird that |upstreamStart| and |upstreamEnd|
belong to different paragraphs.

[Yuta Kitamura, 2014-05-30 04:40:44]

LGTM with comments

https://codereview.chromium.org/300023009/diff/1/Source/core/editing/Composit...
File Source/core/editing/CompositeEditCommand.cpp (left):

https://codereview.chromium.org/300023009/diff/1/Source/core/editing/Composit...
Source/core/editing/CompositeEditCommand.cpp:928:
ASSERT(upstreamStart.deprecatedNode()->isDescendantOf(enclosingBlock(upstreamEnd.deprecatedNode())));
On 2014/05/29 09:00:24, Yuta Kitamura wrote:
> On 2014/05/29 04:04:39, yosi wrote:
> > On 2014/05/28 09:14:10, Yuta Kitamura wrote:
> > > I tried to follow the code but couldn't understand why
> > > this deletion is safe... Could you give a bit more context,
> > > please?
> > 
> > My reasoning is rather simple. We have a case breaking this ASSERT. The
author
> > who add this ASSERT didn't consider empty list but visible.
> 
> I thought about this for some time but I couldn't reach the conclusion
> that this ASSERT is bogus; it looks like a legit assumption to me.
> 
> Rather, I think there's something wrong in positions creation logic in
> lines 896-900, because it's weird that |upstreamStart| and |upstreamEnd|
> belong to different paragraphs.

I started to feel like this ASSERT is indeed bogus.

So I'm fine with removing the ASSERT, but could you un-delete the portion
of the comment explaining this "if" condition? It looks useful. (I'm okay
with removing the text "The visibleEnd." which does not make any sense
to me.)

[yosin_UTC9, 2014-05-30 05:04:26]

Thanks for reviewing!
Committing...

https://codereview.chromium.org/300023009/diff/1/Source/core/editing/Composit...
File Source/core/editing/CompositeEditCommand.cpp (left):

https://codereview.chromium.org/300023009/diff/1/Source/core/editing/Composit...
Source/core/editing/CompositeEditCommand.cpp:928:
ASSERT(upstreamStart.deprecatedNode()->isDescendantOf(enclosingBlock(upstreamEnd.deprecatedNode())));
On 2014/05/30 04:40:45, Yuta Kitamura wrote:
> On 2014/05/29 09:00:24, Yuta Kitamura wrote:
> > On 2014/05/29 04:04:39, yosi wrote:
> > > On 2014/05/28 09:14:10, Yuta Kitamura wrote:
> > > > I tried to follow the code but couldn't understand why
> > > > this deletion is safe... Could you give a bit more context,
> > > > please?
> > > 
> > > My reasoning is rather simple. We have a case breaking this ASSERT. The
> author
> > > who add this ASSERT didn't consider empty list but visible.
> > 
> > I thought about this for some time but I couldn't reach the conclusion
> > that this ASSERT is bogus; it looks like a legit assumption to me.
> > 
> > Rather, I think there's something wrong in positions creation logic in
> > lines 896-900, because it's weird that |upstreamStart| and |upstreamEnd|
> > belong to different paragraphs.
> 
> I started to feel like this ASSERT is indeed bogus.
> 
> So I'm fine with removing the ASSERT, but could you un-delete the portion
> of the comment explaining this "if" condition? It looks useful. (I'm okay
> with removing the text "The visibleEnd." which does not make any sense
> to me.)

Done.

[yosin_UTC9, 2014-05-30 05:04:31]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2014-05-30 05:06:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yosin@chromium.org/300023009/20001

[commit-bot: I haz the power, 2014-05-30 12:59:59]

Change committed as 175118