Closed Bug 1255799 Opened 4 years ago Closed 4 years ago

make devtools/shared/webconsole eslint-clean

Categories

(DevTools :: Console, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(2 files, 3 obsolete files)

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.
MozReview-Commit-ID: AnaXFFHlNLM
MozReview-Commit-ID: Lbx8C41oSqG
Attachment #8729565 - Flags: review?(odvarko)
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.
Attachment #8729566 - Flags: review?(odvarko)
Can you remove devtools/client/netmonitor from eslintignore while you're changing the file? Honza forgot to do it in bug 1252807
Updated per request; and also for changes made in bug 1255827.

MozReview-Commit-ID: AnaXFFHlNLM
Attachment #8729565 - Attachment is obsolete: true
Attachment #8729565 - Flags: review?(odvarko)
Attachment #8730221 - Flags: review?(odvarko)
Attachment #8729566 - Flags: review?(odvarko) → review+
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.
Attachment #8730221 - Flags: review?(odvarko) → review+
(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.
That try push certainly looks terrible.
I will investigate shortly.
Thanks for taking this on, everyone!
Priority: -- → P2
Whiteboard: [btpp-fix-later]
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 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.".
Rebased and fixes applied.

MozReview-Commit-ID: AnaXFFHlNLM
Attachment #8729566 - Attachment is obsolete: true
Attachment #8730221 - Attachment is obsolete: true
MozReview-Commit-ID: Lbx8C41oSqG
Attachment #8731271 - Flags: review+
Attachment #8731272 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fa585cc00902
https://hg.mozilla.org/mozilla-central/rev/932af0390914
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.