Implement more paste commands for the inspector panel

RESOLVED FIXED in Firefox 36

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: fayolle-florent, Assigned: fayolle-florent)

Tracking

(Blocks 1 bug, {dev-doc-complete})

36 Branch
Firefox 36
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 36+)

Details

Attachments

(2 attachments, 5 obsolete attachments)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141103144234

Steps to reproduce:

First reported here:
https://github.com/firebug/firebug.next/issues/183

After bug 993416, we should consider fully reimplement the paste commands that present currently in Firebug 2.0. These are :
- paste HTML > "Replace Content" (inner HTML)
- paste HTML > "As First Child"
- paste HTML > "As Last Child"
- paste HTML > "Before"
- paste HTML > "After"

Florent
Blocks: firebug-gaps
Component: Untriaged → Developer Tools: Inspector
OS: Linux → All
Hardware: x86_64 → All
Assignee: nobody → fayolle-florent
Status: UNCONFIRMED → NEW
Ever confirmed: true
First version of my patch (still WIP, but everything looks to work).

Some questions though: 
- Should we prevent user from modifying inner html on elements that don't expect any child? Is there any property/method to detect this kind of element?
- 6 paste commands clutters the context menu a bit. Maybe we should introduce a submenu for the last 4 items? (as first/last child and before/after)
- Accesskeys for these commands (if any)?

TODO:
- throw instead of returning for WalkerActor.insertAdjacentHTML
- Tests

Florent
Posted patch 1095521.patch (obsolete) — Splinter Review
I just glanced quickly at the code, looks good so far. Please ask for feedback (F?) or review (R?) when you feel ready to get a proper review.

