Don't indent if selection is none.

Don't indent if selection is none.

ApplyBlockElementCommand::doApply() assumes selection to indent points non-null
position. However, there is a chance this assumption is false, e.g. HTML used
in "indent-button-crash.html".

This patch makes ApplyBlockElementCommand::doApply() to check whether assumption
is satisfied or not.

BUG=368133
TEST=LayoutTests/editing/execCommand/indent-button-crash.html

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

[yosin_UTC9, 2014-05-20 08:46:53]

Could you review this patch?
Thanks in advance.

[yoichio, 2014-05-20 09:19:50]

lgtm

With a nit.

https://codereview.chromium.org/291143002/diff/1/LayoutTests/editing/execComm...
File LayoutTests/editing/execCommand/indent-button-crash.html (right):

https://codereview.chromium.org/291143002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/indent-button-crash.html:6: var aoScriptElements
= document.getElementsByTagName('script');
|aoScriptElements| violates the Blink style.

[yosin_UTC9, 2014-05-20 09:42:17]

PTAL

https://codereview.chromium.org/291143002/diff/1/LayoutTests/editing/execComm...
File LayoutTests/editing/execCommand/indent-button-crash.html (right):

https://codereview.chromium.org/291143002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/indent-button-crash.html:6: var aoScriptElements
= document.getElementsByTagName('script');
On 2014/05/20 09:19:50, yoichio wrote:
> |aoScriptElements| violates the Blink style.

Done.

[Yuta Kitamura, 2014-05-20 09:44:35]

Please wrap the change description in every 80-ish
characters.

Unbounded lines in commit log looks really ugly especially
in git log or other tools.

https://codereview.chromium.org/291143002/diff/1/LayoutTests/editing/execComm...
File LayoutTests/editing/execCommand/indent-button-crash.html (right):

https://codereview.chromium.org/291143002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/indent-button-crash.html:8:
aoScriptElements[i].parentNode.removeChild(aoScriptElements[i]);
Wrong indent.

https://codereview.chromium.org/291143002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/indent-button-crash.html:21: onload =
function(){
No space before '{'.

https://codereview.chromium.org/291143002/diff/1/Source/core/editing/ApplyBlo...
File Source/core/editing/ApplyBlockElementCommand.cpp (right):

https://codereview.chromium.org/291143002/diff/1/Source/core/editing/ApplyBlo...
Source/core/editing/ApplyBlockElementCommand.cpp:78: if
(endingSelection().isNone())
Is this logic equivalent to the following?

    VisibleSelection newSelection(visibleStart,
visibleEnd.previous(CannotCrossEditingBoundary),
endingSelection().isDirectional());
    if (newSelection().isNone())
        return;
    setEndingSelection(newSelection);

If so, I'd say my version is more readable because it exits early,
not after setEndingSelection(). Also there's slight difference in
behavior: with your version, this function ends up clearing
the ending selection which will probably affect subsequent
commands.

[yosin_UTC9, 2014-05-21 03:44:07]

PTAL

https://codereview.chromium.org/291143002/diff/1/LayoutTests/editing/execComm...
File LayoutTests/editing/execCommand/indent-button-crash.html (right):

https://codereview.chromium.org/291143002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/indent-button-crash.html:8:
aoScriptElements[i].parentNode.removeChild(aoScriptElements[i]);
On 2014/05/20 09:44:36, Yuta Kitamura wrote:
> Wrong indent.

Done.

https://codereview.chromium.org/291143002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/indent-button-crash.html:21: onload =
function(){
On 2014/05/20 09:44:36, Yuta Kitamura wrote:
> No space before '{'.

Done.

https://codereview.chromium.org/291143002/diff/1/Source/core/editing/ApplyBlo...
File Source/core/editing/ApplyBlockElementCommand.cpp (right):

https://codereview.chromium.org/291143002/diff/1/Source/core/editing/ApplyBlo...
Source/core/editing/ApplyBlockElementCommand.cpp:78: if
(endingSelection().isNone())
On 2014/05/20 09:44:36, Yuta Kitamura wrote:
> Is this logic equivalent to the following?
> 
>     VisibleSelection newSelection(visibleStart,
> visibleEnd.previous(CannotCrossEditingBoundary),
> endingSelection().isDirectional());
>     if (newSelection().isNone())
>         return;
>     setEndingSelection(newSelection);
> 
> If so, I'd say my version is more readable because it exits early,
> not after setEndingSelection(). Also there's slight difference in
> behavior: with your version, this function ends up clearing
> the ending selection which will probably affect subsequent
> commands.
Done.

Yes, you're right. Your version is better than mine.

[Yuta Kitamura, 2014-05-23 06:52:24]

LGTM, sorry for delay!

[yosin_UTC9, 2014-05-23 07:02:58]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2014-05-23 07:03:35]

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

[commit-bot: I haz the power, 2014-05-23 10:35:26]

Change committed as 174671