[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential order

[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and
     apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry.

--- After CL behavior

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEvent/blink-dev/RrnitB0OElc/rirueVekCwAJ

BUG=652393
Committed: https://crrev.com/93db3c3bb91fb3e02080e16a0bd0b813b3d10a3e
Cr-Commit-Position: refs/heads/master@{#422915}

[chongz, 2016-09-27 15:12:45]

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

[commit-bot: I haz the power, 2016-09-27 15:13:03]

Dry run: 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, 2016-09-27 17:23:26]

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

[commit-bot: I haz the power, 2016-09-27 17:23:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chongz, 2016-09-27 21:11:56]

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

[commit-bot: I haz the power, 2016-09-27 21:12:18]

Dry run: 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, 2016-09-27 22:29:06]

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

[commit-bot: I haz the power, 2016-09-27 22:29:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chongz, 2016-09-28 00:51:30]

Description was changed from

==========
Combine |deleteByDrag| and |insertFromDrop| into a single undo


Add layouttests for Drag&Drop crash on removing iframe


Add layouttests for Drag&Drop (require 2 undos)


Add |inputType| for Drag&Drop

BUG=639139
==========

to

==========
[InputEvent] Support |deleteByDrag| and |insertFromDrop| and fire in sequential
order

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

* Potential issues:
  1. JS could hack to merge two unrelated Drag and Drop by
    i. preventDefault() on first |insertFromDrop|
    ii. preventDefault() on second |deleteByDrag|
    iii. The leftovers will still get merged into a single undo entry

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=639139
==========

[chongz, 2016-09-28 00:51:57]

Description was changed from

==========
[InputEvent] Support |deleteByDrag| and |insertFromDrop| and fire in sequential
order

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

* Potential issues:
  1. JS could hack to merge two unrelated Drag and Drop by
    i. preventDefault() on first |insertFromDrop|
    ii. preventDefault() on second |deleteByDrag|
    iii. The leftovers will still get merged into a single undo entry

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=639139
==========

to

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

* Potential issues:
  1. JS could hack to merge two unrelated Drag and Drop by
    i. preventDefault() on first |insertFromDrop|
    ii. preventDefault() on second |deleteByDrag|
    iii. The leftovers will still get merged into a single undo entry

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=639139
==========

[chongz, 2016-09-28 00:52:31]

Description was changed from

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

* Potential issues:
  1. JS could hack to merge two unrelated Drag and Drop by
    i. preventDefault() on first |insertFromDrop|
    ii. preventDefault() on second |deleteByDrag|
    iii. The leftovers will still get merged into a single undo entry

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=639139
==========

to

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

* Potential issues:
  1. JS could hack to merge two unrelated Drag and Drop by
    i. preventDefault() on first |insertFromDrop|
    ii. preventDefault() on second |deleteByDrag|
    iii. The leftovers will still get merged into a single undo entry

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=639139
==========

[chongz, 2016-09-28 00:52:41]

Patchset #1 (id:1) has been deleted

[chongz, 2016-09-28 00:52:50]

Patchset #1 (id:20001) has been deleted

[chongz, 2016-09-28 00:58:30]

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

[commit-bot: I haz the power, 2016-09-28 00:59:05]

Dry run: 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, 2016-09-28 02:16:39]

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

[commit-bot: I haz the power, 2016-09-28 02:16:40]

Dry run: This issue passed the CQ dry run.

[chongz, 2016-09-28 02:19:25]

chongz@chromium.org changed reviewers:
+ yosin@chromium.org

[chongz, 2016-09-28 02:19:26]

yosin@ PTAL, thanks!

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.cpp:724: m_lastEditCommand = cmd;
Has potential issue. These 2 commands might be created from 2 separate
Drag&Drop, but I don't have a better solution for this...

[yosin_UTC9, 2016-09-28 04:19:03]

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/platform/linux/editing/pasteboard/drag-drop-modifies-page-expected.txt
(right):

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/platform/linux/editing/pasteboard/drag-drop-modifies-page-expected.txt:2:
EDITING DELEGATE:
webViewDidChangeSelection:WebViewDidChangeSelectionNotification
I'll convert "drag-drop-modifies-page.html" to use w3c test harness.
Once it is done, we don't bother with editing delegate logging.

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.cpp:724: m_lastEditCommand = cmd;
On 2016/09/28 at 02:19:26, chongz wrote:
> Has potential issue. These 2 commands might be created from 2 separate
Drag&Drop, but I don't have a better solution for this...

