1036297 - markupview updateNodeOuterHTML method jsdoc is incorrect and should return a promise

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

In markup-view.js:

  /**
   * Retrieve the index of a child within its parent's children collection.
   * @param aNode The NodeFront to find the index of.
   * @param newValue The new outerHTML to set on the node.
   * @param oldValue The old outerHTML that will be reverted to find the index of.
   * @returns A promise that will be resolved with the integer index.
   *          If the child cannot be found, returns -1
   */
  updateNodeOuterHTML: function(aNode, newValue, oldValue) {
    ...
  },

- The description of what the function does is incorrect
- The description of the aNode parameter is incorrect
- The description of the oldValue parameter is incorrect
- The function doesn't return a promise, nor does it return -1

The whole jsdoc comment block should be updated.

Comment 1

[Patrick Brosset <:pbro>]

Furthermore, this function would be a lot more useful if it *did* return a promise.
That promise should resolve/reject when the walker's setOuterHTML does resolve/reject.
Also, if the container isn't found, it should reject.

Comment 2

[Ram Dayal Vaishnav [:ram]]

I would like to work on this bug. Can I get it assigned?

Comment 3

[Ram Dayal Vaishnav [:ram]]

Hey Patrick, Thanks for assigning the bug to me. 

Can you help me in getting functionality of this function ? And what is this function supposed to return ?

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Ram Dayal Vaishnav [:Ram] from comment #3)
> Hey Patrick, Thanks for assigning the bug to me. 
> 
> Can you help me in getting functionality of this function ? And what is this
> function supposed to return ?

This function is an asynchronous function used to programmatically replace the outerHTML of any node displayed in the inspector with some other HTML code.
It is called at least from `beginEditingOuterHTML` (just below in the code).

The first comment line for this function is obviously wrong and seems to have been copied from the getNodeChildIndex function.

The goal of this bug is mostly just to change the jsdoc function comment to something more suitable.

Also, to make this function more useful, we should make it return a Promise object (see http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Promise.jsm) so that callers of this function can know when it is done processing the new outerHTML.

To do so, one way is to first create a promise object (somewhere at the beginning of the function):
let def = promise.defer(); 

Then at the end of the function, return it:
return def.promise;

And when the outerHTML has finished being updated, resolve the promise:
this.walker.setOuterHTML(aNode, newValue).then(def.resolve);

Comment 5

[Alexey Novak]

Attachment: 1036297.patch

Hi Patrick,

I saw this bug and it looked like the bug was not looked > 1 month already.
Hope it is not too rude. I created a patch according to your feedback. Does it look OK?

Comment 6

[Ram Dayal Vaishnav [:ram]]

Hi Alexey,

Thank you for the patch. I was quick busy with some community building and other activities so could not take it up. This weekend Mozilla India is throwing Developers Sprint, so I was planning to take it during that event. Anyways, you truly deserve this bug. I hope it get reviewed positively.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8476394 [details] [diff] [review]
1036297.patch

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

Thanks for the patch. Almost there, just a few comments below.
We'll need to run a try build with the new patch too. Do you have try push access?

::: browser/devtools/markupview/markup-view.js
@@ +762,2 @@
>     * @param newValue The new outerHTML to set on the node.
> +   * @param oldValue The old outerHTML that will be reverted to in case of error.

The oldValue isn't passed for error cases, it is used to undo the change. Should be rephrased to something like:

@param oldValue The old outerHTML that will be used if the user undos the update.

@@ +764,2 @@
>     * @returns A promise that will be resolved with the integer index.
>     *          If the child cannot be found, returns -1

This comment line is wrong. The promise does not resolve to the index. Should be rephrased as something like:

@returns A promise that will resolve when the outer HTML has been updated

