1437848 - Enable browser_console_filters.js in new frontend

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Comment 1

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

Attachment: Bug 1437848 - Enable browser_console_filters.js in new frontend

This patch enables `browser_console_filters.js` and also adds some `head.js` methods for controlling filters. I have used these new methods in `browser_console_nsiconsolemessage.js` and `browser_webconsole_filters.js`.

All tests touched by this patch pass when ran using `--verify`.

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

Comment 2

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

Comment on attachment 8954449 [details]
Bug 1437848 - Enable browser_console_filters.js in new frontend

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

Comment 3

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

Comment on attachment 8954449 [details]
Bug 1437848 - Enable browser_console_filters.js in new frontend

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8954449 [details]
Bug 1437848 - Enable browser_console_filters.js in new frontend

https://reviewboard.mozilla.org/r/223528/#review229806

Thanks Mike !
I have some questions and  suggestions mostly about the helpers functions, but this patch is good to go with a green try

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:630
(Diff revision 3)
> + *          - debug
> + *          - css (disabled by default)
> + *          - netxhr (disabled by default)
> + *          - net (disabled by default)
> + */
> +async function getFilterState(hud, category) {

could we return the full state instead of a given property ? In some cases, we call this N times to get all the different categories, returning an object instead could be simpler.

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:634
(Diff revision 3)
> + */
> +async function getFilterState(hud, category) {
> +  const filterBar = await setFilterBarVisible(hud, true);
> +  const button = filterBar.querySelector(`.${category}`);
> +
> +  ok(button, `getFilterState: The ${category} button exists.`);

not sure if we should put assertions in helper methods. We do have tests for those assertions, and here we might just pollute the output of tests, which are already hard to parse.

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:649
(Diff revision 3)
> + *        The category of the tab for which to get the state of. See
> + *        getFilterState() for details.
> + * @param {Boolean}
> + *        Enable or disable the specified filter.
> + */
> +async function setFilterState(hud, category, state) {

similar to the other change, could we allow the user to pass an object to override the state, like: 

```js
setFilterState(hud, {
  error: false,
  netxhr: true
});
```

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:655
(Diff revision 3)
> +  info(`Setting the ${category} category to ${state ? "stated" : "disabled"}`);
> +
> +  const filterBar = await setFilterBarVisible(hud, true);
> +  const button = filterBar.querySelector(`.${category}`);
> +
> +  ok(button, `setFilterState: The ${category} button exists.`);

not sure if we should put assertions in helper methods. We do have tests for those assertions, and here we might just pollute the output of tests, which are already hard to parse.

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:682
(Diff revision 3)
> +/**
> + * Set the visibility of the filter bar.
> + *
> + * @param {Object} hud
> + * @param {Boolean} state
> + *        Set filter bar visibility?

is the question mark wanted ?

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:695
(Diff revision 3)
> +    // Show filter bar if it is hidden.
> +    if (!filterBar) {

maybe we could return only with the opposite test: 

```js
// if the filter bar is already visible, returns it.
if (filterBar) {
  return filterBar;
}

// Click the filter icon to show the filter bar.
…
```

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:703
(Diff revision 3)
> +      toolbar.querySelector(".devtools-filter-icon").click();
> +      filterBar = await waitFor(() => {
> +        return outputNode.querySelector(".webconsole-filterbar-secondary");
> +      });
> +
> +      ok(filterBar, "Filter bar is shown when filter icon is clicked.");

not sure if we should put assertions in helper methods. We do have tests for those assertions, and here we might just pollute the output of tests, which are already hard to parse.

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:721
(Diff revision 3)
> +/**
> + * Reset the filters at the end of a test that has changed them. This is
> + * important when using the `--verify` test option as when it is used you need
> + * to manually reset the filters.
> + *
> + * The css, netxhr and net filters are disabled by default.
> + *
> + * @param {Object} hud
> + */
> +async function resetFilters(hud) {
> +  info("Resetting filters to their default state");
> +
> +  const filterBar = await setFilterBarVisible(hud, true);
> +  const buttons = filterBar.querySelectorAll("button");
> +
> +  for (let button of buttons) {
> +    if (button.classList.contains("css") ||
> +        button.classList.contains("netxhr") ||
> +        button.classList.contains("net")) {
> +      info(`Setting button with class "${button.className}" to an unchecked state`);
> +      if (button.classList.contains("checked")) {
> +        button.click();
> +        await waitFor(() => {
> +          return !button.classList.contains("checked");
> +        });
> +      }
> +    } else {
> +      info(`Setting button with class "${button.className}" to a checked state`);
> +      if (!button.classList.contains("checked")) {
> +        button.click();
> +        await waitFor(() => {
> +          return button.classList.contains("checked");
> +        });
> +      }
> +    }
> +  }

we do have an action for that https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/devtools/client/webconsole/new-console-output/actions/filters.js#39-52

Maybe we could simply dispatch it from here. This way, we'd always do the right thing even if filters amd/or their default state changes ?

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:759
(Diff revision 3)
> +      }
> +    }
> +  }
> +
> +  // Hide the filter bar.
> +  await setFilterBarVisible(hud, false);

not sure if we should do this here. There's no indication in the name that we would also hide the filter bar. If a test needs it, let it call setFilterBarVisible itself

Comment 5

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

Comment on attachment 8954449 [details]
Bug 1437848 - Enable browser_console_filters.js in new frontend

https://reviewboard.mozilla.org/r/223528/#review229816

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:630
(Diff revision 3)
> + *          - debug
> + *          - css (disabled by default)
> + *          - netxhr (disabled by default)
> + *          - net (disabled by default)
> + */
> +async function getFilterState(hud, category) {

Done

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:655
(Diff revision 3)
> +  info(`Setting the ${category} category to ${state ? "stated" : "disabled"}`);
> +
> +  const filterBar = await setFilterBarVisible(hud, true);
> +  const button = filterBar.querySelector(`.${category}`);
> +
> +  ok(button, `setFilterState: The ${category} button exists.`);

Okay, now we only assert if the category doesn't exist.

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:682
(Diff revision 3)
> +/**
> + * Set the visibility of the filter bar.
> + *
> + * @param {Object} hud
> + * @param {Boolean} state
> + *        Set filter bar visibility?

Not really.

Comment 6

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

Comment on attachment 8954449 [details]
Bug 1437848 - Enable browser_console_filters.js in new frontend

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223528/diff/3-4/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9edf63c2aea
Enable browser_console_filters.js in new frontend r=nchevobbe

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e9edf63c2aea