Remove preprocessing from nsContextMenu and make it pass ESLint

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments)

nsContextMenu.js is currently pre-processed and doesn't get ESLint run on it as a result.

Additionally, nsContextMenu is one of those files that gets included in a lot of places - so rather than have to manually specify the globals it defines, I'd rather just get it converted and linted.
Comment on attachment 8850919 [details]
Bug 1350298 - Stop preprocessing nsContextMenu.js - use AppConstants instead.

https://reviewboard.mozilla.org/r/123438/#review125988
Attachment #8850919 - Flags: review?(jaws) → review+
Comment on attachment 8850920 [details]
Bug 1350298 - Fix most of the ESLint issues in nsContextMenu.js.

https://reviewboard.mozilla.org/r/123440/#review125990
Attachment #8850920 - Flags: review?(jaws) → review+
Comment on attachment 8850921 [details]
Bug 1350298 - Reduce the complexity of nsContextMenu:setTarget.

https://reviewboard.mozilla.org/r/123442/#review125992

Hmmm... tentative r+ since you didn't create this bug, but can you look in to this and either fix it here or file a separate bug?

::: browser/base/content/nsContextMenu.js:696
(Diff revision 1)
> +    if (this.target.nodeType == Node.TEXT_NODE) {
> +      // For text nodes, look at the parent node to determine the spellcheck attribute.
> +      this.canSpellCheck = this.target.parentNode &&
> +                           this._isSpellCheckEnabled(this.target);

I realize you didn't write this code but moved it, however this looks like a bug to me. The comment here doesn't match what is actually being done.

Shouldn't we be passing this.target.parentNode to _isSpellCheckEnabled?
Attachment #8850921 - Flags: review?(jaws) → review+
Comment on attachment 8850922 [details]
Bug 1350298 - Enable ESLint of nsContextMenu.js, fixing the last issues. Also enable it for finding globals for the browser-window environment.

https://reviewboard.mozilla.org/r/123444/#review125994
Attachment #8850922 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Comment on attachment 8850921 [details]
> ::: browser/base/content/nsContextMenu.js:696
> (Diff revision 1)
> > +    if (this.target.nodeType == Node.TEXT_NODE) {
> > +      // For text nodes, look at the parent node to determine the spellcheck attribute.
> > +      this.canSpellCheck = this.target.parentNode &&
> > +                           this._isSpellCheckEnabled(this.target);
> 
> I realize you didn't write this code but moved it, however this looks like a
> bug to me. The comment here doesn't match what is actually being done.
> 
> Shouldn't we be passing this.target.parentNode to _isSpellCheckEnabled?

I agree that looks wrong. I think I've also found a possible testcase on the bug where the code was originally written as well. I'll get a bug filed on that later, as I think it may need some discussion/investigation to get the right result (e.g. there's another bug which questions if we actually need canSpellCheck or not).
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b08c0c9b7820
Stop preprocessing nsContextMenu.js - use AppConstants instead. r=jaws
https://hg.mozilla.org/integration/autoland/rev/13e3f762e682
Fix most of the ESLint issues in nsContextMenu.js. r=jaws
https://hg.mozilla.org/integration/autoland/rev/b4a3c008b079
Reduce the complexity of nsContextMenu:setTarget. r=jaws
https://hg.mozilla.org/integration/autoland/rev/e57802002d61
Enable ESLint of nsContextMenu.js, fixing the last issues. Also enable it for finding globals for the browser-window environment. r=jaws
(In reply to Mark Banner (:standard8) from comment #9)
> > Shouldn't we be passing this.target.parentNode to _isSpellCheckEnabled?
> 
> I agree that looks wrong. I think I've also found a possible testcase on the
> bug where the code was originally written as well. I'll get a bug filed on
> that later, as I think it may need some discussion/investigation to get the
> right result (e.g. there's another bug which questions if we actually need
> canSpellCheck or not).

Filed bug 1350898.
You need to log in before you can comment on or make changes to this bug.