Closed Bug 1178863 Opened 4 years ago Closed 4 years ago

ESLint cleanup, batch #3

Categories

(DevTools :: Console, defect)

40 Branch
defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: jfong, Assigned: jfong)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Comment on attachment 8628840 [details] [diff] [review]
Bug1178863.patch

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

These changes look mechanical, so I've randomly reviewed half of the changed files only. I'm sure TRY will pick up error far better than me on this kind of patch.
I've only commented on a couple of minor stuff.

::: browser/devtools/webconsole/test/browser_webconsole_bug_602572_log_bodies_checkbox.js
@@ +25,5 @@
>    if (runCount == 0) {
>      requestLongerTimeout(2);
>    }
>  
> +  // open tab 2

Just curious, why did you move this function out of its original scope? Was it to reduce the complexity of the function and avoid some ESLint warning?

::: browser/devtools/webconsole/test/browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warning.js
@@ +9,5 @@
>  
> +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/" +
> +                 "test/test-bug-762593-insecure-passwords-about-blank-web-console-warning.html";
> +const INSECURE_PASSWORD_MSG = "Password fields present on an insecure " +
> +  "(http://) page. This is a security risk that allows user login " +

You formatted TEST_URI differently (aligned all string parts with the first part), maybe use a consistent style here.
Attachment #8628840 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #2)
> Comment on attachment 8628840 [details] [diff] [review]
> Bug1178863.patch
> 
> Review of attachment 8628840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These changes look mechanical, so I've randomly reviewed half of the changed
> files only. I'm sure TRY will pick up error far better than me on this kind
> of patch.
> I've only commented on a couple of minor stuff.
> 
> :::
> browser/devtools/webconsole/test/
> browser_webconsole_bug_602572_log_bodies_checkbox.js
> @@ +25,5 @@
> >    if (runCount == 0) {
> >      requestLongerTimeout(2);
> >    }
> >  
> > +  // open tab 2
> 
> Just curious, why did you move this function out of its original scope? Was
> it to reduce the complexity of the function and avoid some ESLint warning?

Yep, the ESLint rules do not want more than 3 nested levels (same as the last patch - there was one that had a similar error). I believe it was a red not a yellow (warning). Should we remove that ESLint rule and keep them nested as is until we can supported chained thens? Or leave this change as is?

> 
> :::
> browser/devtools/webconsole/test/
> browser_webconsole_bug_762593_insecure_passwords_about_blank_web_console_warn
> ing.js
> @@ +9,5 @@
> >  
> > +const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/" +
> > +                 "test/test-bug-762593-insecure-passwords-about-blank-web-console-warning.html";
> > +const INSECURE_PASSWORD_MSG = "Password fields present on an insecure " +
> > +  "(http://) page. This is a security risk that allows user login " +
> 
> You formatted TEST_URI differently (aligned all string parts with the first
> part), maybe use a consistent style here.

Okay, will update.
Flags: needinfo?(pbrosset)
(In reply to Jen Fong-Adwent [:ednapiranha] from comment #3)
> Yep, the ESLint rules do not want more than 3 nested levels (same as the
> last patch - there was one that had a similar error). I believe it was a red
> not a yellow (warning). Should we remove that ESLint rule and keep them
> nested as is until we can supported chained thens? Or leave this change as
> is?
Let's keep this change as is.
Flags: needinfo?(pbrosset)
Attached patch Bug1178863.patchSplinter Review
Updated indentation.
Attachment #8628840 - Attachment is obsolete: true
Attachment #8628979 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/69d41db364e8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.