1270994 - Fix ESLint errors in devtools/client/shared/*.js

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement)

Comment 1

[Tim Nguyen :ntim]

Attachment: MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/*.js. r=tromey

Review commit: https://reviewboard.mozilla.org/r/51271/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51271/

Comment 2

[Tim Nguyen :ntim]

Comment on attachment 8749986 [details]
MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/*.js. r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51271/diff/1-2/

Comment 3

[Tim Nguyen :ntim]

This patch fixes the errors for:
- all top-level devtools/client/shared js files (I've excluded JSM files)
- all devtools/client/shared/widgets js files (I've also excluded JSM files)

I've excluded:
- demangle.js because it's a lib
- developer-toolbar.js because it would require some significant changes (there are too much nested callback in some places).

Comment 4

[Tim Nguyen :ntim]

Attachment: MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/(components|redux). r=tromey

Review commit: https://reviewboard.mozilla.org/r/51273/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51273/

Comment 5

[Tim Nguyen :ntim]

The 2nd patch fixes:
- Errors in devtools/client/shared/components/
- Errors in devtools/client/shared/redux/

It excludes:
- devtools/client/shared/redux/middleware/test/ (They have too much errors, I don't want to fix them)

In both patches, you may notice I've disabled some rules, this is either because they don't work properly (react/sort-comp for example), either because I thought it would be messier to fix the error.

You may also see some changes on some of the excluded paths, this is because I've ran ./mach eslint --fix.

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8749990 [details]
MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/(components|redux). r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51273/diff/1-2/

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8749990 [details]
MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/(components|redux). r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51273/diff/2-3/

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8749986 [details]
MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/*.js. r=tromey

https://reviewboard.mozilla.org/r/51271/#review48099

Thank you for the patch.  Overall it looks good, but I had a few nits; and review found one latent bug.

::: devtools/client/shared/doorhanger.js:72
(Diff revision 2)
>   * @param {String} type
> - *        The type of doorhanger to be displayed is, using the `TYPES` definition.
> + *        The type of doorhanger to be displayed is, using the `TYPES`
> + *        definition.
>   * @param {String} selector
> - *        The selector that the doorhanger should be appended to within `window`.
> - *        Defaults to a XUL Document's `window` element.
> + *        The selector that the doorhanger should be appended to within
> + *        `window`.Defaults to a XUL Document's `window` element.

I think there should be a space after the period.

::: devtools/client/shared/source-utils.js:77
(Diff revision 2)
> -    let hostname = isChrome ? null : url.hostname;
> -    let host = isChrome ? null :
> -               url.port ? `${url.host}:${url.port}` :
> -               url.host;
> +    let hostname, host;
> +    if (isChrome) {
> +      hostname = null;
> +      host = null;
> +    } else {
> +      ({hostname} = url);

I think "hostname = url.hostname" would be clearer here

::: devtools/client/shared/undo.js:33
(Diff revision 2)
>    _index: 0,
>  
>    // The current batch depth (see startBatch() for details)
>    _batchDepth: 0,
>  
> -  destroy: function Undo_destroy()
> +  destroy: function undoDestroy() {

Rather than renaming the function, I think removing the name is preferred.  See bug 1098222.  There are a few instances of this in this file.

::: devtools/client/shared/widgets/CubicBezierWidget.js:777
(Diff revision 2)
>        this.dot.style.animationTimingFunction = value;
>        this.restartAnimation();
>      }
>  
>      this.previousValue = value;
> +    return null;

Do any callers use the return value?  If not you might as well instead remove the "false" from the earlier return, and leave out this addition.

::: devtools/client/shared/widgets/FlameGraph.js:462
(Diff revision 2)
>     * Redraws the widget when necessary. The actual graph is not refreshed
>     * every time this function is called, only the cliphead, selection etc.
>     */
> -  _drawWidget: function() {
> +  _drawWidget: function () {
>      if (!this._shouldRedraw) {
> -      return;
> +      return null;

I think it would be clearer not to add this return, and instead to split up the "return void ..." later in the function.

::: devtools/client/shared/widgets/FlameGraph.js:678
(Diff revision 2)
>  
>    /**
>     * Fills all block inside this graph's pyramid.
>     * @see FlameGraph.prototype._drawPyramid
>     */
> -  _drawPyramidFill: function(dataSource, verticalOffset, dataOffset, dataScale) {
> +  _drawPyramidFill(dataSource, verticalOffset, dataOffset, dataScale) {

I think we standardized on "_mumble: function (", n ot "_mumble(".  So this and some other spots in this file need a small change.

::: devtools/client/shared/widgets/Graphs.js:388
(Diff revision 2)
>     *        The selection's { start, end } values.
>     */
> -  setSelection: function(selection) {
> +  setSelection: function (selection) {
>      if (!selection || selection.start == null || selection.end == null) {
> -      throw "Invalid selection coordinates";
> +      let e = "Invalid selection coordinates";
> +      throw e;

I had been assuming that these throw refactorings were to avoid line-length limits.  But this one makes me think that, instead, they are to avoid an eslint rule about throwing a constant.

I think they should all just be changed to throw new Error("...").

::: devtools/client/shared/widgets/Graphs.js:865
(Diff revision 2)
>     * Checks whether the start handle of the selection is hovered.
>     * @return boolean
>     */
> -  _isHoveringStartBoundary: function() {
> +  _isHoveringStartBoundary: function () {
>      if (!this.hasSelection() || !this.hasCursor()) {
> -      return;
> +      return null;

I suspect these functions (I think there are 3 here) that are returning boolean should probably return false rather than null.

::: devtools/client/shared/widgets/Graphs.js:960
(Diff revision 2)
>      // Don't allow the event coordinates to be bigger than the canvas
>      // or less than 0.
>      let maxX = bb.p2.x - bb.p1.x;
>      let maxY = bb.p3.y - bb.p1.y;
>      let mouseX = Math.max(0, Math.min(x, maxX)) * this._pixelRatio;
>      let mouseY = Math.max(0, Math.min(x, maxY)) * this._pixelRatio;

I think removing "y" points out a latent bug: the computation of mouseY is using "x" where it should use "y".

Comment 9

[Tom Tromey :tromey]

Comment on attachment 8749990 [details]
MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/(components|redux). r=tromey

https://reviewboard.mozilla.org/r/51273/#review48171

Thank you.  Looks good.

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8749990 [details]
MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/(components|redux). r=tromey

I landed this patch in bug 1273653

Comment 11

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/*.js; r=jryans

Review commit: https://reviewboard.mozilla.org/r/53966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53966/

Comment 12

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3541516aaa27

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8754421 [details]
MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/*.js; r=jryans

https://reviewboard.mozilla.org/r/53966/#review50686

::: devtools/client/shared/frame-script-utils.js:31
(Diff revision 1)
>    data = data || {};
>    content.location.reload(data.forceget);
>  });
>  
> -addMessageListener("devtools:test:console", function ({ data: { method, args, id } }) {
> +addMessageListener("devtools:test:console", function ({
> +                                              data: { method, args, id }

That's a pretty awkward style for parameters...  Maybe it's better to just have the data argument, and the desstructure the rest on the next line:

addMessageListener(..., function ({ data }) {
  let { method, args, id } = data
  ...

::: devtools/client/shared/frame-script-utils.js:123
(Diff revision 1)
>    }
>  
>    sendAsyncMessage("devtools:test:xhr", responses);
>  }));
>  
> -addMessageListener("devtools:test:profiler", function ({ data: { method, args, id }}) {
> +addMessageListener("devtools:test:profiler", function ({

Another strange formatting case.

Comment 14

[Tom Tromey :tromey]

Comment on attachment 8754421 [details]
MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/*.js; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53966/diff/1-2/

Comment 15

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6f8abdfea66

Comment 16

[Tom Tromey :tromey]

Comment on attachment 8754421 [details]
MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/*.js; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53966/diff/2-3/

Comment 17

[Tom Tromey :tromey]

The previous patch had a bad base revision, so rebased and new try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e51df939c77

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6ac30d36c923

Comment 19

[Pulsebot]

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

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to revert a tiny bit of this to fix an intermittent devtools failure on OSX that started up with your push: https://treeherder.mozilla.org/#/jobs?repo=fx-team&fromchange=b7756d054ba86fd2a6a9bd851dbd58b0b4542b42&filter-searchStr=79eb85633760062e7661cf3ab2e2df555554a646&tochange=3aa793fa2d0b078e07bfe780f0a8859dedafc850

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And of course, that re-broke eslint since aFlags is not defined.
Re-ignoring that specific file in https://hg.mozilla.org/integration/fx-team/rev/2179cd63b7fc

Comment 22

[Pulsebot]

https://hg.mozilla.org/mozilla-central/rev/4e389186c5ee
https://hg.mozilla.org/mozilla-central/rev/46fe2115d46a

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6ac30d36c923
https://hg.mozilla.org/mozilla-central/rev/4e389186c5ee
https://hg.mozilla.org/mozilla-central/rev/46fe2115d46a

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca0d04ce0304
https://hg.mozilla.org/mozilla-central/rev/2179cd63b7fc

Comment 25

[Tom Tromey :tromey]

Sorry about the mess.
I've filed a followup to fix it.

Comment 26

[Tom Tromey :tromey]

Attachment: fix eslint errors in webgl-utils.js; r?jryans

MozReview-Commit-ID: 1T2vOPMAj8G

Comment 27

[Tom Tromey :tromey]

Comment on attachment 8756493 [details] [diff] [review]
fix eslint errors in webgl-utils.js; r?jryans

Wrong bug.