1419087 - Clearing the console should close the sidebar

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

It should work either if we click the "Clear output" button, react to a console.clear() message or a Cmd/Ctrl + L keypress.

Comment 1

[Mike Park [:mparkms]]

Attachment: Bug 1419087 - Clearing the console closes the sidebar.

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932234 [details]
Bug 1419087 - Clearing the console closes the sidebar.

https://reviewboard.mozilla.org/r/203262/#review209046

The code looks great and it is almost ready to land.
I'd like to see if we can make the mochitest even better.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_close_sidebar.js:13
(Diff revision 1)
> +  await SpecialPowers.pushPrefEnv({"set": [
> +    ["devtools.webconsole.sidebarToggle", true]
> +  ]});

Add a comment to say this should be removed when the META for adding the object in sidebar is resolved

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_close_sidebar.js:22
(Diff revision 1)
> +  let hud = await openNewTabAndConsole(TEST_URI);
> +  await showSidebar(hud);
> +
> +  info("Click the clear console button");
> +  let clearButton = hud.ui.document.querySelector(".devtools-button");
> +  EventUtils.synthesizeMouseAtCenter(clearButton, {}, hud.ui.window);

nit: if it works, use clearButton.click() directly

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_close_sidebar.js:24
(Diff revision 1)
> +
> +  info("Click the clear console button");
> +  let clearButton = hud.ui.document.querySelector(".devtools-button");
> +  EventUtils.synthesizeMouseAtCenter(clearButton, {}, hud.ui.window);
> +  await waitFor(() => findMessages(hud, "").length == 0);
> +  sidebar = hud.ui.document.querySelector(".sidebar");

we shouldn't create a global (`let sidebar = …`)

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_close_sidebar.js:30
(Diff revision 1)
> +  ok(!sidebar, "Sidebar hidden after clear console button clicked");
> +
> +  await showSidebar(hud);
> +
> +  info("Send a console.clear()");
> +  ContentTask.spawn(gBrowser.selectedBrowser, {}, function* () {

fhe function does not need to be a generator function

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_close_sidebar.js:30
(Diff revision 1)
> +  ContentTask.spawn(gBrowser.selectedBrowser, {}, function* () {
> +    content.wrappedJSObject.console.clear();
> +  });
> +  await waitFor(() => findMessage(hud, "Console was cleared"));

we should use waitMessage here 

```
let onMessagesCleared = waitMessage("Console was cleared");

ContentTask.spawn(gBrowser.selectedBrowser, {}, function () {
  content.wrappedJSObject.console.clear();
});
await onMessagesCleared;
```

waitFor is using some polling (checking every 50ms if the assertion is true), which is not the cleanest way of checking for things. Here we know when the message that we want to wait for will be dispatched, so using waitForMessage (which listen for a specific event) is better.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_close_sidebar.js:54
(Diff revision 1)
> +  ok(!sidebar, "Sidebar hidden after console.clear()");
> +});
> +
> +async function showSidebar(hud) {
> +  let toggleButton = hud.ui.document.querySelector(".webconsole-sidebar-button");
> +  EventUtils.synthesizeMouseAtCenter(toggleButton, {}, hud.ui.window);

can we try to use toggleButton.click ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_close_sidebar.js:54
(Diff revision 1)
> +  EventUtils.synthesizeMouseAtCenter(toggleButton, {}, hud.ui.window);
> +  await waitUntil(() => hud.ui.document.querySelector(".sidebar"));

maybe we could wait for a dom mutation instead of polling ?
We do have an helper in the head.js. We would wait for the Sidebar parent container to have new children (see https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_inspect.js#35-37)

::: devtools/client/webconsole/new-console-output/test/store/ui.test.js:28
(Diff revision 1)
> +    it("sidebar is hidden on clear", () => {
> +      store.dispatch(actions.sidebarToggle());
> +      expect(store.getState().ui.sidebarVisible).toEqual(true);
> +      store.dispatch(actions.messagesClear());
> +      expect(store.getState().ui.sidebarVisible).toEqual(false);
> +      store.dispatch(actions.messagesClear());
> +      expect(store.getState().ui.sidebarVisible).toEqual(false);
> +    });

nice and clean

Comment 3

[Mike Park [:mparkms]]

Comment on attachment 8932234 [details]
Bug 1419087 - Clearing the console closes the sidebar.

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932234 [details]
Bug 1419087 - Clearing the console closes the sidebar.

https://reviewboard.mozilla.org/r/203262/#review209284

Looks good ! Thanks for making the changes Mike.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

TRY is green let's land this

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/018a1627f33f
Clearing the console closes the sidebar. r=nchevobbe

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/018a1627f33f