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

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1442305 - setFilterState() in head.js fails when setting multiple categories

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

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 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8955202 [details]
Bug 1442305 - setFilterState() in head.js fails when setting multiple categories

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224366/diff/1-2/

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/705748b426e1
setFilterState() in head.js fails when setting multiple categories r=nchevobbe

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/705748b426e1