Get rid of useless test editing/execCommand/indent-button-crash.html

Get rid of useless test editing/execCommand/indent-button-crash.html

This patch gets rid of an useless test "indent-button-crash.html" which no
longer reach original expected code path and other than this, this test does not
do reasonable test. Removing this test makes layout tests little bit faster and
improving code health.

BUG=679977
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2673323002
Cr-Commit-Position: refs/heads/master@{#449217}
Committed: https://chromium.googlesource.com/chromium/src/+/37496e9375c69bd6276c2576fdf0546d88e6f9ea

[yosin_UTC9, 2017-02-06 06:17:22]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 06:17:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-02-06 06:22:08]

Description was changed from

==========
2017-02-06T15:16:16

2017-02-06T15:16:11

BUG=
==========

to

==========
Get rid of useless test editing/execCommand/indent-button-crash.html

This patch gets rid of an useless test "indent-button-crash.html" which no
longer reach original expected code path and other than this, this test does not
do reasonable test. Removing this test makes layout tests little bit faster and
improving code health.

BUG=679977
TEST=n/a; no behavior changes
==========

[yosin_UTC9, 2017-02-06 06:22:20]

yosin@chromium.org changed reviewers:
+ xiaochengh@chromium.org, yoichio@chromium.org

[yosin_UTC9, 2017-02-06 06:22:20]

PTAL

[Xiaocheng, 2017-02-06 07:02:43]

By "no longer reach original expected code path", do you mean it no longer
reaches the changed code in https://codereview.chromium.org/291143002?

[yosin_UTC9, 2017-02-06 07:53:28]

On 2017/02/06 at 07:02:43, xiaochengh wrote:
> By "no longer reach original expected code path", do you mean it no longer
reaches the changed code in https://codereview.chromium.org/291143002?

Yes, endingSelection().visible{Start,End}() returns null. endingSelection()
itself is <bar2>,0 in following DOM tree:
BODY (editable)
	BAR1 (editable)
SE		BAR2 (editable)
			BAR3 (editable)
				#text ""

"indent" command is invoked after getRangeAt(0).deleteContents() which removes
"abc" inserted by "selectionstart".
So, there are no visible texts in document before executing "indent" command.

The test is added in very early day of fixing cluster fuzz bugs. This test isn't
acceptable today's standard. (^_^;)

void ApplyBlockElementCommand::doApply(EditingState* editingState) {
  if (!endingSelection().rootEditableElement())
    return;

  // ApplyBlockElementCommands are only created directly by editor commands'
  // execution, which updates layout before entering doApply().
  DCHECK(!document().needsLayoutTreeUpdate());

  VisiblePosition visibleEnd = endingSelection().visibleEnd();
  VisiblePosition visibleStart = endingSelection().visibleStart();
  if (visibleStart.isNull() || visibleStart.isOrphan() || visibleEnd.isNull() ||
      visibleEnd.isOrphan())
    return;

[commit-bot: I haz the power, 2017-02-06 10:22:22]

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

[commit-bot: I haz the power, 2017-02-06 10:22:23]

Dry run: This issue passed the CQ dry run.

[yoichio, 2017-02-07 08:08:58]

Is there any layouttest that goes on
ApplyBlockElementCommand::doApply, L78-
if (visibleEnd.deepEquivalent() != visibleStart.deepEquivalent() &&
      isStartOfParagraph(visibleEnd)) {
    const Position& newEnd =
        previousPositionOf(visibleEnd, CannotCrossEditingBoundary)
            .deepEquivalent();
    SelectionInDOMTree::Builder builder;
    builder.collapse(visibleStart.toPositionWithAffinity());
    if (newEnd.isNotNull())
      builder.extend(newEnd);
    builder.setIsDirectional(endingSelection().isDirectional());
    const SelectionInDOMTree& newSelection = builder.build();
    if (newSelection.isNone())
      return;
    setEndingSelection(newSelection);
 }

block?
It looks strange that we check if builder.build() creates Null selection.

[yosin_UTC9, 2017-02-07 09:07:42]

On 2017/02/07 at 08:08:58, yoichio wrote:
> Is there any layouttest that goes on
> ApplyBlockElementCommand::doApply, L78-
> if (visibleEnd.deepEquivalent() != visibleStart.deepEquivalent() &&
>       isStartOfParagraph(visibleEnd)) {
>     const Position& newEnd =
>         previousPositionOf(visibleEnd, CannotCrossEditingBoundary)
>             .deepEquivalent();
>     SelectionInDOMTree::Builder builder;
>     builder.collapse(visibleStart.toPositionWithAffinity());
>     if (newEnd.isNotNull())
>       builder.extend(newEnd);
>     builder.setIsDirectional(endingSelection().isDirectional());
>     const SelectionInDOMTree& newSelection = builder.build();
>     if (newSelection.isNone())
>       return;
>     setEndingSelection(newSelection);
>  }
> 
> block?
> It looks strange that we check if builder.build() creates Null selection.

Not sure. It seems we can omit |if (newSeleciton.isNone()) return;|, since
|visibleStart| is not null.
I filed this[1].

Note: This block doesn't relate to the test to be removed.


[1] http://crbug.com/689392 ⚐	ApplyBlockElementCommand::doApply() has redundant
null selection check

[yoichio, 2017-02-08 02:20:25]

> Note: This block doesn't relate to the test to be removed.
The code block changed in https://codereview.chromium.org/2673323002/
 turned to that block in 2 years...

[yoichio, 2017-02-09 04:32:14]

As offline discussion, lgtm to remove this test.

[yosin_UTC9, 2017-02-09 04:34:05]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-02-09 04:34:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 06:09:42]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486614845773170, "parent_rev":
"5be8b586be828f6c54d153f74a5af39c96645377", "commit_rev":
"37496e9375c69bd6276c2576fdf0546d88e6f9ea"}

[commit-bot: I haz the power, 2017-02-09 06:10:38]

Description was changed from

==========
Get rid of useless test editing/execCommand/indent-button-crash.html

This patch gets rid of an useless test "indent-button-crash.html" which no
longer reach original expected code path and other than this, this test does not
do reasonable test. Removing this test makes layout tests little bit faster and
improving code health.

BUG=679977
TEST=n/a; no behavior changes
==========

to

==========
Get rid of useless test editing/execCommand/indent-button-crash.html

This patch gets rid of an useless test "indent-button-crash.html" which no
longer reach original expected code path and other than this, this test does not
do reasonable test. Removing this test makes layout tests little bit faster and
improving code health.

BUG=679977
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2673323002
Cr-Commit-Position: refs/heads/master@{#449217}
Committed:
https://chromium.googlesource.com/chromium/src/+/37496e9375c69bd6276c2576fdf0...
==========

[commit-bot: I haz the power, 2017-02-09 06:10:38]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/37496e9375c69bd6276c2576fdf0...