Closed
Bug 1274964
Opened 9 years ago
Closed 9 years ago
Fix remaining eslint errors in devtools/client/inspector
Categories
(DevTools :: Inspector, defect, P3)
DevTools
Inspector
Tracking
(firefox49 fixed)
RESOLVED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: pbro, Assigned: pbro)
References
Details
Attachments
(1 file)
No description provided.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/54608/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54608/
Attachment #8755432 -
Flags: review?(jryans)
Assignee | ||
Comment 2•9 years ago
|
||
Jryans: most of the patch should be simple enough (minor eslint changes in tests, mostly). But here are some comments about the trickier parts:
- The testActor registration and access functions aren't being exported in a common way, and so eslint can't find them automatically, so I've just added a /* globals */ comment for them.
- inspector-search.js uses a fallthrough in a switch/case that eslint doesn't like, so I ended up just disabling eslint for that line.
- inspector-panel.js is a long file that had many lines longer than 80 chars. Instead of wrapping them all and ending up with potentially weird formatting, I wanted to experiment with a 100 chars long lines, which is why there's a comment for this at the top of the file. I'd like us to have the same config for this rule everywhere in the codebase, but at the same time, this enables making files eslint-clean pretty easily. I once started to remove eslint errors in some of devtools/server/actors file, and all I was doing was wrapping lines at weird places. Just changing 80 to 100 in those cases helps a lot. And I don't think people mind too much what's the actual value, just that there is one.
- I completely rewrote browser_inspector_search-suggests-ids-and-classes.js because it looked weird and it made it easier to fix eslint errors that way. It dates from before we used tasks so much in tests and so it just didn't fit in with the other tests. The functionality is the same.
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8755432 [details]
MozReview Request: Bug 1274964 - ESLint cleanup of devtools/client/inspector; r=jryans
https://reviewboard.mozilla.org/r/54608/#review51212
::: devtools/client/inspector/inspector-panel.js:1108
(Diff revision 1)
> }
>
> - let hierarchical = aPseudo == ":hover" || aPseudo == ":active";
> - return this.walker.addPseudoClassLock(node, aPseudo, {parents: hierarchical});
> + let hierarchical = pseudo == ":hover" || pseudo == ":active";
> + return this.walker.addPseudoClassLock(node, pseudo, {parents: hierarchical});
> }
> + return null;
The walker methods called here are RDP methods, right? I would think they return promises, so this should return `promise.resolve()` or something equivalent so there is a promise returned by all paths.
::: devtools/client/inspector/inspector-panel.js:1159
(Diff revision 1)
> /**
> * Clear any pseudo-class locks applied to the current hierarchy.
> */
> clearPseudoClasses: function () {
> if (!this.walker) {
> - return;
> + return null;
Should be resolved promise.
::: devtools/client/inspector/inspector-search.js:507
(Diff revision 1)
> if (query.endsWith("*") || state === this.States.ATTRIBUTE) {
> // Hide the popup if the query ends with * (because we don't want to
> // suggest all nodes) or if it is an attribute selector (because
> // it would give a lot of useless results).
> this.hidePopup();
> - return;
> + return null;
Should this function have a return value at all? It's only bound as a event handler it seems...
::: devtools/client/inspector/test/browser_inspector_menu-01-sensitivity.js:284
(Diff revision 1)
> /**
> * A helper that simulates a contextmenu event on the given chrome DOM element.
> */
> function contextMenuClick(element) {
> let evt = element.ownerDocument.createEvent("MouseEvents");
> - let button = 2; // right click
> + let button = 2;
I am not a big fan of this rule that blocks inline comments... Just an aside. :)
Attachment #8755432 -
Flags: review?(jryans) → review+
Thanks for working on this! :)
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> ::: devtools/client/inspector/inspector-panel.js:1108
> (Diff revision 1)
> > }
> >
> > - let hierarchical = aPseudo == ":hover" || aPseudo == ":active";
> > - return this.walker.addPseudoClassLock(node, aPseudo, {parents: hierarchical});
> > + let hierarchical = pseudo == ":hover" || pseudo == ":active";
> > + return this.walker.addPseudoClassLock(node, pseudo, {parents: hierarchical});
> > }
> > + return null;
>
> The walker methods called here are RDP methods, right? I would think they
> return promises, so this should return `promise.resolve()` or something
> equivalent so there is a promise returned by all paths.
You're right. I have corrected this. Although in this case it made no difference because nothing was using this return value.
Comment 8•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•