1408945 - Enable browser_webconsole_optimized_out_vars.js in the new console frontend

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1408945 - enable browser_webconsole_optimized_out_vars.js;

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

Comment 2

[Julian Descottes [:jdescottes]]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=af033904873d2fbdb8a84c44cd27f3a35509f98d

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931435 [details]
Bug 1408945 - enable browser_webconsole_optimized_out_vars.js;

https://reviewboard.mozilla.org/r/202580/#review208112

Thanks Julian, some comments to make it even better :)

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_optimized_out_vars.js:39
(Diff revision 1)
> -    hud.jsterm.execute("upvar");
> +  hud.jsterm.execute("upvar");
> -    yield waitForMessages({
> +  await waitFor(() => findMessage(hud, "optimized out", ".message"));

can we use waitForMessage instead ? waitFor is polling, and we should only use it when we don't know when/how the message is triggered.

const onOptimizedOutMessage = waitForMessage(hud, "optimized out"); // Ommit the third param since it defaults to ".message"
hud.jsterm.execute("upvar");
await onOptimizedOutMessage;
ok(true, "Optimized out message logged");

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_optimized_out_vars.js:46
(Diff revision 1)
>  
> -function ensureThreadClientState(aPanel, aState) {
> +// Debugger helper functions adapted from devtools/client/debugger/test/head.js.
> -  let thread = aPanel.panelWin.gThreadClient;
> -  let state = thread.state;
>  
> -  info("Thread is: '" + state + "'.");
> +async function ensureThreadClientState(panel, expectedState) {

nit: rename panel to debuggerPanel to make it obvious what panel refers to

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_optimized_out_vars.js:55
(Diff revision 1)
> +    await waitForThreadEvent(panel, expectedState);
>    }
> -  return waitForThreadEvents(aPanel, aState);
>  }
>  
> -function waitForThreadEvents(aPanel, aEventName, aEventRepeat = 1) {
> +function waitForThreadEvent(panel, eventName) {

nit: rename panel to debuggerPanel to make it obvious what panel refers to

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_optimized_out_vars.js:58
(Diff revision 1)
> -  thread.addListener(aEventName, function onEvent(eventName, ...args) {
> -    info("Thread event '" + eventName + "' fired: " + (++count) + " time(s).");
> +  return new Promise(resolve => {
> +    let thread = panel.panelWin.gThreadClient;
>  
> -    if (count == aEventRepeat) {
> +    thread.addListener(eventName, function onEvent() {
> -      ok(true, "Enough '" + eventName + "' thread events have been fired.");
>        thread.removeListener(eventName, onEvent);
> -      deferred.resolve.apply(deferred, args);
> -    }
> +      resolve();
> +    });
>    });

I think we can use addOneTimeListener, which returns a promise.

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8931435 [details]
Bug 1408945 - enable browser_webconsole_optimized_out_vars.js;

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

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931435 [details]
Bug 1408945 - enable browser_webconsole_optimized_out_vars.js;

https://reviewboard.mozilla.org/r/202580/#review208410

One minor comment, but this is good, thanks Julian

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_optimized_out_vars.js:55
(Diff revision 2)
>  
> -  if (state == aState) {
> -    return promise.resolve(null);
> +async function ensureThreadClientState(debuggerPanel, state) {
> +  let thread = debuggerPanel.panelWin.gThreadClient;
> +  info(`Thread is: '${thread.state}'.`);
> +  if (thread.state != state) {
> +    info("Waiting for thread event: '${eventName}'.");

I think `eventName` should be `state`

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8931435 [details]
Bug 1408945 - enable browser_webconsole_optimized_out_vars.js;

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ef48daab640
enable browser_webconsole_optimized_out_vars.js;r=nchevobbe

Comment 8

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/7ef48daab640

Comment 9

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ef48daab640