DevTools: [Elements] Implement shortcut-based node cut-copy-pasting

DevTools: [Elements] Implement shortcut-based node cut-copy-pasting

R=aandrey, pfeldman, vsevik
BUG=394384
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179031

[apavlov, 2014-07-17 16:31:34]

PS2 adds visual marks to nodes cut/copied to the clipboard. See
http://imgur.com/SKCrrt1 for a screenshot.

[aandrey, 2014-07-18 05:30:05]

https://codereview.chromium.org/397303002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/397303002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorDOMAgent.cpp:1348: if (anchorNodeId &&
*anchorNodeId) {
dup if-

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/elements/ElementsPanel.js (right):

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/elements/ElementsPanel.js:1260: delete
this._clipboardNodeData;
remove

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/elements/ElementsPanel.js:1306:
node.moveTo(targetNode, null, expandCallback);
will it work for consecutive paste events? should "move" be executed only first
time, or prevent the second paste w/o copy/cut?

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/main/Main.js:548: if
(WebInspector.inspectorView.currentPanel() &&
WebInspector.inspectorView.currentPanel()["handleCutEvent"])
extract WebInspector.inspectorView.currentPanel() into a var

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/main/Main.js:554: if
(WebInspector.inspectorView.currentPanel() &&
WebInspector.inspectorView.currentPanel()["handlePasteEvent"])
ditto

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/DOMModel.js (right):

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/DOMModel.js:466: if (callback) {
call the callback after the copyText?

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/protocol...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/protocol...
Source/devtools/protocol.json:2299: { "name": "targetNodeId", "$ref": "NodeId",
"description": "Id of the element to copy into." },
to drop into

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/protocol...
Source/devtools/protocol.json:2300: { "name": "insertBeforeNodeId", "$ref":
"NodeId", "optional": true, "description": "Drop element copy before given one."
},
add into description what an empty insertBeforeNodeId would mean

[apavlov, 2014-07-18 09:02:08]

Thanks a lot for the thorough review, Andrey!

https://codereview.chromium.org/397303002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/397303002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorDOMAgent.cpp:1348: if (anchorNodeId &&
*anchorNodeId) {
On 2014/07/18 05:30:04, aandrey wrote:
> dup if-

Just to be absolutely sure :) Done.

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/elements/ElementsPanel.js (right):

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/elements/ElementsPanel.js:1260: delete
this._clipboardNodeData;
On 2014/07/18 05:30:04, aandrey wrote:
> remove

Done.

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/elements/ElementsPanel.js:1306:
node.moveTo(targetNode, null, expandCallback);
On 2014/07/18 05:30:05, aandrey wrote:
> will it work for consecutive paste events? should "move" be executed only
first
> time, or prevent the second paste w/o copy/cut?

Thanks, I knew I had missed something for this case. Let's forbid consecutive
pastes after a cut.

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/main/Main.js:548: if
(WebInspector.inspectorView.currentPanel() &&
WebInspector.inspectorView.currentPanel()["handleCutEvent"])
On 2014/07/18 05:30:05, aandrey wrote:
> extract WebInspector.inspectorView.currentPanel() into a var

Done.

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/main/Main.js:554: if
(WebInspector.inspectorView.currentPanel() &&
WebInspector.inspectorView.currentPanel()["handlePasteEvent"])
On 2014/07/18 05:30:05, aandrey wrote:
> ditto

Done.

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/DOMModel.js (right):

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/DOMModel.js:466: if (callback) {
On 2014/07/18 05:30:05, aandrey wrote:
> call the callback after the copyText?

The idea here was that if we have a callback, the default copy behavior should
not occur. Now I think this is not quite correct. Thanks.

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/protocol...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/protocol...
Source/devtools/protocol.json:2299: { "name": "targetNodeId", "$ref": "NodeId",
"description": "Id of the element to copy into." },
On 2014/07/18 05:30:05, aandrey wrote:
> to drop into

It is not clear WHAT we are going to drop. Expanded the description.

https://codereview.chromium.org/397303002/diff/20001/Source/devtools/protocol...
Source/devtools/protocol.json:2300: { "name": "insertBeforeNodeId", "$ref":
"NodeId", "optional": true, "description": "Drop element copy before given one."
},
On 2014/07/18 05:30:05, aandrey wrote:
> add into description what an empty insertBeforeNodeId would mean

Done.

[aandrey, 2014-07-18 09:18:15]

lgtm

[apavlov, 2014-07-18 12:24:32]

Updated patch to add Copy (formerly, Copy as HTML), Cut and Paste context menu
items.

[pfeldman, 2014-07-25 09:57:37]

https://codereview.chromium.org/397303002/diff/110001/Source/devtools/protoco...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/397303002/diff/110001/Source/devtools/protoco...
Source/devtools/protocol.json:2301: { "name": "deep", "type": "boolean",
"optional": true, "description": "Whether a deep clone should be made.
<code>true</code> by default." }
How can this be not deep?

[apavlov, 2014-07-25 10:02:04]

https://codereview.chromium.org/397303002/diff/110001/Source/devtools/protoco...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/397303002/diff/110001/Source/devtools/protoco...
Source/devtools/protocol.json:2301: { "name": "deep", "type": "boolean",
"optional": true, "description": "Whether a deep clone should be made.
<code>true</code> by default." }
On 2014/07/25 09:57:37, pfeldman wrote:
> How can this be not deep?

I'm just exposing the existing DOM API to our protocol clients, see
http://mdn.beonex.com/en/DOM/Node.cloneNode.html. If you feel we should forbid
shallow clones, I'll remove the parameter.

[apavlov, 2014-07-25 10:18:32]

On 2014/07/25 10:02:04, apavlov wrote:
>
https://codereview.chromium.org/397303002/diff/110001/Source/devtools/protoco...
> File Source/devtools/protocol.json (right):
> 
>
https://codereview.chromium.org/397303002/diff/110001/Source/devtools/protoco...
> Source/devtools/protocol.json:2301: { "name": "deep", "type": "boolean",
> "optional": true, "description": "Whether a deep clone should be made.
> <code>true</code> by default." }
> On 2014/07/25 09:57:37, pfeldman wrote:
> > How can this be not deep?
> 
> I'm just exposing the existing DOM API to our protocol clients, see
> http://mdn.beonex.com/en/DOM/Node.cloneNode.html. If you feel we should forbid
> shallow clones, I'll remove the parameter.

Removed.

[pfeldman, 2014-07-25 10:30:49]

protocol.json lgtm

[eustas, 2014-07-25 13:18:13]

lgtm

[apavlov, 2014-07-25 13:28:21]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-07-25 13:28:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/apavlov@chromium.org/397303002/150001

[aandrey, 2014-07-25 13:31:59]

The CQ bit was unchecked by aandrey@chromium.org

[aandrey, 2014-07-25 13:32:54]

not lgtm

also, can you extract the context menu code changes into a separate patch?

https://codereview.chromium.org/397303002/diff/150001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/397303002/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.cpp:1327: RefPtrWillBeRawPtr<Node>
clonedNode = node->cloneNode(true);
what if node==targetElement? should it be a noop?

https://codereview.chromium.org/397303002/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.cpp:1351: if (current == node) {
nit: i'd move this check inside the loop

https://codereview.chromium.org/397303002/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.cpp:1352: *errorString = "Unable to move
node into self or descendant";
i'd expect moving node to self to be a noop w/o error.

https://codereview.chromium.org/397303002/diff/150001/Source/devtools/front_e...
File Source/devtools/front_end/elements/ElementsTreeOutline.js (right):

https://codereview.chromium.org/397303002/diff/150001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:234: if
(this._clipboardNodeData.isCut && (node === targetNode ||
node.isAncestor(targetNode)))
node.isSelfOrAncestor(targetNode)

https://codereview.chromium.org/397303002/diff/150001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:1606:
contextMenu.appendItem(WebInspector.UIString("Copy"),
treeOutline._copyNode.bind(treeOutline));
This callbacks should always work regardless of the window.getSelection() result
and WebInspector.isBeingEdited(). Also it should work on the node, for which the
context menu was open, not for the selectedDOMNode.

https://codereview.chromium.org/397303002/diff/150001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:1609:
contextMenu.appendItem(WebInspector.UIString("Paste"),
treeOutline._pasteNode.bind(treeOutline));
maybe always append this option, but make disabled if cannot paste

[apavlov, 2014-07-25 13:35:44]

The CQ bit was unchecked by apavlov@chromium.org

[apavlov, 2014-07-25 15:04:48]

Pavel explicitly asked me to implement the context menu items, hence PS4.

https://codereview.chromium.org/397303002/diff/150001/Source/core/inspector/I...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/397303002/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.cpp:1327: RefPtrWillBeRawPtr<Node>
clonedNode = node->cloneNode(true);
On 2014/07/25 13:32:54, aandrey wrote:
> what if node==targetElement? should it be a noop?

No - a clone of the node with its children will be appended to the nodes's
children list instead. It's an entirely valid case.

https://codereview.chromium.org/397303002/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.cpp:1351: if (current == node) {
On 2014/07/25 13:32:54, aandrey wrote:
> nit: i'd move this check inside the loop

Done.

https://codereview.chromium.org/397303002/diff/150001/Source/core/inspector/I...
Source/core/inspector/InspectorDOMAgent.cpp:1352: *errorString = "Unable to move
node into self or descendant";
On 2014/07/25 13:32:54, aandrey wrote:
> i'd expect moving node to self to be a noop w/o error.

Moving a node to its own list of children is definitely an impossible situation
and should be considered an error. These operations must either change the DOM
or return an error. Otherwise, clients will have hard time figuring out why the
DOM has not changed.

https://codereview.chromium.org/397303002/diff/150001/Source/devtools/front_e...
File Source/devtools/front_end/elements/ElementsTreeOutline.js (right):

https://codereview.chromium.org/397303002/diff/150001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:234: if
(this._clipboardNodeData.isCut && (node === targetNode ||
node.isAncestor(targetNode)))
On 2014/07/25 13:32:54, aandrey wrote:
> node.isSelfOrAncestor(targetNode)

This works for ordinary DOM Node's, not our WI.DOMNode's

https://codereview.chromium.org/397303002/diff/150001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:1606:
contextMenu.appendItem(WebInspector.UIString("Copy"),
treeOutline._copyNode.bind(treeOutline));
On 2014/07/25 13:32:54, aandrey wrote:
> This callbacks should always work regardless of the window.getSelection()
result
> and WebInspector.isBeingEdited().
> Also it should work on the node, for which the
> context menu was open, not for the selectedDOMNode.

Done

https://codereview.chromium.org/397303002/diff/150001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:1609:
contextMenu.appendItem(WebInspector.UIString("Paste"),
treeOutline._pasteNode.bind(treeOutline));
On 2014/07/25 13:32:54, aandrey wrote:
> maybe always append this option, but make disabled if cannot paste

Done.

[aandrey, 2014-07-25 15:44:27]

lgtm

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
File Source/devtools/front_end/elements/ElementsTreeOutline.js (right):

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:174: _cutNode:
function(node)
remove this in favor of .bind(..., true)?

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:182: _copyNode:
function(node)
ditto

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:193:
this._setClipboardData(null);
should it be after all the returns?

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:219:
this._setClipboardData({node: node, isCut: isCut});
nit: spaces after { and before }

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:246: _pasteNode:
function(targetNode)
remove this (looks like the code would be a lot simpler w/o these 3 intermediate
functions)?

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:265:
event.preventDefault();
maybe event.consume(false); ?

[apavlov, 2014-07-25 16:09:57]

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
File Source/devtools/front_end/elements/ElementsTreeOutline.js (right):

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:174: _cutNode:
function(node)
On 2014/07/25 15:44:27, aandrey wrote:
> remove this in favor of .bind(..., true)?

Done.

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:182: _copyNode:
function(node)
On 2014/07/25 15:44:27, aandrey wrote:
> ditto

Done.

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:193:
this._setClipboardData(null);
On 2014/07/25 15:44:26, aandrey wrote:
> should it be after all the returns?

Nope. We don't want users to go dizzy about having a node AND some unrelated
text in the clipboard. A copy or cut shortcut will clear the clipboard up front
and then, perhaps, put something in there.

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:219:
this._setClipboardData({node: node, isCut: isCut});
On 2014/07/25 15:44:27, aandrey wrote:
> nit: spaces after { and before }

Done.

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:246: _pasteNode:
function(targetNode)
On 2014/07/25 15:44:26, aandrey wrote:
> remove this (looks like the code would be a lot simpler w/o these 3
intermediate
> functions)?

Done.

https://codereview.chromium.org/397303002/diff/170001/Source/devtools/front_e...
Source/devtools/front_end/elements/ElementsTreeOutline.js:265:
event.preventDefault();
On 2014/07/25 15:44:27, aandrey wrote:
> maybe event.consume(false); ?

true, since we need to preventDefault(). Done.

[apavlov, 2014-07-25 16:10:02]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-07-25 16:11:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/apavlov@chromium.org/397303002/210001

[commit-bot: I haz the power, 2014-07-25 17:17:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/17123)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18971)

[commit-bot: I haz the power, 2014-07-25 17:41:29]

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

[commit-bot: I haz the power, 2014-07-25 17:41:31]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[apavlov, 2014-07-28 08:43:57]

The CQ bit was checked by apavlov@chromium.org

[apavlov, 2014-07-28 08:44:14]

The CQ bit was unchecked by apavlov@chromium.org

[apavlov, 2014-07-28 10:30:38]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-07-28 10:31:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/apavlov@chromium.org/397303002/250001

[commit-bot: I haz the power, 2014-07-28 11:34:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19203)

[commit-bot: I haz the power, 2014-07-28 12:33:35]

Change committed as 179031