How about having DragAndDropCommand, derived from CompositeEditCommand, which
hold DeleteByDrag and InputFromDrop, if they aren't canceled?
We hold DragAndDropCommand in m_lastEditCommand and check here like:

if (m_lastEditCommand && m_lastEditCommand->isDragAndDropCommand()) {
  if (cmd is DeleteByDrag or cmd is InsertFromDrag)
   m_lastEditCOmamnd->appendCommandToComposite(cmd);
}

In this way, it seems we don't need to introduce CompositeEditCommand::append()
and append for vector of EditCommand

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:300:
void CompositeEditCommand::appendCommandToComposite(CompositeEditCommand*
command)
Good invention!
We can implement Undo/Redo for drag-and-drop as easy as others.
(^_^)b

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
(right):

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:843:
if (!m_referenceMovePosition.isNull()) {
nit: s/!m_referenceMovePosition.isNull()/m_referenceMovePosition.isNotNull()/

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:844:
// Update the position otherwise it may become invalid after the selection is
deleted.
Could you utilize |RelocatablePosition|?

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:141: bool
doDeleteByDragWithEvents(Element* dragTarget, bool smartDelete, const Position&
referenceMovePosition)
Please add comment about |bool| return value.

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:142: {
This function should be in Editor class and avoid to use |bool| parameter.

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:149: const bool
shouldDelete = dispatchBeforeInputEditorCommand(dragTarget,
InputEvent::InputType::DeleteByDrag, nullptr) ==
DispatchEventResult::NotCanceled;
I think we should dispatch event to drag source rather than drag target since
deletion is occurred at drag source rather than drag target.

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:161: bool
doInsertFromDropWithEvents(Element* dropTarget, DragData* dragData,
DocumentFragment* fragment, Range* dropCaretRange, bool smartInsert, bool
chosePlainText)
Please add comment about |bool| return value.

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:162: {
This function should be in Editor class and avoid to use |bool| parameter.

[chongz, 2016-09-28 20:11:23]

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

[commit-bot: I haz the power, 2016-09-28 20:11:48]

Dry run: 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, 2016-09-28 21:24:04]

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

[commit-bot: I haz the power, 2016-09-28 21:24:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chongz, 2016-09-28 23:27:43]

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

[commit-bot: I haz the power, 2016-09-28 23:28:11]

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

[chongz, 2016-09-29 00:44:57]

Description was changed from

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

* Potential issues:
  1. JS could hack to merge two unrelated Drag and Drop by
    i. preventDefault() on first |insertFromDrop|
    ii. preventDefault() on second |deleteByDrag|
    iii. The leftovers will still get merged into a single undo entry

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=639139
==========

to

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=639139
==========

[commit-bot: I haz the power, 2016-09-29 01:01:23]

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

[commit-bot: I haz the power, 2016-09-29 01:01:24]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chongz, 2016-09-29 01:12:25]

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

[chongz, 2016-09-29 01:12:48]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-09-29 01:12:51]

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

[chongz, 2016-09-29 01:13:01]

Patchset #2 (id:40002) has been deleted

[chongz, 2016-09-29 02:36:22]

yosin@ PTAL again, thanks!

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/platform/linux/editing/pasteboard/drag-drop-modifies-page-expected.txt
(right):

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/platform/linux/editing/pasteboard/drag-drop-modifies-page-expected.txt:2:
EDITING DELEGATE:
webViewDidChangeSelection:WebViewDidChangeSelectionNotification
On 2016/09/28 04:19:03, Yosi_UTC9 wrote:
> I'll convert "drag-drop-modifies-page.html" to use w3c test harness.
> Once it is done, we don't bother with editing delegate logging.

Cool thanks!

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.cpp:724: m_lastEditCommand = cmd;
On 2016/09/28 04:19:03, Yosi_UTC9 wrote:
> On 2016/09/28 at 02:19:26, chongz wrote:
> > Has potential issue. These 2 commands might be created from 2 separate
> Drag&Drop, but I don't have a better solution for this...
> 
> How about having DragAndDropCommand, derived from CompositeEditCommand, which
> hold DeleteByDrag and InputFromDrop, if they aren't canceled?
> We hold DragAndDropCommand in m_lastEditCommand and check here like:
> 
> if (m_lastEditCommand && m_lastEditCommand->isDragAndDropCommand()) {
>   if (cmd is DeleteByDrag or cmd is InsertFromDrag)
>    m_lastEditCOmamnd->appendCommandToComposite(cmd);
> }
> 

Added command group wrapper |DragAndDropCommand|, which doesn't do anything by
itself, but could catch following |DeleteByDrag| and |InsertFromDrop|, and
combine into a single undo entry.

> In this way, it seems we don't need to introduce
CompositeEditCommand::append()
> and append for vector of EditCommand

I'm not sure what this means, do you mean we don't need
|EditCommandComposition::append(EditCommandComposition*)|?

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:300:
void CompositeEditCommand::appendCommandToComposite(CompositeEditCommand*
command)
On 2016/09/28 04:19:03, Yosi_UTC9 wrote:
> Good invention!
> We can implement Undo/Redo for drag-and-drop as easy as others.
> (^_^)b

Thanks! :>

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
(right):

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:843:
if (!m_referenceMovePosition.isNull()) {
On 2016/09/28 04:19:03, Yosi_UTC9 wrote:
> nit: s/!m_referenceMovePosition.isNull()/m_referenceMovePosition.isNotNull()/

Done.

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:844:
// Update the position otherwise it may become invalid after the selection is
deleted.
On 2016/09/28 04:19:03, Yosi_UTC9 wrote:
> Could you utilize |RelocatablePosition|?

Done.

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:141: bool
doDeleteByDragWithEvents(Element* dragTarget, bool smartDelete, const Position&
referenceMovePosition)
On 2016/09/28 04:19:03, Yosi_UTC9 wrote:
> Please add comment about |bool| return value.

Done.

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:142: {
On 2016/09/28 04:19:03, Yosi_UTC9 wrote:
> This function should be in Editor class and avoid to use |bool| parameter.

Done.

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:149: const bool
shouldDelete = dispatchBeforeInputEditorCommand(dragTarget,
InputEvent::InputType::DeleteByDrag, nullptr) ==
DispatchEventResult::NotCanceled;
On 2016/09/28 04:19:03, Yosi_UTC9 wrote:
> I think we should dispatch event to drag source rather than drag target since
> deletion is occurred at drag source rather than drag target.

Sorry for the confusion, I've renamed it to |dragSource|.

The naming story is I have 2 event targets |dragTarget| and |dropTarget|, where
|dragTarget| is essentially the event target of the "Drag" operation (i.g.
Source of Drag&Drop).

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:161: bool
doInsertFromDropWithEvents(Element* dropTarget, DragData* dragData,
DocumentFragment* fragment, Range* dropCaretRange, bool smartInsert, bool
chosePlainText)
On 2016/09/28 04:19:03, Yosi_UTC9 wrote:
> Please add comment about |bool| return value.

Done.

https://codereview.chromium.org/2374743002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:162: {
On 2016/09/28 04:19:03, Yosi_UTC9 wrote:
> This function should be in Editor class and avoid to use |bool| parameter.

Done.

[commit-bot: I haz the power, 2016-09-29 02:39:50]

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

[commit-bot: I haz the power, 2016-09-29 02:39:51]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2016-10-03 09:19:03]

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Editor.h (right):

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.h:62: enum class SmartDelete {
Yes, No };
How about enum class DeleteMode { Simple, Smart }?

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.h:63: enum class SmartInsert {
Yes, No };
How about enum class InsertMode { Simple, Smart }?

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.h:64: enum class ChosePlainText {
Yes, No };
How about DragSourceType { HTMLSource, PlainTextSource }?

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.h:117: void
deleteSelectionWithSmartDelete(bool smartDelete, InputEvent::InputType, const
Position& referenceMovePosition = Position());
Is it better to use |SmartDelete| enum class instead of bool?

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
(right):

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:847:
RelocatablePosition relocatableReferencePosition(m_referenceMovePosition);
Good usage!

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:941:
if (relocatableReferencePosition.position().isNotNull()) {
Could you use early-return style?

if (relocatableReferencePosition.position().isNull()) {
  clearTransientState();
  return;
}

...

It is OK to appear |clearTransientState()| twice.

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/DragAndDropCommand.h
(right):

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DragAndDropCommand.h:12: //
|DragAndDropCommand| is a dummy command. It doesn't do anything by itself, but
will act as a catcher for the following |DeleteByDrag| and |InsertFromDrop|
commands, and combine them into a single undo entry.
Could you reflow a comment to fit in 80 chars / line style?

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:547: const SmartInsert
canSmartReplace = SmartInsert::No;
nit: Since we have enum class for smart insert and plain text, we don't need to
have constant bool variable.

[chongz, 2016-10-03 22:25:09]

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

[commit-bot: I haz the power, 2016-10-03 22:25:37]

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

[chongz, 2016-10-03 22:27:01]

The CQ bit was unchecked by chongz@chromium.org

[chongz, 2016-10-03 22:27:11]

Patchset #3 (id:110001) has been deleted

[chongz, 2016-10-03 22:38:45]

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

[commit-bot: I haz the power, 2016-10-03 22:39:56]

Dry run: 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, 2016-10-03 22:59:04]

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

[commit-bot: I haz the power, 2016-10-03 22:59:05]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[chongz, 2016-10-03 23:39:23]

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

[chongz, 2016-10-03 23:39:40]

Patchset #3 (id:130001) has been deleted

[commit-bot: I haz the power, 2016-10-03 23:40:06]

Dry run: 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, 2016-10-04 01:14:38]

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

