1255799 - make devtools/shared/webconsole eslint-clean

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Tom Tromey :tromey]

In bug 1244227, I made network-monitor.js eslint-clean.
It seemed best to me to split that patch out and try to clean
the entire directory, to avoid regressions here.

Comment 1

[Tom Tromey :tromey]

Attachment: make devtools/client/webconsole eslint-clean

MozReview-Commit-ID: AnaXFFHlNLM

Comment 2

[Tom Tromey :tromey]

Attachment: remove obsolete TODO from network-monitor.js

MozReview-Commit-ID: Lbx8C41oSqG

Comment 3

[Tom Tromey :tromey]

Comment on attachment 8729566 [details] [diff] [review]
remove obsolete TODO from network-monitor.js

This is trivia -- while doing the eslint cleanup, I noticed an obsolete comment.
I separated this out so I'd remember to point it out for review.

Comment 4

[Tim Nguyen :ntim]

Can you remove devtools/client/netmonitor from eslintignore while you're changing the file? Honza forgot to do it in bug 1252807

Comment 5

[Tom Tromey :tromey]

Attachment: make devtools/client/webconsole eslint-clean

Updated per request; and also for changes made in bug 1255827.

MozReview-Commit-ID: AnaXFFHlNLM

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8730221 [details] [diff] [review]
make devtools/client/webconsole eslint-clean

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

Looks good to me!

Only found couple of places where function argument is removed. Is there a reason for it?
Otherwise I think it should stay there to make it clear what's passed in.

Honza

::: devtools/shared/webconsole/network-monitor.js
@@ +282,5 @@
>    /**
>     * Handle progress event as data is transferred.  This is used to record the
>     * size on the wire, which may be compressed / encoded.
>     */
> +  onProgress: function(request, context, progress) {

Just curious, why did you remove the last 'progressMax' argument and kept the others?

@@ +1662,5 @@
>     * nsIWebProgressListener.onStateChange. If the state change tells that a file
>     * URI has been loaded, then the remote Web Console instance is notified.
>     * @private
>     */
> +  _checkFileActivity: function(progress, request, state) {

The same here, last argument removed.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=829506150b9b

Honza

Comment 8

[Tom Tromey :tromey]

(In reply to Jan Honza Odvarko [:Honza] from comment #6)

> Only found couple of places where function argument is removed. Is there a
> reason for it?

I think this part of the patch was written before we changed the eslint
config to allow unused arguments.  I'll add them back.

Comment 9

[Tom Tromey :tromey]

That try push certainly looks terrible.
I will investigate shortly.

Comment 10

[Lin Clark [:linclark]]

Thanks for taking this on, everyone!

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8730221 [details] [diff] [review]
make devtools/client/webconsole eslint-clean

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

::: devtools/shared/webconsole/network-helper.js
@@ +775,5 @@
>      // Make sure there's at least one param available.
>      // Be careful here, params don't necessarily need to have values, so
>      // no need to verify the existence of a "=".
> +    if (!queryString) {
> +      return [];

Here's the problem.  Making this "return null" or "return undefined" instead fixes the failure I'm seeing.

Comment 12

[Tom Tromey :tromey]

Comment on attachment 8730221 [details] [diff] [review]
make devtools/client/webconsole eslint-clean

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

::: devtools/shared/webconsole/utils.js
@@ +91,4 @@
>        temp = [];
> +      Array.forEach(object, function(value, index) {
> +        if (!filter || filter(index, value, object)) {
> +          temp.push(recursive ? this.cloneObject(value) : value);

And this should not be "this." but rather "WebConsoleUtils.".

Comment 13

[Tom Tromey :tromey]

Attachment: make devtools/client/webconsole eslint-clean

Rebased and fixes applied.

MozReview-Commit-ID: AnaXFFHlNLM

Comment 14

[Tom Tromey :tromey]

Attachment: remove obsolete TODO from network-monitor.js

MozReview-Commit-ID: Lbx8C41oSqG

Comment 15

[Tom Tromey :tromey]

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

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/fa585cc00902
https://hg.mozilla.org/integration/fx-team/rev/932af0390914

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa585cc00902
https://hg.mozilla.org/mozilla-central/rev/932af0390914