Closed Bug 1442305 Opened 2 years ago Closed 2 years ago

setFilterState() in head.js fails when setting multiple categories

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(1 file)

setFilterState() in head.js returns early when setting multiple categories if no change is needed and we are setting multiple categories.

Basically, I used return in a loop when I should have used a continue.
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Comment on attachment 8955202 [details]
Bug 1442305 - setFilterState() in head.js fails when setting multiple categories

https://reviewboard.mozilla.org/r/224366/#review230562

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:673
(Diff revision 1)
>      if (check) {
>        // Enable filter.
>        if (checked) {
> -        return;
> +        continue;
>        }
>        button.click();
>        await waitFor(() => {
>          return button.classList.contains("checked");
>        });
>      } else if (checked) {
>        // Disable filter.
>        button.click();
>        await waitFor(() => {
>          return !button.classList.contains("checked");
>        });
>      }

I find `continue` statement hard to read, you need to go up see which loop you're in and you can lose track of what the code is doing.

Could we reorder things a bit so it's more clear what is done ?

```js
// If the button isn't already in the state we want.
if (check !== buttonIsChecked) {
  button.click();
  await waitFor(() => 
    button.classList.contains("checked") === check);
}
```

What do you think of this ?
Comment on attachment 8955202 [details]
Bug 1442305 - setFilterState() in head.js fails when setting multiple categories

https://reviewboard.mozilla.org/r/224366/#review230618

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:673
(Diff revision 1)
>      if (check) {
>        // Enable filter.
>        if (checked) {
> -        return;
> +        continue;
>        }
>        button.click();
>        await waitFor(() => {
>          return button.classList.contains("checked");
>        });
>      } else if (checked) {
>        // Disable filter.
>        button.click();
>        await waitFor(() => {
>          return !button.classList.contains("checked");
>        });
>      }

Yup... not sure what I was thinking.
Comment on attachment 8955202 [details]
Bug 1442305 - setFilterState() in head.js fails when setting multiple categories

https://reviewboard.mozilla.org/r/224366/#review230562

> I find `continue` statement hard to read, you need to go up see which loop you're in and you can lose track of what the code is doing.
> 
> Could we reorder things a bit so it's more clear what is done ?
> 
> ```js
> // If the button isn't already in the state we want.
> if (check !== buttonIsChecked) {
>   button.click();
>   await waitFor(() => 
>     button.classList.contains("checked") === check);
> }
> ```
> 
> What do you think of this ?

Makes way more sense... not sure what I was thinking.
Comment on attachment 8955202 [details]
Bug 1442305 - setFilterState() in head.js fails when setting multiple categories

https://reviewboard.mozilla.org/r/224366/#review230644

Looks good, thanks for addressing the comment Mike
Attachment #8955202 - Flags: review?(nchevobbe) → review+
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/705748b426e1
setFilterState() in head.js fails when setting multiple categories r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/705748b426e1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.