[commit-bot: I haz the power, 2016-10-04 01:14:39]

Dry run: This issue passed the CQ dry run.

[chongz, 2016-10-04 02:17:57]

Description was changed from

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=639139
==========

to

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

[chongz, 2016-10-04 02:18:39]

yosin@ I've updated as per your comments, PTAL again, thanks!

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Editor.h (right):

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.h:62: enum class SmartDelete {
Yes, No };
On 2016/10/03 09:19:03, Yosi_UTC9 wrote:
> How about enum class DeleteMode { Simple, Smart }?

Done.

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.h:63: enum class SmartInsert {
Yes, No };
On 2016/10/03 09:19:03, Yosi_UTC9 wrote:
> How about enum class InsertMode { Simple, Smart }?

Done.

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.h:64: enum class ChosePlainText {
Yes, No };
On 2016/10/03 09:19:03, Yosi_UTC9 wrote:
> How about DragSourceType { HTMLSource, PlainTextSource }?

Done.

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.h:117: void
deleteSelectionWithSmartDelete(bool smartDelete, InputEvent::InputType, const
Position& referenceMovePosition = Position());
On 2016/10/03 09:19:03, Yosi_UTC9 wrote:
> Is it better to use |SmartDelete| enum class instead of bool?

Done.

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
(right):

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:847:
RelocatablePosition relocatableReferencePosition(m_referenceMovePosition);
On 2016/10/03 09:19:03, Yosi_UTC9 wrote:
> Good usage!

