Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

It was pointed out in https://bugzil.la/1405004#c2 that it would be
a good idea to lint for unused variables.  It does not look like the
no-unused-vars lint rule is enabled in the default eslint config.

It is described in more detail here:

	https://eslint.org/docs/rules/no-unused-vars

It has an option for also linting for calls to functions without
using all arguments, which seems sensible.  I will investigate if
that makes sense for Marionette.

A quick look at this shows we have 146 new errors that need to be
fixed before we can enable these lint rules.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8914731 [details]
Bug 1405279 - Remove unused variables.

https://reviewboard.mozilla.org/r/186012/#review191382

Doh! What a clean-up! That is amazing...

::: testing/marionette/element.js:929
(Diff revision 1)
>  
>  // TODO(ato): Not implemented.
>  // In fact, it's not defined in the spec.
> -element.isKeyboardInteractable = function(el) {
> +element.isKeyboardInteractable = function() {
>    return true;
>  };

We could just use:

`element.isKeyboardInteractable = () => true;`

::: testing/marionette/evaluate.js:21
(Diff revision 1)
>    JavaScriptError,
>    ScriptTimeoutError,
>    WebDriverError,
>  } = Cu.import("chrome://marionette/content/error.js", {});
>  
> -const logger = Log.repository.getLogger("Marionette");
> +const log = Log.repository.getLogger("Marionette");

Please leave logger here.

::: testing/marionette/listener.js
(Diff revision 1)
>  // Append only once to avoid duplicated output after listener.js gets reloaded
>  if (logger.ownAppenders.length == 0) {
>    logger.addAppender(new Log.DumpAppender());
>  }
>  
> -const modalHandler = function() {

Strange that we don't use that anymore. Are we having a different code path now?

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8914732 [details]
Bug 1405279 - Lint for unused variables.

https://reviewboard.mozilla.org/r/186014/#review191384

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 5 • 7 years ago
mozreview-review-reply

Comment on attachment 8914731 [details]
Bug 1405279 - Remove unused variables.

https://reviewboard.mozilla.org/r/186012/#review191382

> We could just use:
> 
> `element.isKeyboardInteractable = () => true;`

Fixed.

> Strange that we don't use that anymore. Are we having a different code path now?

We monitor modals in chrome space.  I can’t remember when this was
last used.

	Pulsebot
	Comment 8 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2da9eaeb0ea2
Remove unused variables. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/7a589e9d10ab
Lint for unused variables. r=whimboo

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 9 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2da9eaeb0ea2
https://hg.mozilla.org/mozilla-central/rev/7a589e9d10ab