@@ +766,5 @@
>     */
>    updateNodeOuterHTML: function(aNode, newValue, oldValue) {
>      let container = this._containers.get(aNode);
>      if (!container) {
> +      return -1;

In case a method returns a promise, it's better if it *always* returns a promise, this makes it a lot easier for consumers of this method to handle its return value.
In this case where the container isn't found, you shouldn't return -1, but instead a rejected promise:

if (!container) {
  return promise.reject();
}

This way, consumers of this method can do:

this.updateNodeOuterHTML(node, new, old).then(() => {
  /* if the html was updated */
}, () => {
  /* if the container wasn't found */
});

@@ +776,5 @@
>        this._outerHTMLChildIndex = i;
>        this._outerHTMLNode = aNode;
>  
>        container.undo.do(() => {
> +        this.walker.setOuterHTML(aNode, newValue).then(def.resolve);

this.walker.setOuterHTML(...) returns a promise, but there are no guarantees that it will succeed everytime, in case it fails, this method needs to reject the promise:

this.walker.setOuterHTML(aNode, newValue).then(def.resolve, def.reject);

Same on the line below.

Comment 8

[Alexey Novak]

Hi Ram,
Feel slightly bad for taking this bug from you. I genuinely thought it is one of those forgotten by everyone bugs. Thanks for letting me get on with it.


Thanks Patrick for your feedback! I tried to change comments as much as I could from my understanding of the function and your comments in this bug. But your description makes function much more obvious.

No I do not have try push access. Is it something you can change in my profile? Are there any documentation I can read on how to utilize this access?

I will address the changes ^ shortly

Comment 9

[Alexey Novak]

Attachment: 1036297.2.patch

Hey Patrick,
I made changes according to your comments. Your comments were amazing since you pretty much pointed out all the changes I required.
Let me know if it looks OK or I need to modify anything else.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8476777 [details] [diff] [review]
1036297.2.patch

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

Code changes look good! Thanks for the patch.
Did you build and test locally that editing HTML still worked fine in the markup-view?
Did you run the existing mochitests to ensure there are no regressions? ./mach mochitest-devtools browser/devtools/markupview
Also, we need to run this patch on try, to make sure your changes don't break on other platforms. Have you got push access to Try? (I can do it for you if not).

Comment 11

[Alexey Novak]

Hi Patrick,

It took me a while to rebuild the latest fx-team with my changes but finally done. I was able to modify HTML in markup-view without any problem.

Tests passed fine as well:
3079 INFO Browser Chrome Test Summary
3080 INFO Passed:  810
3081 INFO Failed:  0
3082 INFO Todo:    0

To be honest those tests looked REALLY cool.

As I mentioned before - I do not think that can run my patch on try. Do not think I have an access. If you can give it to me and explain how to use it that would be great. If not then I need your help to spawn one.

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Alexey Novak from comment #11)
> Hi Patrick,
> 
> It took me a while to rebuild the latest fx-team with my changes but finally
> done. I was able to modify HTML in markup-view without any problem.
> 
> Tests passed fine as well:
> 3079 INFO Browser Chrome Test Summary
> 3080 INFO Passed:  810
> 3081 INFO Failed:  0
> 3082 INFO Todo:    0
> 
> To be honest those tests looked REALLY cool.
> 
> As I mentioned before - I do not think that can run my patch on try. Do not
> think I have an access. If you can give it to me and explain how to use it
> that would be great. If not then I need your help to spawn one.
Thanks. I'll run that patch on Try for you.
See https://www.mozilla.org/hacking/commit-access-policy/ and https://www.mozilla.org/hacking/committer/ for information about mozilla's access policy.

Comment 13

[Patrick Brosset <:pbro>]

Pending try build: https://tbpl.mozilla.org/?tree=Try&rev=314160724a92

Comment 14

[Patrick Brosset <:pbro>]

Oh, one thing, can you attach a new patch with the commit message being:
> Bug 1036297 - markupview updateNodeOuterHTML method jsdoc is incorrect and should return a promise, r=pbrosset
instead of:
> Bug 1036297 - markupview updateNodeOuterHTML method jsdoc is incorrect and should return a promise, r=patrick
Thanks!

Comment 15

[Patrick Brosset <:pbro>]

Attachment: 1036297.2.patch

Changed the commit message to have the correct reviewer nick.
And pushed to fx-team as try build is green:

https://hg.mozilla.org/integration/fx-team/rev/958e5cfaebb2

Comment 16

[Alexey Novak]

Thanks for changing commit message for me Patrick.

Can I change the status of the bug to be RESOLVED? :)

Comment 17

[Patrick Brosset <:pbro>]

(In reply to Alexey Novak from comment #16)
> Thanks for changing commit message for me Patrick.
> 
> Can I change the status of the bug to be RESOLVED? :)
Not yet. This will be done by the person who will merge that patch into mozilla-central.

Comment 18

[Alexey Novak]

Ok got it. Will know for future.

Thanks for helping out me!
If there are any other good bugs to take care of - I can give it a shot.

Comment 19

[Patrick Brosset <:pbro>]

(In reply to Alexey Novak from comment #18)
> If there are any other good bugs to take care of - I can give it a shot.
That'd be great! There's a few resources linked from the DevTools wiki page: https://wiki.mozilla.org/DevTools/GetInvolved#Mentored_and_Good_First_Bugs
Also, here's a bugzilla search that will show all mentored devtools bugs: https://bugzilla.mozilla.org/buglist.cgi?f1=bug_mentor&list_id=11059732&o1=isnotempty&resolution=---&query_format=advanced&bug_status=NEW&component=Developer%20Tools&component=Developer%20Tools%3A%203D%20View&component=Developer%20Tools%3A%20Canvas%20Debugger&component=Developer%20Tools%3A%20Console&component=Developer%20Tools%3A%20Debugger&component=Developer%20Tools%3A%20Framework&component=Developer%20Tools%3A%20Graphic%20Commandline%20and%20Toolbar&component=Developer%20Tools%3A%20Inspector&component=Developer%20Tools%3A%20Memory&component=Developer%20Tools%3A%20Netmonitor&component=Developer%20Tools%3A%20Object%20Inspector&component=Developer%20Tools%3A%20Profiler&component=Developer%20Tools%3A%20Responsive%20Mode&component=Developer%20Tools%3A%20Scratchpad&component=Developer%20Tools%3A%20Source%20Editor&component=Developer%20Tools%3A%20Storage%20Inspector&component=Developer%20Tools%3A%20Style%20Editor&component=Developer%20Tools%3A%20Timeline&component=Developer%20Tools%3A%20User%20Stories&component=Developer%20Tools%3A%20Web%20Audio%20Editor&component=Developer%20Tools%3A%20WebGL%20Shader%20Editor&component=Developer%20Tools%3A%20WebIDE&product=Firefox

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/958e5cfaebb2