Thanks!

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:941:
if (relocatableReferencePosition.position().isNotNull()) {
On 2016/10/03 09:19:03, Yosi_UTC9 wrote:
> Could you use early-return style?
> 
> if (relocatableReferencePosition.position().isNull()) {
>   clearTransientState();
>   return;
> }
> 
> ...
> 
> It is OK to appear |clearTransientState()| twice.
> 

Done.

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/DragAndDropCommand.h
(right):

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/DragAndDropCommand.h:12: //
|DragAndDropCommand| is a dummy command. It doesn't do anything by itself, but
will act as a catcher for the following |DeleteByDrag| and |InsertFromDrop|
commands, and combine them into a single undo entry.
On 2016/10/03 09:19:03, Yosi_UTC9 wrote:
> Could you reflow a comment to fit in 80 chars / line style?

Done.

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/2374743002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/DragController.cpp:547: const SmartInsert
canSmartReplace = SmartInsert::No;
On 2016/10/03 09:19:03, Yosi_UTC9 wrote:
> nit: Since we have enum class for smart insert and plain text, we don't need
to
> have constant bool variable.

Done.

[yosin_UTC9, 2016-10-04 07:37:43]

lgtm

[chongz, 2016-10-04 13:53:19]

chongz@chromium.org changed reviewers:
+ ojan@chromium.org

[chongz, 2016-10-04 13:53:20]

ojan@ PTAL, thanks!

[chongz, 2016-10-04 19:14:35]

CCing dtapuska@

[ojan, 2016-10-04 19:55:19]

lgtm

The code changes look good. Would be good to have the change description
describe the code changes at a high level as well though (e.g. using an empty
command group to get a single entry in the undo stack). In theory, someone
should be able to understand roughly what your patch is doing without looking at
the code.

[chongz, 2016-10-04 20:05:12]

Description was changed from

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

to

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Apply empty wrapper command |DragAndDropCommand| first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

[chongz, 2016-10-04 20:05:59]

Description was changed from

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Apply empty wrapper command |DragAndDropCommand| first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

to

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and   
      apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

[chongz, 2016-10-04 20:06:50]

Description was changed from

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and   
      apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

to

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and   
     apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry.

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

[chongz, 2016-10-04 20:08:54]

Description was changed from

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and   
     apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry.

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

to

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and   
     apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry.

---

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

[chongz, 2016-10-04 20:13:06]

On 2016/10/04 19:55:19, ojan wrote:
> lgtm
> 
> The code changes look good. Would be good to have the change description
> describe the code changes at a high level as well though (e.g. using an empty
> command group to get a single entry in the undo stack). In theory, someone
> should be able to understand roughly what your patch is doing without looking
at
> the code.

Sorry for the unclear description. I've added a high level summery for the
implementation, hope that works.

[chongz, 2016-10-04 20:13:51]

Description was changed from

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and   
     apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry.

---

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

to

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and   
     apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry.

--- After CL behavior

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

[chongz, 2016-10-04 20:30:31]

The CQ bit was checked by chongz@chromium.org

[commit-bot: I haz the power, 2016-10-04 20:31:24]

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, 2016-10-04 20:38:21]

Description was changed from

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and   
     apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry.

--- After CL behavior

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

to

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and   
     apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry.

--- After CL behavior

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

[commit-bot: I haz the power, 2016-10-04 20:38:22]

Committed patchset #3 (id:150001)

[commit-bot: I haz the power, 2016-10-04 20:41:34]

Description was changed from

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and   
     apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry.

--- After CL behavior

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393
==========

to

==========
[InputEvent] Support |deleteByDrag|, |insertFromDrop| and fire in sequential
order

Key point of the implementation:
  * We want to apply 2 commands but only push 1 undo entry.

Solution:
  1. Introduce empty wrapper command |DragAndDropCommand| and
     apply first
  2. Apply |DeleteSelectionCommand| for |deleteByDrag|
  3. Apply |ReplaceSelectionCommand| for |insertFromDrop|

The wrapper command in 1. won't create undo entry by itself, but will
act as a catcher and combine the following 2 commands into a single
undo entry.

--- After CL behavior

* Event order: (See [1])
  1. 'drop'
  2. 'beforeinput' InputType=|deleteByDrag|
  3. (DOM update for deletion)
  4. 'input' InputType=|deleteByDrag|
  5. 'beforeinput' InputType=|insertFromDrop|
  6. (DOM update for insertion)
  7. 'input' InputType=|insertFromDrop|
  8. 'dragend'

* Canceling 'beforeinput' will only prevent DOM update for current
  'beforeinput', and won't affect remaining events.
e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop'

* |dataTransfer| field:
  1. NULL for |deleteByDrag|
  2. Readonly |dataTransfer| for |insertFromDrop|

* Undo entry:
  1. Creates a single undo entry for Drag&Drop, if no JS-triggered DOM mutation
happened in between
  2. Otherwise creates 2 separate undo entries

For more detailed behavior please refer to LayoutTest.

[1] Event order for |deleteByDrag| and |insertFromDrop|:
https://github.com/w3c/input-events/issues/24#issuecomment-249153755

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEven...

BUG=652393

Committed: https://crrev.com/93db3c3bb91fb3e02080e16a0bd0b813b3d10a3e
Cr-Commit-Position: refs/heads/master@{#422915}
==========

[commit-bot: I haz the power, 2016-10-04 20:41:36]

Patchset 3 (id:??) landed as
https://crrev.com/93db3c3bb91fb3e02080e16a0bd0b813b3d10a3e
Cr-Commit-Position: refs/heads/master@{#422915}