Closed Bug 1270994 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Framework, enhancement)

Product:
DevTools
Component:
Framework
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: ntim, Assigned: tromey)

References

Details

Attachments

(1 file, 3 obsolete files)

MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/*.js; r=jryans
58 bytes, text/x-review-board-request
jryans
: review+
Details 
      No description provided.
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/
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).
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.
Assignee: nobody → ntim.bugs
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 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 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".
Attachment #8749986 - Flags: review?(ttromey)
Attachment #8749990 - Flags: review?(ttromey) → review+
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.
Severity: normal → enhancement
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
Attachment #8749990 - Attachment is obsolete: true
Assignee: ntim.bugs → ttromey
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.
Attachment #8749986 - Attachment is obsolete: true
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/
Attachment #8754421 - Attachment description: MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/*.js; r?jryans → MozReview Request: Bug 1270994 - Fix ESLint errors in devtools/client/shared/*.js; r=jryans
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/
The previous patch had a bad base revision, so rebased and new try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e51df939c77
Keywords: checkin-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
Blocks: 1275637
Sorry about the mess.
I've filed a followup to fix it.
Flags: needinfo?(ttromey)
MozReview-Commit-ID: 1T2vOPMAj8G
Comment on attachment 8756493 [details] [diff] [review]
fix eslint errors in webgl-utils.js; r?jryans

Wrong bug.
Attachment #8756493 - Attachment is obsolete: true
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: