Closed
Bug 1437848
Opened 6 years ago
Closed 6 years ago
Enable browser_console_filters.js in new frontend
Categories
(DevTools :: Console, enhancement, P1)
DevTools
Console
Tracking
(firefox60 fixed)
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: nchevobbe, Assigned: miker)
References
Details
(Whiteboard: [newconsole-mvp])
Attachments
(1 file)
No description provided.
Reporter | ||
Updated•6 years ago
|
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Priority: -- → P2
Whiteboard: [newconsole-mvp]
Updated•6 years ago
|
Severity: normal → enhancement
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
OS: Unspecified → All
Priority: P2 → P1
Hardware: Unspecified → All
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 4•6 years ago
|
||
mozreview-review |
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
Attachment #8954449 -
Flags: review?(nchevobbe) → review+
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Pushed by mratcliffe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e9edf63c2aea Enable browser_console_filters.js in new frontend r=nchevobbe
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e9edf63c2aea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•