Closed Bug 1404851 Opened 2 years ago Closed 2 years ago

Migrate browser_webconsole_bug_651501_document_body_autocomplete.js to the new frontend

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox57 wontfix, firefox58 fix-optional, firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox57 --- wontfix
firefox58 --- fix-optional
firefox59 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

(Blocks 1 open bug)

Details

(Whiteboard: [newconsole-mvp])

Attachments

(1 file)

No description provided.
Priority: -- → P3
Priority: P3 → P2
Whiteboard: [newconsole-mvp]
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8932801 [details]
Bug 1404851 - Rename, adapt and enable browser_webconsole_document_body_autocomplete; .

https://reviewboard.mozilla.org/r/203854/#review209346

Looks good to me, thanks!

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_document_body_autocomplete.js:6
(Diff revision 1)
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
>  /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  // Tests that document.body autocompletes in the web console. See Bug 651501.

It looks like the reason for this test is that document.body is actually a native getter and we used not to provide autocompletion on native getters (risk of side effects etc...). Maybe mention that in the test's main comment, otherwise it's hard to understand why we have this test on top of browser_jsterm_completion.js and browser_jsterm_autocomplete_escape_key.js

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_document_body_autocomplete.js:18
(Diff revision 1)
> +  let { jsterm } = await openNewTabAndConsole(TEST_URI);
>  
> -add_task(function* () {
> -  yield loadTab(TEST_URI);
> +  const {
> +    autocompletePopup: popup,
> +    completeNode,
> +    inputNode,

eslint: inputNode is not used

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_document_body_autocomplete.js:33
(Diff revision 1)
> -
> -    is(popup.itemCount, jsterm._autocompleteCache.length,
> +  is(popup.itemCount, jsterm._autocompleteCache.length, "popup.itemCount is correct");
> +  ok(jsterm._autocompleteCache.includes("addEventListener"),
> -       "popup.itemCount is correct");
> -    isnot(jsterm._autocompleteCache.indexOf("addEventListener"), -1,
> -          "addEventListener is in the list of suggestions");
> +        "addEventListener is in the list of suggestions");
> -    isnot(jsterm._autocompleteCache.indexOf("bgColor"), -1,
> +  ok(jsterm._autocompleteCache.includes("bgColor"), "bgColor is in the list of suggestions");

eslint: > 90chars

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_document_body_autocomplete.js:34
(Diff revision 1)
> -    is(popup.itemCount, jsterm._autocompleteCache.length,
> +  ok(jsterm._autocompleteCache.includes("addEventListener"),
> -       "popup.itemCount is correct");
> -    isnot(jsterm._autocompleteCache.indexOf("addEventListener"), -1,
> -          "addEventListener is in the list of suggestions");
> +        "addEventListener is in the list of suggestions");
> -    isnot(jsterm._autocompleteCache.indexOf("bgColor"), -1,
> -          "bgColor is in the list of suggestions");
> +  ok(jsterm._autocompleteCache.includes("bgColor"), "bgColor is in the list of suggestions");
> +  ok(jsterm._autocompleteCache.includes("ATTRIBUTE_NODE"), "ATTRIBUTE_NODE is in the list of suggestions");

eslint: > 90chars

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_document_body_autocomplete.js:46
(Diff revision 1)
> -  });
> -
>    let inputStr = "document.b";
>    jsterm.setInputValue(inputStr);
>    EventUtils.synthesizeKey("o", {});
>    let testStr = inputStr.replace(/./g, " ") + " ";

nit: move this variable right before the `is(completeNode.value, testStr + [...])` as it is only needed on this line. 
I would also be fine with having just "           dy" (+ a comment) for this test rather than having a regexp.
Attachment #8932801 - Flags: review?(jdescottes) → review+
Thanks for the review Julian. I renamed the test so it is more obvious what it does, and added some comments.
Pushed to TRY before landing
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a91f7382c96
Rename, adapt and enable browser_webconsole_document_body_autocomplete; r=jdescottes.
https://hg.mozilla.org/mozilla-central/rev/4a91f7382c96
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.