1350298 - Remove preprocessing from nsContextMenu and make it pass ESLint

Status: RESOLVED FIXED

(Firefox :: Menus, enhancement)

Description

[Mark Banner (:standard8)]

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 1

[Mark Banner (:standard8)]

Attachment: Bug 1350298 - Stop preprocessing nsContextMenu.js - use AppConstants instead.

Review commit: https://reviewboard.mozilla.org/r/123438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123438/

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1350298 - Fix most of the ESLint issues in nsContextMenu.js.

Review commit: https://reviewboard.mozilla.org/r/123440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123440/

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1350298 - Reduce the complexity of nsContextMenu:setTarget.

Review commit: https://reviewboard.mozilla.org/r/123442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123442/

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1350298 - Enable ESLint of nsContextMenu.js, fixing the last issues. Also enable it for finding globals for the browser-window environment.

Review commit: https://reviewboard.mozilla.org/r/123444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123444/

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8850919 [details]
Bug 1350298 - Stop preprocessing nsContextMenu.js - use AppConstants instead.

https://reviewboard.mozilla.org/r/123438/#review125988

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8850920 [details]
Bug 1350298 - Fix most of the ESLint issues in nsContextMenu.js.

https://reviewboard.mozilla.org/r/123440/#review125990

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 9

[Mark Banner (:standard8)]

(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).

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Mark Banner (:standard8)]

(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.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b08c0c9b7820
https://hg.mozilla.org/mozilla-central/rev/13e3f762e682
https://hg.mozilla.org/mozilla-central/rev/b4a3c008b079
https://hg.mozilla.org/mozilla-central/rev/e57802002d61