(In reply to fayolle-florent from comment #1)
> First version of my patch (still WIP, but everything looks to work).
> 
> Some questions though: 
> - Should we prevent user from modifying inner html on elements that don't
> expect any child? Is there any property/method to detect this kind of
> element?
I'm not aware of any such way to detect this, but it sounds like there wouldn't be a lot of elements we want to prevent the modification on anyway. We should simply check whether a given paste command is available for the current node (by checking its type/tagName) and enable/disable the corresponding items.
> - 6 paste commands clutters the context menu a bit. Maybe we should
> introduce a submenu for the last 4 items? (as first/last child and
> before/after)
Yes, a submenu would be great. Top-level menu items should only be for things to get used the most, and should therefore be quick to scan.
> - Accesskeys for these commands (if any)?
Yes, if you can find simple access keys. The problem for an access key is finding the right letter, one that isn't already taken, and that actually appears in the label. Also (on mac at least), access keys aren't discoverable at all.
> TODO:
> - throw instead of returning for WalkerActor.insertAdjacentHTML
Indeed, throwing would be better here, with a corresponding error message. Throwing errors in protocol methods end up rejecting the promise on the client-side, so this gives a nice way for the markup-view to handle the error and react any way it wants.
> - Tests
Yes!
Posted patch 1095521.patch (obsolete) — Splinter Review
> I just glanced quickly at the code, looks good so far. Please ask for feedback (F?) 
> or review (R?) when you feel ready to get a proper review.

Right! Here is my progress:
- added accesskeys
- created a submenu
- throw exceptions for WalkerActor.insertAdjacentHTML

If that is OK for you, I begin writing tests.

> I'm not aware of any such way to detect this, but it sounds like there wouldn't be a lot of elements 
> we want to prevent the modification on anyway. We should simply check whether a given paste command 
> is for the current node (by checking its type/tagName) and enable/disable the corresponding items.

Hmm, lots of maintenance here I fear, for not much worth.

Florent
Attachment #8519487 - Attachment is obsolete: true
Attachment #8519851 - Flags: feedback?(pbrosset)
Posted patch 1095521.patch (obsolete) — Splinter Review
Patch with the tests.

I took the liberty to change the refont the test page (browser/devtools/inspector/test/doc_inspector_menu.html). Is that OK for you?

Also added that question (which is probably a bikeshedding):
> +  // xxxFlorent: (nit) looks like never used, right? (unless there are other files that access it?)

Florent
Attachment #8519851 - Attachment is obsolete: true
Attachment #8519851 - Flags: feedback?(pbrosset)
Attachment #8520758 - Flags: review?(pbrosset)
Comment on attachment 8520758 [details] [diff] [review]
1095521.patch

Review of attachment 8520758 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this patch.

Nit: there's no commit message in the patch. You can add one by doing 'hg qref -m "Bug 1095521 - blah blah blah blah; r=pbrosset"

The biggest comment I have is about backward compatibility.
Here's the thing, using any version of firefox, we can connect to any other version of firefox via the remote debugging protocol. So it means that there is no guaranty that the markup-view.js or inspector-panel.js will be able to access the new methods you have added in inspector.js.

Think about this use case for instance: once your patch is in nightly, someone uses nightly to connect to an older firefoxos 1.4 phone via WebIDE. This fxos version is not going to have the new inspector.js file, and therefore the inspector will fail.

We usually have, what we call, "traits" on the server (on the root actor actually) to let the client know what features to expect of the server. Based on these traits (which are usually booleans), we enable/disable features on the client-side.
So one way would be to add a new trait to /toolkit/devtools/server/actors/root.js (search for "traits" in this file). Maybe something like HTMLPasteEnabled: true or something like that, and test this on the client (see get isOuterHTMLEditable in inspector-panel.js).
The client could hide the new menu items altogether if the trait isn't there.

Another way is to test for methods on the front (which is the client-side representation of the actor). All the methods you added to the WalkerActor can be called from the client, and this is made possible because front-sides of these methods were auto-implemented on WalkerFront.
So in markup-view.js, you could simply do things like: |if (this.walker.removeNodes) { ... }| or |if (this.walker.insertAdjacentHTML) { ... }| to show/hide things in the UI or enable/disable features or prevent the code from executing functions it cannot.

Please see my other comments in the code below:

::: browser/devtools/framework/selection.js
@@ +195,5 @@
>  
> +  /**
> +   * @returns true if the selection is the <body> HTML element.
> +   */
> +  isBody: function() {

Can you please rename this method |isBodyNode|, move it near the bottom of the file, next to |isDocumentNode|, and use |this.nodeFront| instead of |this._nodeFront|.
This is just to make it more consistent with the rest of the file.

@@ +204,5 @@
> +
> +  /**
> +   * @returns true if the selection is the <head> HTML element.
> +   */
> +  isHead: function() {

Same comments as for isBody

::: browser/devtools/inspector/inspector-panel.js
@@ +57,4 @@
>    this.panelDoc = iframeWindow.document;
>    this.panelWin = iframeWindow;
>    this.panelWin.inspector = this;
> +  // xxxFlorent: (nit) looks like never used, right? (unless there are other files that access it?)

I think you're right, |this._inspector| doesn't seem to be used in this file, and neither from other files. Let's remove it.

@@ +648,5 @@
> +    let pasteBefore = this.panelDoc.getElementById("node-menu-pastebefore");
> +    let pasteAfter = this.panelDoc.getElementById("node-menu-pasteafter");
> +    let pasteFirstChild = this.panelDoc.getElementById("node-menu-pastefirstchild");
> +    let pasteLastChild = this.panelDoc.getElementById("node-menu-pastelastchild");
> +    // Is the clipboard content appropriate? Is the element editable?

nit: please add an empty line before this comment, to space things out a little.

@@ +817,5 @@
> +   * Paste the contents of the clipboard into the selected Node's inner HTML.
> +   */
> +  pasteInnerHTML: function InspectorPanel_pasteInnerHTML() {
> +    let content = this._getClipboardContentForPaste();
> +    if (content) {

Usually, in the devtools codebase, when a whole method body can only be executed if a certain condition is true, we invert the condition and early return, to avoid indenting the whole function.

Also, this method is async, please make it return a promise. Something like this:

let content = this._getClipboardContentForPaste();
if (!content) {
  return promise.reject();
}

let node = this.selection.nodeFront;
return this.markup.getNodeInnerHTML(node).then(oldContent => {
  this.markup.updateNodeInnerHTML(node, content, oldContent);
});

@@ +830,5 @@
> +   * Paste the contents of the clipboard as adjacent HTML to the selected Node.
> +   * @param position The position as specified for Element.insertAdjacentHTML
> +   *        (i.e. "beforeBegin", "afterBegin", "beforeEnd", "afterEnd").
> +   */
> +  pasteAdjacentHTML: function InspectorPanel_pasteAdjacent(position) {

Same comments than for method pasteInnerHTML.

::: browser/devtools/inspector/test/browser_inspector_menu.js
@@ +63,4 @@
>    },
>  ];
>  
> +const PASTE_ADJACENT_HTML_DATA = [

Can you please move these new test steps and corresponding functions into a new test file?
This will help keep this file to a reasonable size and help with maintainability.
You can name the new file as simply as browser_inspector_menu-02.js and rename this one with a -01.js at the end.
This is a pattern we often use in our tests.

@@ +285,5 @@
> +    yield selectNode(nodeFront, inspector);
> +    let markupTagLine = getContainerForNodeFront(nodeFront, inspector).tagLine;
> +
> +    for (let data of PASTE_ADJACENT_HTML_DATA) {
> +      let { desc, clipboardData, menuId } = data;

nit: you can get rid of this line if you destructure the object in the for statement directly: for (let {desc, clipboardData, menuId} of PASTE_ADJACENT_HTML_DATA)

::: browser/devtools/markupview/markup-view.js
@@ +935,5 @@
> +   * Replace the innerHTML of any node displayed in the inspector with
> +   * some other HTML code
> +   * @param aNode node which innerHTML will be replaced.
> +   * @param newValue The new innerHTML to set on the node.
> +   * @param oldValue The old innerHTML that will be used if the user undos the update.

nit: missing jsdoc @param types.

@@ +938,5 @@
> +   * @param newValue The new innerHTML to set on the node.
> +   * @param oldValue The old innerHTML that will be used if the user undos the update.
> +   * @returns A promise that will resolve when the inner HTML has been updated.
> +   */
> +  updateNodeInnerHTML: function(aNode, newValue, oldValue) {

About |aNode|: we don't usually name arguments like this anymore. Just |node| is fine here.

@@ +949,5 @@
> +
> +    container.undo.do(() => {
> +      this.walker.setInnerHTML(aNode, newValue).then(def.resolve, def.reject);
> +    }, () => {
> +      this.walker.setInnerHTML(aNode, oldValue).then(def.resolve, def.reject);

The _undo_ part here should not resolve or reject the promise. The promise would already have been resolved or rejected by the _do_ part.

@@ +961,5 @@
> +   *
> +   * @param aNode The reference node.
> +   * @param position The position as specified for Element.insertAdjacentHTML
> +   *        (i.e. "beforeBegin", "afterBegin", "beforeEnd", "afterEnd").
> +   * @param newValue The adjacent HTML.

nit: missing jsdoc @param types:
@param {NodeFront} node The reference node.
@param {String} position The pos....

Also missing @return jsdoc, it should say that this method returns a promise.

@@ +963,5 @@
> +   * @param position The position as specified for Element.insertAdjacentHTML
> +   *        (i.e. "beforeBegin", "afterBegin", "beforeEnd", "afterEnd").
> +   * @param newValue The adjacent HTML.
> +   */
> +  insertAdjacentHTMLToNode: function(aNode, position, value) {

About |aNode|: we don't usually name arguments like this anymore. Just |node| is fine here.

@@ +973,5 @@
> +    let def = promise.defer();
> +
> +    let injectedNodes = [];
> +    container.undo.do(() => {
> +      this.walker.insertAdjacentHTML(aNode, position, value).then((nodes) => {

Parens () around nodes aren't needed.

@@ +977,5 @@
> +      this.walker.insertAdjacentHTML(aNode, position, value).then((nodes) => {
> +        return injectedNodes = nodes;
> +      }).then(def.resolve, def.reject);
> +    }, () => {
> +      this.walker.removeNodes(injectedNodes).then(def.resolve, def.reject);

The _undo_ part here should not resolve or reject the promise. The promise would already have been resolved or rejected by the _do_ part.

::: toolkit/devtools/server/actors/inspector.js
@@ +2054,5 @@
>  
>    /**
> +   * Set a node's innerHTML property.
> +   */
> +  setInnerHTML: method(function(node, value) {

I think this method should do some input validation.

node here is a "domnode" type, which is anything you can see in the markup-view tree. This means this can be a DOCTYPE node, a <!-- comment --> node, a #text node, it could also be a #document node (which is what you see below iframe nodes).

So this method should make sure node is what we want it to be before setting the innerHTML (we can't only rely on the client-side to do this).

@@ +2060,5 @@
> +    rawNode.innerHTML = value;
> +  }, {
> +    request: {
> +      node: Arg(0, "domnode"),
> +      value: Arg(1),

value: Arg(1, "string")

@@ +2063,5 @@
> +      node: Arg(0, "domnode"),
> +      value: Arg(1),
> +    },
> +    response: {
> +    }

nit: move the closing } on the line above, next to {

@@ +2141,5 @@
> +   *
> +   * @param {Node} node
> +   * @param {string} position The position whose value has to be one of the
> +   *        possible values of the first argument of Element.insertAdjacentHTML.
> +   *        (i.e. "beforeBegin", "afterBegin", "beforeEnd", "afterEnd").

nit: please rephrase to something like:
@param {String} position One of "beforeBegin", "afterBegin", "beforeEnd", "afterEnd" (see Element.insertAdjacentHTML).

@@ +2144,5 @@
> +   *        possible values of the first argument of Element.insertAdjacentHTML.
> +   *        (i.e. "beforeBegin", "afterBegin", "beforeEnd", "afterEnd").
> +   * @param {string} value The HTML content.
> +   */
> +  insertAdjacentHTML: method(function(node, position, value) {

Can you test the value of position and throw if it is invalid?

@@ +2152,5 @@
> +    if (node.isDocumentElement()) {
> +      throw new Error("Can't insert adjacent element to the root.");
> +    }
> +
> +    let insertAsSibling = position === "beforeBegin"

Boolean flag variables should have names that start with "is" or "has" or similar. Like "isSiblingInsertion"

@@ +2161,5 @@
> +        "or the head.");
> +    }
> +
> +    let rawParentNode = rawNode.parentNode;
> +    if (!rawParentNode && isBeforeOrAfter) {

isBeforeOrAfter isn't defined in this file.

@@ +2166,5 @@
> +      throw new Error("Can't insert as sibling without parent node.");
> +    }
> +
> +    // We can't use insertAdjacentHTML, because we want to return the nodes
> +    // being created (so the front can remove them if the user undo the change).

if the user *undoes the change

@@ +2172,5 @@
> +    let range = rawNode.ownerDocument.createRange();
> +    if (position === "beforeBegin" || position === "afterEnd") {
> +      range.selectNode(rawNode);
> +    }
> +    else {

nit: else on same line as closing brace above.

@@ +2195,5 @@
> +      case "beforeEnd":
> +        rawNode.appendChild(docFrag);
> +        break;
> +    }
> +    return newRawNodes.map(newRawNode => new NodeActor(this, newRawNode));

I think you should instead return a |disconnectedNodeArray| return value type.
Take a look at the |items| method which uses it.

@@ +2199,5 @@
> +    return newRawNodes.map(newRawNode => new NodeActor(this, newRawNode));
> +  }, {
> +    request: {
> +      node: Arg(0, "domnode"),
> +      position: Arg(1),

position: Arg(1, "string"),

@@ +2200,5 @@
> +  }, {
> +    request: {
> +      node: Arg(0, "domnode"),
> +      position: Arg(1),
> +      value: Arg(2)

value: Arg(2, "string")

@@ +2227,5 @@
> +        node.rawNode.parentNode.removeChild(node.rawNode);
> +        // Mutation events will take care of the rest.
> +      }
> +    }
> +    return nextSibling;

It feels weird for this method to return the nextSibling of the last node. Why the last one? I understand this is required for the |removeNode| method below, but I would make this other method do this instead.

@@ +2256,5 @@
> +    request: {
> +      node: Arg(0, "array:domnode")
> +    },
> +    response: {
> +    }

nit: closing } on the previous line

@@ +2258,5 @@
> +    },
> +    response: {
> +    }
> +  }),
> +

nit: extra empty line here.
Attachment #8520758 - Flags: review?(pbrosset)
Posted patch 1095521.patch (obsolete) — Splinter Review
Thanks for this great review! 
This is my current update, though it may fail in running some tests that run without the patch. So a thorough review is not required at the moment.

Some questions from my side:
1. In browser/devtools/markupview/markup-view.js, _flashMutatedNodes looks to throw exceptions and prevents from triggering the "markupmutation" event (see my comment in my patch). Do you know how to fix that cleanly? Maybe emit the event before flashing mutated nodes (if that doesn't change the behavior of the inspector)?
2. I refactored a bit in toolkit/devtools/server/actors/inspector.js, so both NodeList.items and Walker.insertAdjacentHTML use the new method Walker.attachElements. I am not sure what Walker.attachElement was intended to do so I documented it as I understood. Is that fine for you?

Florent
Attachment #8520758 - Attachment is obsolete: true
Comment on attachment 8523562 [details] [diff] [review]
1095521.patch

Review of attachment 8523562 [details] [diff] [review]:
-----------------------------------------------------------------

I updated with a fresh update of mozilla-central and recompile the whole. Now I just get one error for both:

> 3499 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_highlighter-comments.js | Test timed out - expected PASS
SUITE-END | took 199s

Also my previous questions remain :).

Florent
Attachment #8523562 - Flags: review?(pbrosset)
(In reply to fayolle-florent from comment #8)
> 1. In browser/devtools/markupview/markup-view.js, _flashMutatedNodes looks
> to throw exceptions and prevents from triggering the "markupmutation" event
> (see my comment in my patch). Do you know how to fix that cleanly? Maybe
> emit the event before flashing mutated nodes (if that doesn't change the
> behavior of the inspector)?
Let's make _flashMutatedNodes not throw by fixing the root cause of this problem. There shouldn't a way for a container to be null/undefined here.
Debugging this issue really quickly, I can see that when we go in the |else if (type === "childList")| and we iterate on |added|, then |this.getContainer(added)| apparently sometimes returns undefined. Which it shouldn't.
Can you debug a little further than that? Try to understand why undefined is returned in this case? And if it is expected, then add an |if (addedContainer)| around |addedOrEditedContainers.add(addedContainer);|. But if it's not expected, try and find the root cause of this too.
Let me know if you need more help with this.

> 2. I refactored a bit in toolkit/devtools/server/actors/inspector.js, so
> both NodeList.items and Walker.insertAdjacentHTML use the new method
> Walker.attachElements. I am not sure what Walker.attachElement was intended
> to do so I documented it as I understood. Is that fine for you?
These changes look fine to me at first glance.

I'll have a quick look at the rest of the code now. But as you said, this isn't ready for review yet, so please do ping me when it is.
Comment on attachment 8523562 [details] [diff] [review]
1095521.patch

Review of attachment 8523562 [details] [diff] [review]:
-----------------------------------------------------------------

I've only quickly glanced at this patch, and made a few comments.
I see my comments have been addressed, even the root actor trait thing, thanks for that!
So this is very close. Please ask for a new review once the flash thing is fixed or if you need help.

::: browser/devtools/inspector/test/browser_inspector_menu-02.js
@@ +50,5 @@
> +];
> +
> +// Test context menu functionality:
> +// 1) menu items are disabled/enabled depending on the clicked node
> +// 2) actions triggered by the items work correctly

Please move this global test comment at the top of the file (after "use strict"), this is the first thing a reader should see.

::: browser/devtools/markupview/markup-view.js
@@ +971,5 @@
> +   * @param {NodeFront} node The reference node.
> +   * @param {string} position The position as specified for Element.insertAdjacentHTML
> +   *        (i.e. "beforeBegin", "afterBegin", "beforeEnd", "afterEnd").
> +   * @param {string} newValue The adjacent HTML.
> +   * @returns A promise that will resolve when the adjacent HTML has 

nit: trailing whitespace

::: toolkit/devtools/server/actors/inspector.js
@@ +1326,5 @@
> +    }
> +
> +    return {
> +      nodes: nodeActors,
> +      newParents: Array.from(newParents)

nit: these 3 are identical in this case:
- Array.from(newParents)
- [parent for (parent of newParents]
- [...newParents]
But I guess I prefer the last one because it's shorter. Up to you, Array.from is fine here too, it is probably easier to read too.
Attachment #8523562 - Flags: review?(pbrosset)
Posted patch 1095521.patch (obsolete) — Splinter Review
> Debugging this issue really quickly, I can see that when we go in the |else if (type === "childList")| 
> and we iterate on |added|, then |this.getContainer(added)| apparently sometimes returns undefined. 
> Which it shouldn't.

Right, in my mind it was something more complex than I thought... So yes, I imagine that a container exist only if the corresponding node is displayed in the inspector panel (or/and maybe in other places). 
So in that specific case, if the added nodes are not displayed because their parent node are collapsed, |this.getContainer(added)| returns undefined, which is normal (so we have to test the value of |addedContainer|). 

That should be fixed now.

> Please move this global test comment at the top of the file (after "use 
> strict"), this is the first thing a reader should see.
Done.

> nit: trailing whitespace
Oops, fixed, thanks!

> nit: these 3 are identical in this case:
> - Array.from(newParents)
> - [parent for (parent of newParents]
> - [...newParents]
> But I guess I prefer the last one because it's shorter. Up to you, Array.from is fine here too, it is probably easier to read too.

Right. I would have thought the last one would have been much less optimized than the third one (I made some tests months ago). 
I tested again today, and found that the |Array.from()| is just ~1.5x faster than the third one. So I guess we don't really care, and I put your choice (which I slightly prefer too).

TODO:
- Rebase again with my patch
- Ultimate test run with the last version of this patch...

So no need to review yet.

Florent
Attachment #8523562 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 8524892 [details] [diff] [review]
1095521.patch

Review of attachment 8524892 [details] [diff] [review]:
-----------------------------------------------------------------

Tests are OK for browser/devtools/inspector and browser/devtools/markupview (I can run the tests fully if needed).

I think it is ready for review.

Florent
Attachment #8524892 - Flags: review?(pbrosset)
Test results show 9 failures for (what I think is) unrelated tests:
- 38800 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/netmonitor/test/browser_net_charts-03.js | The second sum's value is correct. - Got World 36,60, expected World 36.60
- 38801 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/netmonitor/test/browser_net_timing-division.js | The division on the seconds time scale looks legit. - 
- 38802 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveui.js | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get] at resource://gre/modules/FileUtils.jsm:63
- 38803 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveui.js | Test timed out - expected PASS
- 38804 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/responsivedesign/test/browser_responsiveui.js | Found a tab after previous test timed out: data:text/html,mop - expected PASS
- 38805 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_graphs-09a.js | The average tooltip displays the correct value. - Got 41,71, expected 41.71
- 38806 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_num-l10n.js | The first number was properly localized. - Got 1 234,56, expected 1,234.56
- 38807 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_console_keyboard_accessibility.js | scroll position now at bottom - Got 1602, expected 1601
- 38808 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_597460_filter_scroll.js | scroll location is correct - 


See the attached file for details if you think it is worthy.

Florent
Comment on attachment 8524892 [details] [diff] [review]
1095521.patch

Review of attachment 8524892 [details] [diff] [review]:
-----------------------------------------------------------------

The code changes look good to me. R+ with the few nits listed below fixed.
I have tested the patch locally, couldn't find any issues. I've also tested it with a fxos simulator that doesn't have the server-side code, just to make sure the backward compatibility was handled correctly. No problems there too.
So I pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c7dca5d19192
Let's wait and see if it turns green. If it does, let's go ahead and land this patch.

::: browser/devtools/inspector/test/browser_inspector_menu-02.js
@@ +222,5 @@
> +      "The original node was removed.");
> +    // TODO Bug 1100001
> +    /*yield undoChange(inspector);
> +    ok(getNode(outerHTMLSelector), "The original node has been restored " +
> +      "after undo.");*/

nit: remove these 4 commented lines. The bug is created anyway.

::: browser/devtools/inspector/test/doc_inspector_menu-02.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <title>Inspector Tree Menu Test</title>

Please add <meta charset="utf-8">
to avoid warnings in the logs.

::: browser/devtools/inspector/test/head.js
@@ +651,5 @@
>      return promise.resolve();
>    }
>  }
> +
> +// Stolen from marupview/test/head.js

No need for this comment. Yes we do have some major code duplication in our head.js files, but it's not a huge deal, they are not a big maintainability problem.

@@ +672,5 @@
> +  inspector.markup.undo.undo();
> +  return mutated;
> +}
> +
> +// Stolen from marupview/test/head.js

same here

::: browser/devtools/markupview/markup-view.js
@@ +808,5 @@
> +   * @param outer A boolean that, if true, makes the function return the
> +   *              outerHTML, otherwise the innerHTML.
> +   * @returns A promise that will be resolved with the outerHTML / innerHTML.
> +   */
> +  _getNodeHTML: function(aNode, outer) {

Please rename the boolean parameter to isOuter

@@ +815,5 @@
> +
> +    if (outer)
> +      walkerPromise = this.walker.outerHTML(aNode);
> +    else
> +      walkerPromise = this.walker.innerHTML(aNode);

Parenthesis around if/else bodies please.
if (...) {
  ...
} else {
  ...
}

@@ +823,5 @@
> +        longstr.release().then(null, console.error);
> +        def.resolve(html);
> +      });
> +    });
> +    return def.promise;

nit: you can avoid creating the extra promise by rewriting this way:

return walkerPromise.then(longstr => {
  return longstr.string().then(html => {
    longstr.release().then(null, console.error);
    return html;
  });
});

This way you can remove |let def ...| at the top and the |return def.promise| at the end.

@@ +841,5 @@
> +   * @param aNode The NodeFront to get the innerHTML for.
> +   * @returns A promise that will be resolved with the innerHTML.
> +   */
> +  getNodeInnerHTML: function(aNode) {
> +    return this._getNodeHTML(aNode, false);

nit: you can remove the 2nd parameter here.

::: browser/locales/en-US/chrome/browser/devtools/inspector.dtd
@@ +13,4 @@
>  <!ENTITY inspectorHTMLPasteOuter.label      "Paste Outer HTML">
>  <!ENTITY inspectorHTMLPasteOuter.accesskey  "P">
>  
> +<!ENTITY inspectorHTMLPasteInner.label      "Paste Inner HTML">

So, I know this file doesn't have any localization comments yet, but it should, this helps i10n people localize the software easier.
So can you make sure all the new properties you added are commented?

You can stick a comment like this before each ENTITY:

<!-- LOCALIZATION NOTE (inspectorHTMLPasteInner.label): This is the label shown in the inspector contextual-menu for the item that lets users paste inner HTML in the current node -->

::: toolkit/devtools/server/actors/inspector.js
@@ +2213,5 @@
> +      case "beforeBegin":
> +        rawParentNode.insertBefore(docFrag, rawNode);
> +        break;
> +      case "afterEnd":
> +        // Note: if the second argument is null, rawParentNode.insertBefore 

nit: trailing whitespace at the end of this line.

@@ +2226,5 @@
> +      default:
> +        throw new Error('Invalid position value. Must be either ' +
> +          '"beforeBegin", "beforeEnd", "afterBegin" or "afterEnd".');
> +    }
> +    return this.attachElements(newRawNodes);

nit: add an empty line before the return statement.

@@ +2243,5 @@
> +   * throw an exception.
> +   *
> +   * @param {NodeActor} node The node to remove.
> +   */
> +  _earlyCheckForRemoveNode: function(node) {

Better name functions after what they actually do, not after who uses them.
The name you've chosen here kind of make the function unusable by any other functions than removeNode(s).
Someone in the future might think this function is very suited to their use case.

Can you please do the following things:
- rename it isDocumentOrDocumentElementNode
- make it return a boolean only (not throw)
- change the jsdoc at the top
- make removeNode and removeNodes throw instead if the function returns true

::: toolkit/devtools/server/actors/root.js
@@ +120,4 @@
>    traits: {
>      sources: true,
>      editOuterHTML: true,
> +    pasteHTML: true,

Can you add a comment above pasteHTML that explains what this trait is about?
And while you're at it, also add one above editOuterHTML.
Attachment #8524892 - Flags: review?(pbrosset) → review+
Thanks Patrick!

>> +    // TODO Bug 1100001
>> +    /*yield undoChange(inspector);
>> +    ok(getNode(outerHTMLSelector), "The original node has been restored " +
>> +      "after undo.");*/
> 
> nit: remove these 4 commented lines. The bug is created anyway.

Oh, I thought it would have been useful to keep it commented so when bug 1100001 is fixed, we can uncomment and have the tests immediately. As you want.

> nit: you can remove the 2nd parameter here.

I like making explicit this kind of parameter (but as you want too).

> So, I know this file doesn't have any localization comments yet, but it should, this helps i10n people localize the software easier.

Agree! (didn't know that it existed)

Florent
Posted patch 1095521.patchSplinter Review
How about now?

For the localization comments, I tried to popularize things, I hope my explanations are clear and correct.

Florent
Attachment #8524892 - Attachment is obsolete: true
Attachment #8527208 - Flags: review?(pbrosset)
Comment on attachment 8527208 [details] [diff] [review]
1095521.patch

Review of attachment 8527208 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect! Good job on the patch Florent. Thanks for helping close this firebug gap.
Attachment #8527208 - Flags: review?(pbrosset) → review+
Last try was green: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c7dca5d19192
Changes in last patch are only nits.
Asking for checkin in fx-team.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d3032b990c98
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
As usual, you shouldn't use "..." but "…". Also, the space before makes no sense

<!ENTITY inspectorHTMLPasteExtraSubmenu.label      "Paste ...">
<!ENTITY inspectorHTMLPasteExtraSubmenu.accesskey  "T">

And since the available character is "t" in the label, I would use lowercase (not that it fails, it's just cleaner).
(In reply to Francesco Lodolo [:flod] from comment #22)
> As usual, you shouldn't use "..." but "…". Also, the space before makes no
> sense
> 
> <!ENTITY inspectorHTMLPasteExtraSubmenu.label      "Paste ...">
> <!ENTITY inspectorHTMLPasteExtraSubmenu.accesskey  "T">
> 
> And since the available character is "t" in the label, I would use lowercase
> (not that it fails, it's just cleaner).
Sorry, I didn't know about this. Filed follow-up bug 1104094 to correct this.
Florent: do you think you could also take care of this simple bug?
Flags: needinfo?(fayolle-florent)
> Florent: do you think you could also take care of this simple bug?

Sure.

Florent
Flags: needinfo?(fayolle-florent)
Will, this bug added some new "paste" commands in the context menu of the markup view in 36.  Could you please add this information to MDN?
Flags: needinfo?(wbamberg)
Keywords: dev-doc-needed
bgrins: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector#Element_popup_menu_2, is that OK?
Flags: needinfo?(wbamberg) → needinfo?(bgrinstead)
(In reply to Will Bamberg [:wbamberg] from comment #26)
> bgrins:
> https://developer.mozilla.org/en-US/docs/Tools/
> Page_Inspector#Element_popup_menu_2, is that OK?

Yup, looks good - thanks!
Flags: needinfo?(bgrinstead)
Release Note Request (optional, but appreciated)
[Why is this notable]: New feature in inspector
[Suggested wording]: Inspector : More paste options in markup view
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector#Element_popup_menu_2
relnote-firefox: --- → ?
Added to the release notes with "Inspector : More paste options in markup view" as wording.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.