1246188 - Make webconsole.js ESLint-clean

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[AJ Kerrigan [:ajkerrigan]]

There are 28 remaining ESLint issues (13 errors, 15 warnings) in webconsole.js Clean these up so we can be one step closer to removing devtools files from the ESLint ignore list (Bug 1231957).

Comment 1

[AJ Kerrigan [:ajkerrigan]]

Attachment: Make webconsole.js ESLint-clean

Most of the changes here are cosmetic, but a couple fixes required
semantic changes (generally to enforce consistent return values or
remove unused variables/parameters). I expect more back-and-forth
in this review than in the previous rounds of webconsole eslint
cleanup.

Comment 2

[AJ Kerrigan [:ajkerrigan]]

Comment on attachment 8716416 [details] [diff] [review]
Make webconsole.js ESLint-clean

The one change I want to explicitly point out concerns mergeFilteredMessageNode():

https://bugzilla.mozilla.org/attachment.cgi?id=8716416&action=diff#a/devtools/client/webconsole/webconsole.js_sec6

The method header comment and original call signature suggest that the second parameter was being filtered out, but in reality it was being ignored. It appears that the method was figuring out the right thing to do without needing to be passed the filtered node.

The changed code should be functionally equivalent to the previous version, but it still smells funky. I stopped short of reworking it further, because I don't think I'm familiar enough with the code yet to do that without talking it through first.

Comment 3

[Lin Clark [:linclark]]

So close to having an ESLint-passing web console!

It looks like this patch is causing some test failures. You can see which ones by clicking on the orange squares in the try push (which is what we call our test server). The ones that are a result of this patch would be in dt1-dt8 and oth.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f442b5c27b6f&selectedJob=16450807

The best strategy might be to break this patch into multiple patches. First we could commit the things that should be totally safe (whitespace changes, etc). That should help isolate which changes are breaking tests.

Let me know if you'd like a walk through of any of the test stuff. I'll be onboarding a new employee next month, so I'm planning to make a couple of screencasts on that soon anyway.

Comment 4

[AJ Kerrigan [:ajkerrigan]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3f28da7e001

Comment 5

[AJ Kerrigan [:ajkerrigan]]

Attachment: patch v2

Comment 6

[AJ Kerrigan [:ajkerrigan]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5de8c4ea50d1

Comment 7

[AJ Kerrigan [:ajkerrigan]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7575e99e52a5

Comment 8

[AJ Kerrigan [:ajkerrigan]]

Attachment: patch v3

TL;DR - The try run is green now. There were issues with two specific changes, and my gut reaction was to fix the first and roll back the second. There's more detail below, and I'm happy to rework/split/discuss/etc if you have different ideas.

1. l10n --> L10n change

There were two additional places that needed a change, which
I missed in my initial searches:

- https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm#28
- https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/worker-utils.js#13

All tests related to l10n are showing up green now, so I'm inclined to leave that change in. I'm happy to break it out into a separate bug or patch though if you think that's a safer move.

2. Including timestamp when copying items to the clipboard

- https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#2710-2714

This was a strange one. ESLint was complaining that the timestampString was declared but not used. Since the comments and code suggested there would be a timestmap, I added it to the clipboard text. That caused some tests to fail, so I could either roll back the change or update the tests.

I decided to rollback the change because of how strangely it seemed the tests were failing. There would be a 'Timed out while polling clipboard for pasted data' error on a given test, when the last test to actually touch the clipboard came earlier in the test suite.

For now, I removed timestampString from copySelectedItems() altogether since it wasn't being used anyway.

Comment 9

[Lin Clark [:linclark]]

Comment on attachment 8718870 [details] [diff] [review]
patch v3

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

Nice investigation work! Both of those sound like the right call.

A couple of comments below. There have also been a few changes to hudservice.js and webconsole.js in the last few hours which make the current patch not apply, so you'll want to pull down the latest changes.

::: devtools/client/webconsole/webconsole.js
@@ +1168,5 @@
>        }
>      }
>  
>      if (dupeNode) {
> +      this.mergeFilteredMessageNode(dupeNode);

What ESLint rule was this triggering?

@@ +1892,5 @@
>    openNetworkPanel: function(requestId) {
>      let toolbox = gDevTools.getToolbox(this.owner.target);
>      // The browser console doesn't have a toolbox.
>      if (!toolbox) {
> +      return promise.reject(l10n.getStr("noToolboxInConsole"));

I'd like to hold on this one. This will be rewritten soon, so will go away as part of the rewrite.

@@ -2706,5 @@
>      for (let item of children) {
>        // Ensure the selected item hasn't been filtered by type or string.
>        if (!item.classList.contains("filtered-by-type") &&
>            !item.classList.contains("filtered-by-string")) {
> -        let timestampString = l10n.timestampString(item.timestamp);

For this, we should probably removed the "timestamp and" from the comment above, too.

@@ +3214,5 @@
>  
>      // attempt to execute the content of the inputNode
>      executeString = executeString || this.getInputValue();
>      if (!executeString) {
> +      return deferred.resolve();

I'd like to hold on this change. We'll be rewriting this method soon anyway, and this will change as part of that rewrite.

Comment 10

[AJ Kerrigan [:ajkerrigan]]

Comment on attachment 8718870 [details] [diff] [review]
patch v3

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

::: devtools/client/webconsole/webconsole.js
@@ +1168,5 @@
>        }
>      }
>  
>      if (dupeNode) {
> +      this.mergeFilteredMessageNode(dupeNode);

mergeFilteredMessageNode() originally took two parameters, but the second was not used. It triggered the "no-unused_vars" rule.

@@ +1892,5 @@
>    openNetworkPanel: function(requestId) {
>      let toolbox = gDevTools.getToolbox(this.owner.target);
>      // The browser console doesn't have a toolbox.
>      if (!toolbox) {
> +      return promise.reject(l10n.getStr("noToolboxInConsole"));

OK - I'll yank this out (including the updated method comment and additional localized string).

@@ -2706,5 @@
>      for (let item of children) {
>        // Ensure the selected item hasn't been filtered by type or string.
>        if (!item.classList.contains("filtered-by-type") &&
>            !item.classList.contains("filtered-by-string")) {
> -        let timestampString = l10n.timestampString(item.timestamp);

True, good catch. Updating this.

@@ +3214,5 @@
>  
>      // attempt to execute the content of the inputNode
>      executeString = executeString || this.getInputValue();
>      if (!executeString) {
> +      return deferred.resolve();

That works. Reverting.

Comment 11

[AJ Kerrigan [:ajkerrigan]]

Attachment: patch v4

This patch addresses the last round of review comments, and is also
squashed and rebased onto the most recent tip of central.

Comment 12

[Lin Clark [:linclark]]

I just did another try push to make sure. It hasn't fully come back yet, but there's enough for me to call it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=937ffa51d4d3&selectedJob=16803874

I'll talk with bgrins tomorrow about excluding the return rule temporarily until we get to the rewrite that will take care of the non-compliant code.

Thanks for your work on this! We're so close to having an ESLint compliant component!!

\o/

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5576824f1a67

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5576824f1a67