Make RemoveFormat command to work with mixed editable

Make RemoveFormat command to work with mixed editable

This patch changes assertion in |ApplyStyleCommand::removeInlineStyle()| to
consider |PositionIsBeforeChildren| as same as |PositionIsBeforeAnchor| and
|offsetInContainerNode <= 0|.

In the attached test case, |firstPositionInOrBeforeNode(next.get())| returns
|PositionIsBeforeChildren| for B element. When we don't have uneditable
element, inline styles are removed by |removeInlineStyleFromElement| for
BODY element rather than each child in BODY element. Blink doesn't go the path
contains the assertion.

BUG=388212
TEST=LayoutTests/editing/execCommand/remove-foramt-mixed-editable-crash.html

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

[yosin_UTC9, 2014-06-26 08:27:32]

Could you review this patch?
Thanks in advance.

[Yuta Kitamura, 2014-07-01 09:12:35]

https://codereview.chromium.org/332613003/diff/1/Source/core/editing/ApplySty...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/332613003/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1158: ASSERT(s.anchorType() ==
Position::PositionIsBeforeAnchor || s.anchorType() ==
Position::PositionIsBeforeChildren || s.offsetInContainerNode() <= 0);
Who creates the position of type PositionIsBeforeChildren? It's possible
that the creator of the position is to blame, instead of this assertion.

[yosin_UTC9, 2014-07-02 03:30:36]

https://codereview.chromium.org/332613003/diff/1/Source/core/editing/ApplySty...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/332613003/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1158: ASSERT(s.anchorType() ==
Position::PositionIsBeforeAnchor || s.anchorType() ==
Position::PositionIsBeforeChildren || s.offsetInContainerNode() <= 0);
On 2014/07/01 09:12:35, Yuta Kitamura wrote:
> Who creates the position of type PositionIsBeforeChildren? It's possible
> that the creator of the position is to blame, instead of this assertion.

In the attached test case, |firstPositionInOrBeforeNode(next.get())|, L1159=just
below assertion, returns 
|PositionIsBeforeChildren| for B element.

Here is definition of |firstPositionInOrBeforeNode|
inline Position firstPositionInOrBeforeNode(Node* node)
{
    if (!node)
        return Position();
    return editingIgnoresContent(node) ? positionBeforeNode(node) :
firstPositionInNode(node);
}

[Yuta Kitamura, 2014-07-02 07:27:21]

https://codereview.chromium.org/332613003/diff/1/Source/core/editing/ApplySty...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/332613003/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1158: ASSERT(s.anchorType() ==
Position::PositionIsBeforeAnchor || s.anchorType() ==
Position::PositionIsBeforeChildren || s.offsetInContainerNode() <= 0);
On 2014/07/02 03:30:36, Yosi_UTC9 wrote:
> On 2014/07/01 09:12:35, Yuta Kitamura wrote:
> > Who creates the position of type PositionIsBeforeChildren? It's possible
> > that the creator of the position is to blame, instead of this assertion.
> 
> In the attached test case, |firstPositionInOrBeforeNode(next.get())|,
L1159=just
> below assertion, returns 
> |PositionIsBeforeChildren| for B element.
> 
> Here is definition of |firstPositionInOrBeforeNode|
> inline Position firstPositionInOrBeforeNode(Node* node)
> {
>     if (!node)
>         return Position();
>     return editingIgnoresContent(node) ? positionBeforeNode(node) :
> firstPositionInNode(node);
> }

I don't understand your reasoning. This ASSERT is located
before the assignment at line 1159. How does that affect
the value of |s.anchorType()| at line 1158?

[yosin_UTC9, 2014-07-02 08:04:03]

https://codereview.chromium.org/332613003/diff/1/Source/core/editing/ApplySty...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/332613003/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1158: ASSERT(s.anchorType() ==
Position::PositionIsBeforeAnchor || s.anchorType() ==
Position::PositionIsBeforeChildren || s.offsetInContainerNode() <= 0);
On 2014/07/02 07:27:21, Yuta Kitamura wrote:
> On 2014/07/02 03:30:36, Yosi_UTC9 wrote:
> > On 2014/07/01 09:12:35, Yuta Kitamura wrote:
> > > Who creates the position of type PositionIsBeforeChildren? It's possible
> > > that the creator of the position is to blame, instead of this assertion.
> > 
> > In the attached test case, |firstPositionInOrBeforeNode(next.get())|,
> L1159=just
> > below assertion, returns 
> > |PositionIsBeforeChildren| for B element.
> > 
> > Here is definition of |firstPositionInOrBeforeNode|
> > inline Position firstPositionInOrBeforeNode(Node* node)
> > {
> >     if (!node)
> >         return Position();
> >     return editingIgnoresContent(node) ? positionBeforeNode(node) :
> > firstPositionInNode(node);
> > }
> 
> I don't understand your reasoning. This ASSERT is located
> before the assignment at line 1159. How does that affect
> the value of |s.anchorType()| at line 1158?
The ASSERT and firstPositionInOrBeforeNode() is in while loop. |s| is updated
here and checked in next iteration.

[Yuta Kitamura, 2014-07-03 05:11:29]

lgtm

https://codereview.chromium.org/332613003/diff/1/Source/core/editing/ApplySty...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/332613003/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1158: ASSERT(s.anchorType() ==
Position::PositionIsBeforeAnchor || s.anchorType() ==
Position::PositionIsBeforeChildren || s.offsetInContainerNode() <= 0);
Okay, I think this is an oversight from rniwa's change
<https://chromium.googlesource.com/chromium/blink/+/0db51d7c3a2245cb7b99c6956f...>.

[yosin_UTC9, 2014-07-03 05:18:43]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2014-07-03 05:19:31]

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

[commit-bot: I haz the power, 2014-07-03 05:22:51]

Change committed as 177448