Closed Bug 1331802 Opened 3 years ago Closed 3 years ago

Fix remaining ESLint issues in devtools/server/actors/

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: ntim, Assigned: fabien)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 2 obsolete files)

There are 5 files left to clean up:
devtools/server/actors/webconsole.js
devtools/server/actors/object.js
devtools/server/actors/script.js
devtools/server/actors/styleeditor.js
devtools/server/actors/stylesheets.js
Hi , I am newbie can anyone help me out to take this bug and how to fix and test it later .
Flags: needinfo?(ntim.bugs)
(In reply to Naveen from comment #1)
> Hi , I am newbie can anyone help me out to take this bug and how to fix and
> test it later .

First, you'll need to set up your development environment: https://wiki.mozilla.org/DevTools/Hacking

Then you'll need to run:

./mach eslint devtools/server/actors/ --no-ignore

and fix the errors shown in the console.

The last step is to attach a diff of your changes here.


Let me know if you need more info.
Flags: needinfo?(ntim.bugs)
Thanks for the input i will try the above steps and get back to you if I stuck in the middle :)
Hi  Tim Nguyen :
I tried to set up environment and run the command which you gave above but came across an error while runnig the above-given commands any help would be appreciated. below are the error prints 

----------------------
$ ./mach eslint devtools/server/actors/ --no-ignore --fix
An error occurred running eslint. Please check the following error messages:

'node' is not recognized as an internal or external command,
operable program or batch file.
A failure occured in the eslint linter.
? 1 problem (0 errors, 0 warnings, 1 failure)
(In reply to Naveen from comment #4)
> Hi  Tim Nguyen :
> I tried to set up environment and run the command which you gave above but
> came across an error while runnig the above-given commands any help would be
> appreciated. below are the error prints 
> 
> ----------------------
> $ ./mach eslint devtools/server/actors/ --no-ignore --fix
> An error occurred running eslint. Please check the following error messages:
> 
> 'node' is not recognized as an internal or external command,
> operable program or batch file.
> A failure occured in the eslint linter.
> ? 1 problem (0 errors, 0 warnings, 1 failure)

Can you try ./mach eslint --setup ?

If that doesn't work, try installing node manually from https://nodejs.org/en/
Finally got the issue .. node path was not added in m PATH variable even though it is installed in my system ..
Fixed the issue by adding node path to PATH = $PATH:~/path/to/node.exe

Now i have got errors printed out in the shell which counts around 551 errors and 169 warnings so now i have fix them all am i right ..
(In reply to Tim Nguyen :ntim from comment #2)
> (In reply to Naveen from comment #1)
> > Hi , I am newbie can anyone help me out to take this bug and how to fix and
> > test it later .
> 
> First, you'll need to set up your development environment:
> https://wiki.mozilla.org/DevTools/Hacking
> 
> Then you'll need to run:
> 
> ./mach eslint devtools/server/actors/ --no-ignore
> 
> and fix the errors shown in the console.
> 
> The last step is to attach a diff of your changes here.
> 
> 
> Let me know if you need more info.

I am able to fix errors and warning as of now i am stuck in below 3 erros which i am not able to understand any help ?

c:\mozilla-source\mozilla-central\devtools\server\actors\object.js
  51:3  error  'createValueGrip' is already declared in the upper scope.  no-shadow (eslint) //tried understanding this but since only one declaration        is present in the whole file didn't get what to do
c:\mozilla-source\mozilla-central\devtools\server\actors\script.js
  774:52  error  'createValueGrip' is already declared in the upper scope.  no-shadow (eslint) //same as above
c:\mozilla-source\mozilla-central\devtools\server\actors\webconsole.js
  1253:21  error  Function 'EvalWithDebugger' has a complexity of 43.  complexity (eslint) // we need to move few if-else statements to new function not sure how to do this .

? 3 problems (3 errors, 0 warnings)
(In reply to Naveen from comment #7)
> (In reply to Tim Nguyen :ntim from comment #2)
> > (In reply to Naveen from comment #1)
> > > Hi , I am newbie can anyone help me out to take this bug and how to fix and
> > > test it later .
> > 
> > First, you'll need to set up your development environment:
> > https://wiki.mozilla.org/DevTools/Hacking
> > 
> > Then you'll need to run:
> > 
> > ./mach eslint devtools/server/actors/ --no-ignore
> > 
> > and fix the errors shown in the console.
> > 
> > The last step is to attach a diff of your changes here.
> > 
> > 
> > Let me know if you need more info.
> 
> I am able to fix errors and warning as of now i am stuck in below 3 erros
> which i am not able to understand any help ?
> 
> c:\mozilla-source\mozilla-central\devtools\server\actors\object.js
>   51:3  error  'createValueGrip' is already declared in the upper scope. 
> no-shadow (eslint) //tried understanding this but since only one declaration
> is present in the whole file didn't get what to do
That just means you need to rename createValueGrip to a different name in that function.

> c:\mozilla-source\mozilla-central\devtools\server\actors\script.js
>   774:52  error  'createValueGrip' is already declared in the upper scope. 
> no-shadow (eslint) //same as above
Same here.

> c:\mozilla-source\mozilla-central\devtools\server\actors\webconsole.js
>   1253:21  error  Function 'EvalWithDebugger' has a complexity of 43. 
> complexity (eslint) // we need to move few if-else statements to new
> function not sure how to do this .
Feel free to wrap the function with: /* eslint-disable complexity */ then /* eslint-enable complexity */ for now.
Assignee: nobody → naveen.maltesh
(In reply to Tim Nguyen :ntim from comment #8)
> (In reply to Naveen from comment #7)
> > (In reply to Tim Nguyen :ntim from comment #2)
> > > (In reply to Naveen from comment #1)
> > > > Hi , I am newbie can anyone help me out to take this bug and how to fix and
> > > > test it later .
> > > 
> > > First, you'll need to set up your development environment:
> > > https://wiki.mozilla.org/DevTools/Hacking
> > > 
> > > Then you'll need to run:
> > > 
> > > ./mach eslint devtools/server/actors/ --no-ignore
> > > 
> > > and fix the errors shown in the console.
> > > 
> > > The last step is to attach a diff of your changes here.
> > > 
> > > 
> > > Let me know if you need more info.
> > 
> > I am able to fix errors and warning as of now i am stuck in below 3 erros
> > which i am not able to understand any help ?
> > 
> > c:\mozilla-source\mozilla-central\devtools\server\actors\object.js
> >   51:3  error  'createValueGrip' is already declared in the upper scope. 
> > no-shadow (eslint) //tried understanding this but since only one declaration
> > is present in the whole file didn't get what to do
> That just means you need to rename createValueGrip to a different name in
> that function.
> 
> > c:\mozilla-source\mozilla-central\devtools\server\actors\script.js
> >   774:52  error  'createValueGrip' is already declared in the upper scope. 
> > no-shadow (eslint) //same as above
> Same here.
> 
> > c:\mozilla-source\mozilla-central\devtools\server\actors\webconsole.js
> >   1253:21  error  Function 'EvalWithDebugger' has a complexity of 43. 
> > complexity (eslint) // we need to move few if-else statements to new
> > function not sure how to do this .
> Feel free to wrap the function with: /* eslint-disable complexity */ then /*
> eslint-enable complexity */ for now.

Yes thanks for the suggestion all errors are fixed now , how to add diff here ? i did hg status and all the above files are shown as modified and hg  add *, next i executed hg commit -m "message" , now need to now how to add diff to show the cahnges i made ..
Use `hg bzexport` or `hg export . > my-change.patch` to export a diff.
Attachment #8844433 - Attachment description: fixes for → Bug 1331802 - Fix remaining ESLint issues in devtools/server/actors/
Attachment #8844433 - Flags: review+
Hi Tim Nguyen, Attached Patch link above please review .
Comment on attachment 8844433 [details] [diff] [review]
Bug 1331802 - Fix remaining ESLint issues in devtools/server/actors/

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

There seems to be some unrelated files in this patch: configure and old-configure.

Also, can you remove devtools/server/actors/ from .eslintignore ?
Attachment #8844433 - Flags: review+ → review?(ttromey)
Attached patch adding .eslintignore changes for (obsolete) — Splinter Review
Attachment #8844433 - Attachment is obsolete: true
Attachment #8844433 - Flags: review?(ttromey)
Your last patch should be combined with the previous one.
Attachment #8844433 - Attachment is obsolete: false
Attachment #8844433 - Flags: review?(ttromey)
(In reply to Tim Nguyen :ntim from comment #15)
> Your last patch should be combined with the previous one.

Sorry i didn't get how to do this. And is there any way to remove unrelated patches like configure and old-configure
(In reply to Naveen from comment #16)
> (In reply to Tim Nguyen :ntim from comment #15)
> > Your last patch should be combined with the previous one.
> 
> Sorry i didn't get how to do this. And is there any way to remove unrelated
> patches like configure and old-configure

I'm not a mercurial user by any stretch, but you should be able to use
the rebase extension to squash patches together.  To remove a file that's
been erroneously added to a patch, you can just remove the file, commit the
removal, and then use rebase to squash the removal into the patch that
added the file.
Comment on attachment 8844433 [details] [diff] [review]
Bug 1331802 - Fix remaining ESLint issues in devtools/server/actors/

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

Thank you for the patch.

I am going to r- this, because it needs many changes, and I want to emphasize the need for a re-review.

As mentioned in an earlier review, the configure and old-configure files also have to be removed beforehand.

::: devtools/server/actors/object.js
@@ +31,5 @@
>   * @param hooks Object
>   *        A collection of abstract methods that are implemented by the caller.
>   *        ObjectActor requires the following functions to be implemented by
>   *        the caller:
> + *          - createValueGripforObj

It's fine to rename this here but I think most of the other renames are unnecessary, and even a bit harmful in that they make the code less clear.

@@ +46,5 @@
>   *              Decrement the actor's grip depth
>   *          - globalDebugObject
>   *              The Debuggee Global Object as given by the ThreadActor
>   */
>  function ObjectActor(obj, {

This argument is completely removed and yet it's referred to by all the "this.hooks" changes.  I think this probably won't pass testing?

This is the spot where I'd do the renaming, just with "createValueGrip: createValueGripForObj", and then change the next spot (which I'll mark).

@@ +57,5 @@
>  }) {
>    assert(!obj.optimizedOut,
>           "Should not create object actors for optimized out values!");
>    this.obj = obj;
>    this.hooks = {

... this spot here.

@@ +89,3 @@
>        g.class = "Proxy";
> +      g.proxyTarget = this.hooks.createValueGripforObj(this.obj.proxyTarget);
> +      g.proxyHandler = this.hooks.createValueGripforObj(this.obj.proxyHandler);

Then all these changes would not be needed.

@@ +1866,5 @@
>      return true;
>    },
>  
>    function Object(objectActor, grip, rawObj) {
> +    /* specialStringBehavior = false*/

Probably better to just remove this comment.

@@ +2150,5 @@
>  /**
>   * Create a grip for the given debuggee value.  If the value is an
>   * object, will create an actor with the given lifetime.
>   */
> +function createValueGripforObj(value, pool, makeObjectGrip) {

Renaming this seems incorrect.

::: devtools/server/actors/script.js
@@ +70,5 @@
>      if (this.size === 0) {
>        return;
>      }
>  
> +    function* findKeys(Object, key) {

This rename is puzzling because...

@@ +77,4 @@
>            yield key;
>          }
> +      } else {
> +        for (let keys of Object.keys(Object)) {

... doesn't it change which |keys| method might be called?

@@ +214,5 @@
>  SourceActorStore.prototype = {
>    /**
>     * Lookup an existing actor id that represents this source, if available.
>     */
> +  getReusableActorId: function (Source, OriginalUrl) {

Normally we don't use upper-case names here.  Just use "source" and "originalUrl".  This appears in multiple spots.

::: devtools/server/actors/styleeditor.js
@@ +4,5 @@
>  
>  "use strict";
>  
>  const {Cc, Ci} = require("chrome");
> +// const Services = require("Services");

Don't comment out code, just delete it.

@@ +17,5 @@
>  loader.lazyGetter(this, "CssLogic", () => require("devtools/shared/inspector/css-logic"));
>  
>  var TRANSITION_CLASS = "moz-styleeditor-transitioning";
>  var TRANSITION_DURATION_MS = 500;
> +var TRANSITION_RULE = ":root.moz-styleeditor-transitioning,\n" +

In this particular case using a `...` string would be clearer.

@@ +503,5 @@
>  exports.StyleEditorActor = StyleEditorActor;
>  
>  /**
>   * Normalize multiple relative paths towards the base paths on the right.
> +

It's fine to just delete all this if it isn't used.

::: devtools/server/actors/stylesheets.js
@@ +19,2 @@
>  const {SourceMapConsumer} = require("source-map");
> +/* const { installHelperSheet,

Just delete.

@@ +37,5 @@
>    transition-timing-function: ease-out !important;
>    transition-property: all !important;
>  }`;
>  
> +// var LOAD_ERROR = "error-load";

Another deletion, several instances of this here and elsewhere -- I didn't note them all.

@@ +84,5 @@
>    _getText: function () {
>      if (this.text) {
>        return promise.resolve(this.text);
>      }
> +    let Content = this.sourceMap.sourceContentFor(this.url);

This seems strange; it's better to use lowercase names, so if there is a name clash (I'm just guessing), it's better to just pick a different name entirely.

@@ +126,5 @@
>    get matches() {
>      return this.mql ? this.mql.matches : null;
>    },
>  
> +  initialize: function (MediaRule, SourceMap) {

Lowercase.

::: devtools/server/actors/webconsole.js
@@ +14,4 @@
>  const DevToolsUtils = require("devtools/shared/DevToolsUtils");
>  const ErrorDocs = require("devtools/server/actors/errordocs");
> +// Fix for error XPCNativeWrapper not defined
> +const XPCNativeWrapper = require("toolkit/content/XPCNativeWrapper");

Rather than this, I think there should just be a global declaration, as is done in other actors, e.g.:

https://dxr.mozilla.org/mozilla-central/rev/58753259bfeb3b818eac7870871b0aae1f8de64a/devtools/server/actors/tab.js#9

@@ +52,3 @@
>   *        Optional, the parent actor.
>   */
> +function WebConsoleActor(Connection, ParentActor) {

Lower-case names, here and elsewhere.  We actually use camel case names starting with a lower-case letter, so "parentActor", not "ParentActor" or "parentactor".

@@ +177,5 @@
>     * @private
>     * @return nsIDOMWindow
>     *         The window to use, or null if no window could be found.
>     */
> +  _getWindowForBrowserConsole: function GetWindowForBrowserConsole() {

Rather than rename this by removing the "WCA_", you can simply remove the name entirely.  These names aren't needed any more and we intended to remove them all over time anyway.

@@ +927,5 @@
>          try {
>            errorDocURL = ErrorDocs.GetURL(error);
> +        } catch (ex) {
> +          let msg = "caught exception while GetURL";
> +          DevToolsUtils.reportException(msg, ex);

I think it's probably clearer to just inline the string constant, rather than introduce a new variable.

@@ +1024,2 @@
>      // This is the general case (non-paused debugger)
> +    } else {

The comment should come in the body of the "else", not before it.
Attachment #8844433 - Flags: review?(ttromey) → review-
Hi
I can take over this task.

How can I test my changes ? `./mach mochitest devtools/server/tests/mochitest/` ?
Flags: needinfo?(ntim.bugs)
(In reply to Fabien Casters from comment #19)
> Hi
> I can take over this task.
Great, thanks!
> How can I test my changes ? `./mach mochitest
> devtools/server/tests/mochitest/` ?

A push to the try server is the best way to test your changes.

However, if you want a quick indication whether you haven't broken anything (without waiting for a try push to finish), I would suggest either:
- Running Firefox (`./mach run`) and checking if the devtools work
- Running `./mach test devtools/server/tests/`
Flags: needinfo?(ntim.bugs)
Attachment #8844433 - Attachment is obsolete: true
Attachment #8844745 - Attachment is obsolete: true
Assignee: naveen.maltesh → fabien
Comment on attachment 8864313 [details]
Bug 1331802 - Fix remaining ESLint issues in devtools/server/actors/

https://reviewboard.mozilla.org/r/135940/#review139238

Thank you for the patch.  This looks pretty good.  I did find one spot that I think is in error; I am going to r+ the patch on the condition that this is fixed.

::: devtools/server/actors/stylesheets.js:96
(Diff revision 1)
>        policy: Ci.nsIContentPolicy.TYPE_INTERNAL_STYLESHEET,
>        window: this.window
>      };
> -    return fetch(this.url, options).then(({content}) => {
> -      this.text = content;
> +    return fetch(this.url, options).then(({content: text}) => {
> +      this.text = text;
>        return content;

I think this should also "return text".
Attachment #8864313 - Flags: review?(ttromey) → review+
Comment on attachment 8864313 [details]
Bug 1331802 - Fix remaining ESLint issues in devtools/server/actors/

https://reviewboard.mozilla.org/r/135940/#review139238

> I think this should also "return text".

Oops. It's fixed.
Flags: needinfo?(ttromey)
It looks reasonable, so I'm sending it through try.
Flags: needinfo?(ttromey)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d716f0c8b0637da362a56843a3365f308bc786e0

There are some new devtools failures in there, so I guess something in the patch needs debugging.
Could you try running some of these failing tests locally and see what is going on?
I can provide some pointers if you need them.
Flags: needinfo?(fabien)
Those did seem to be intermittents, so I'm going to land this.
Thanks once again.
Flags: needinfo?(fabien)
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8458fcd8c078
Fix remaining ESLint issues in devtools/server/actors/ r=tromey
https://hg.mozilla.org/mozilla-central/rev/8458fcd8c078
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.