Closed Bug 1316723 Opened 8 years ago Closed 8 years ago

Fix more eslint errors in devtools/client/shared/

Categories

(DevTools :: Shared Components, defect)

Product:
DevTools
Component:
Shared Components
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

Details

Attachments

(1 file)

Bug 1316723 - Fix more eslint errors in devtools/client/shared/.
58 bytes, text/x-review-board-request
tromey
: review+
Details 
      No description provided.
Blocks: devtools-eslint
Be sure to note bug 1316700 and bug 1275637; the latter turns out to be more of a real
bug in webgl-utils.
(In reply to Tom Tromey :tromey from comment #1)
> Be sure to note bug 1316700 and bug 1275637; the latter turns out to be more
> of a real
> bug in webgl-utils.

I'm not touching any of those files in my clean up :)
Comment on attachment 8809742 [details]
Bug 1316723 - Fix more eslint errors in devtools/client/shared/.

https://reviewboard.mozilla.org/r/92270/#review92390

Thank you for doing this.  I especially like that you took the extra effort to clean up the tests.
I found a few issues - mostly nits, but at least one possible latent bug revealed by your patch.
Nothing too serious.

::: devtools/client/shared/redux/middleware/test/test_middleware-task-03.js:31
(Diff revision 4)
> +        "generator errors dispatch ERROR_TYPE actions with error");
>  });
>  
>  function generatorError() {
>    return function* (dispatch, getState) {
> -    throw "task-middleware-error-generator";
> +    let error = "task-middleware-error-generator";

I think it's better to use "throw new Error(...)", rather than work around the eslint warning like this.

::: devtools/client/shared/test/browser_flame-graph-04.js:70
(Diff revision 4)
>  }
>  
>  function getAverageCharWidth() {
>    let letterWidthsSum = 0;
> +
> +  /* eslint-disable no-inline-comments */

It's fine here to just move the comments before the line they are commenting.  Or they could be rewritten, like:

// space == 32
let start = 32;

Or even just remove them entirely like:

let start = " ".charCodeAt(0);

::: devtools/client/shared/test/browser_layoutHelpers.js:25
(Diff revision 4)
>    some.style.top = win.innerHeight + "px";
>    some.style.left = win.innerWidth + "px";
>    // The tests start with a black 2x2 pixels square below bottom right.
>    // Do not resize the window during the tests.
>  
> +  /* eslint-disable no-inline-comments */

I'd prefer if the comments were just moved.

::: devtools/client/shared/test/test-actor.js:875
(Diff revision 4)
>    /**
>     * Get the current rect of the border region of the box-model highlighter
>     */
>    getSimpleBorderRect: Task.async(function* (toolbox) {
>      let {border} = yield this._getBoxModelStatus(toolbox);
> -    let {p1, p2, p3, p4} = border.points;
> +    let {p1, p2, p4} = border.points;

This seems fishy.  I think either the computation of width or height, immediately below, must be incorrect.

::: devtools/client/shared/test/unit/test_parseDeclarations.js:241
(Diff revision 4)
>        priority: "",
>        offsets: [0, 47]
>      }]
>    },
>    {
> -    input: "content: \"a not s\\\
> +    input: "content: \"a not s\\          o very long title\"",

I wonder whether the original here was an attempt to embed a newline?  I probably wrote this test but I don't recall.
Attachment #8809742 - Flags: review?(ttromey) → review+
Comment on attachment 8809742 [details]
Bug 1316723 - Fix more eslint errors in devtools/client/shared/.

https://reviewboard.mozilla.org/r/92270/#review92390

> I think it's better to use "throw new Error(...)", rather than work around the eslint warning like this.

The test expects a string, to I left it as it is.

> It's fine here to just move the comments before the line they are commenting.  Or they could be rewritten, like:
> 
> // space == 32
> let start = 32;
> 
> Or even just remove them entirely like:
> 
> let start = " ".charCodeAt(0);

Done

> I'd prefer if the comments were just moved.

Done

> This seems fishy.  I think either the computation of width or height, immediately below, must be incorrect.

I've checked the calculations and they look sensible to me, if the points are laid up like this:

p1 +--------+ p2
   |        |
   |        |
p4 +--------+ p3

or like this:

p1 +--------+ p2
   |        |
   |        |
p3 +--------+ p4


You could replace p4 by p3 for the height calculation, but it currently works fine the way it is, so why change it?

> I wonder whether the original here was an attempt to embed a newline?  I probably wrote this test but I don't recall.

I tried to compute this string with the JS console, and it seems there's no newline.
Plus, the test passes.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03c833666294
(I've fixed the this.toolbox is undefined error)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/516fd53f0f84
Fix more eslint errors in devtools/client/shared/. r=tromey
Assignee: nobody → ntim.bugs
https://hg.mozilla.org/mozilla-central/rev/516fd53f0f84
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: