Bug 1510422 - Fix autocomplete cache handling; r=Honza.
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Thu, 29 Nov 2018 13:58:57 +0000
changeset 507929 85dab0b936897a3ce5d7f96bdd2a1c92bc0a8500
parent 507928 6c5424f777093578ce457708056e2f0d72c6b5e4
child 507953 a89d378954538f7fe0cad49681b409e80c3f8a0f
child 507955 a865260c7f9741eb5c875c32ea16f59ac19db9cb
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersHonza
bugs1510422, 1462394
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1510422 - Fix autocomplete cache handling; r=Honza. In Bug 1462394, we moved the autocomplete data handling out of the JsTerm to the Redux store. In the process, we regress some cases like `await n`, which should display `navigator`, but isn't anymore when the user types the whole sequence. Ctrl+Space would still show the popup, which indicates that the issue is not on the server-side. This issue is caused because our new code decides that we should hit the cache when typing the `n`, and there's nothing in the cache. Previously, we were clearing the cache as soon as the input last string wasn't alphanumeric, which we don't anymore. To fix that, instead of relying on the last string of the input (which could be wrong in cases like `x.["hello `), we clear the cache when the autocomplete service returns a null `matches` property. In the JsPropertyProvider, we use to return null whenever there isn't any search done (incorrect input, empty match prop, …). So it seems like a good idea to bust the cache when the server returns null. This requires some changes to the autocomplete service, as well as some in jsPropertyProvider (e.g. to handle `await `). Tests are added both on the client and the frontend to make sure we don't regress this (those tests fail without the actual fix). Differential Revision: https://phabricator.services.mozilla.com/D13231
devtools/client/webconsole/reducers/autocomplete.js
devtools/client/webconsole/test/mochitest/browser.ini
devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_await.js
devtools/server/actors/webconsole.js
devtools/shared/webconsole/js-property-provider.js
devtools/shared/webconsole/test/test_jsterm_autocomplete.html
--- a/devtools/client/webconsole/reducers/autocomplete.js
+++ b/devtools/client/webconsole/reducers/autocomplete.js
@@ -32,16 +32,20 @@ function autocomplete(state = getDefault
         cache: null,
         pendingRequestId: action.id,
       };
     case AUTOCOMPLETE_DATA_RECEIVE:
       if (action.id !== state.pendingRequestId) {
         return state;
       }
 
+      if (action.data.matches === null) {
+        return getDefaultState();
+      }
+
       return {
         ...state,
         cache: {
           input: action.input,
           frameActorId: action.frameActorId,
           ...action.data,
         },
         pendingRequestId: null,
--- a/devtools/client/webconsole/test/mochitest/browser.ini
+++ b/devtools/client/webconsole/test/mochitest/browser.ini
@@ -183,16 +183,17 @@ skip-if = verify
 [browser_console_webconsole_ctrlw_close_tab.js]
 [browser_console_webconsole_iframe_messages.js]
 [browser_console_webconsole_private_browsing.js]
 [browser_jsterm_accessibility.js]
 [browser_jsterm_add_edited_input_to_history.js]
 [browser_jsterm_autocomplete_accept_no_scroll.js]
 [browser_jsterm_autocomplete_array_no_index.js]
 [browser_jsterm_autocomplete_arrow_keys.js]
+[browser_jsterm_autocomplete_await.js]
 [browser_jsterm_autocomplete_cached_results.js]
 [browser_jsterm_autocomplete_commands.js]
 [browser_jsterm_autocomplete_control_space.js]
 [browser_jsterm_autocomplete_crossdomain_iframe.js]
 [browser_jsterm_autocomplete_escape_key.js]
 [browser_jsterm_autocomplete_extraneous_closing_brackets.js]
 [browser_jsterm_autocomplete_helpers.js]
 [browser_jsterm_autocomplete_in_chrome_tab.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_await.js
@@ -0,0 +1,44 @@
+/* -*- 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/ */
+
+"use strict";
+
+// See Bug 585991.
+
+const TEST_URI = `data:text/html;charset=utf-8,Autocomplete await expression`;
+
+add_task(async function() {
+  // Run test with legacy JsTerm
+  await pushPref("devtools.webconsole.jsterm.codeMirror", false);
+  await performTests();
+  // And then run it with the CodeMirror-powered one.
+  await pushPref("devtools.webconsole.jsterm.codeMirror", true);
+  await performTests();
+});
+
+async function performTests() {
+  const { jsterm } = await openNewTabAndConsole(TEST_URI);
+  const { autocompletePopup } = jsterm;
+
+  info("Check that the await keyword is in the autocomplete");
+  await setInputValueForAutocompletion(jsterm, "aw");
+  checkJsTermCompletionValue(jsterm, "  ait", "completeNode has expected value");
+
+  EventUtils.synthesizeKey("KEY_Tab");
+  is(jsterm.getInputValue(), "await", "'await' tab completion");
+
+  const updated = jsterm.once("autocomplete-updated");
+  EventUtils.sendString(" ");
+  await updated;
+
+  info("Check that the autocomplete popup is displayed");
+  const onPopUpOpen = autocompletePopup.once("popup-opened");
+  EventUtils.sendString("P");
+  await onPopUpOpen;
+
+  ok(autocompletePopup.isOpen, "popup is open");
+  ok(autocompletePopup.items.some(item => item.label === "Promise"),
+    "popup has expected `Promise` item");
+}
--- a/devtools/server/actors/webconsole.js
+++ b/devtools/server/actors/webconsole.js
@@ -1212,22 +1212,29 @@ WebConsoleActor.prototype =
       const result = JSPropertyProvider({
         dbgObject,
         environment,
         inputValue: request.text,
         cursor: request.cursor,
         invokeUnsafeGetter: false,
         webconsoleActor: this,
         selectedNodeActor: request.selectedNodeActor,
-      }) || {};
+      });
 
       if (!hadDebuggee && dbgObject) {
         this.dbg.removeDebuggee(this.evalWindow);
       }
 
+      if (result === null) {
+        return {
+          from: this.actorID,
+          matches: null,
+        };
+      }
+
       matches = result.matches || new Set();
       matchProp = result.matchProp;
       isElementAccess = result.isElementAccess;
 
       // We consider '$' as alphanumeric because it is used in the names of some
       // helper functions; we also consider whitespace as alphanum since it should not
       // be seen as break in the evaled string.
       const lastNonAlphaIsDot = /[.][a-zA-Z0-9$\s]*$/.test(reqText);
--- a/devtools/shared/webconsole/js-property-provider.js
+++ b/devtools/shared/webconsole/js-property-provider.js
@@ -108,30 +108,34 @@ function analyzeInputString(str) {
           const after = characters.slice(i + 1);
           const trimmedBefore = Array.from(before.join("").trimRight());
           const trimmedAfter = Array.from(after.join("").trimLeft());
 
           const nextNonSpaceChar = trimmedAfter[0];
           const nextNonSpaceCharIndex = after.indexOf(nextNonSpaceChar);
           const previousNonSpaceChar = trimmedBefore[trimmedBefore.length - 1];
 
-          // There's only spaces after that, so we can return.
-          if (!nextNonSpaceChar) {
-            return buildReturnObject();
-          }
-
           // If the previous char isn't a dot or opening bracket, and the next one isn't
           // one either, and the current computed statement is not a
           // variable/function/class declaration, update the start position.
           if (
             previousNonSpaceChar !== "." && nextNonSpaceChar !== "." &&
             previousNonSpaceChar !== "[" && nextNonSpaceChar !== "[" &&
             !NO_AUTOCOMPLETE_PREFIXES.includes(currentLastStatement)
           ) {
-            start = i + nextNonSpaceCharIndex;
+            start = i + (
+              nextNonSpaceCharIndex >= 0
+                ? nextNonSpaceCharIndex
+                : (after.length + 1)
+            );
+          }
+
+          // There's only spaces after that, so we can return.
+          if (!nextNonSpaceChar) {
+            return buildReturnObject();
           }
 
           // Let's jump to handle the next non-space char.
           i = i + nextNonSpaceCharIndex;
         } else if (OPEN_BODY.includes(c)) {
           bodyStack.push({
             token: c,
             start,
--- a/devtools/shared/webconsole/test/test_jsterm_autocomplete.html
+++ b/devtools/shared/webconsole/test/test_jsterm_autocomplete.html
@@ -12,16 +12,18 @@
 <p>Test for JavaScript terminal autocomplete functionality</p>
 
 <script class="testbody" type="text/javascript">
   SimpleTest.waitForExplicitFinish();
   const {
     MAX_AUTOCOMPLETE_ATTEMPTS,
     MAX_AUTOCOMPLETIONS
   } = require("devtools/shared/webconsole/js-property-provider");
+  const RESERVED_JS_KEYWORDS = require("devtools/shared/webconsole/reserved-js-words");
+
 
   addEventListener("load", startTest);
 
   async function startTest() {
     // First run the tests with a tab as a target.
     let state = await new Promise(resolve => attachConsoleToTab(["PageError"], resolve));
     await performTests({state, isWorker: false});
 
@@ -101,16 +103,17 @@
       doAutocompleteProxyThrowsOwnKeys,
       doAutocompleteDotSurroundedBySpaces,
       doAutocompleteAfterOr,
       doInsensitiveAutocomplete,
       doElementAccessAutocomplete,
       doAutocompleteAfterOperator,
       dontAutocompleteAfterDeclaration,
       doKeywordsAutocomplete,
+      dontAutocomplete,
     ];
 
     if (!isWorker) {
       // `Cu` is not defined in workers, then we can't test `Cu.Sandbox`
       tests.push(doAutocompleteSandbox);
       // Some cases are handled in worker context because we can't use Parser.jsm.
       // See Bug 1507181.
       tests.push(
@@ -161,17 +164,17 @@
       ["foo", "foobar", "foobaz", "omg", "omgfoo", "omgstr", "strfoo"]);
   }
 
   async function doAutocomplete4(client) {
     // Check that completion requests can have no suggestions.
     info("test autocomplete for 'dump(window.foobarObject.)'");
     let response = await client.autocomplete("dump(window.foobarObject.)");
     ok(!response.matchProp, "matchProp");
-    is(response.matches.length, 0, "matches.length");
+    is(response.matches, null, "matches is null");
   }
 
   async function doAutocompleteLarge1(client) {
     // Check that completion requests with too large objects will
     // have no suggestions.
     info("test autocomplete for 'window.largeObject1.'");
     let response = await client.autocomplete("window.largeObject1.");
     ok(!response.matchProp, "matchProp");
@@ -414,33 +417,29 @@
     is(
       res.matches.join("|"),
       "`da\"ta\"test`|`da'ta'test`|`da\\`ta\\`test`|`data-test`|`dataTest`",
       "autocomplete returns the expected items, wrapped in the same quotes the user entered");
     is(res.isElementAccess, true);
 
     info(`test autocomplete for '['`);
     res = await client.autocomplete(`[`);
-    is(res.matches.length, 0, "it does not return anything");
-    is(res.isElementAccess, false);
+    is(res.matches, null, "it does not return anything");
 
     info(`test autocomplete for '[1,2,3'`);
     res = await client.autocomplete(`[1,2,3`);
-    is(res.matches.length, 0, "it does not return anything");
-    is(res.isElementAccess, false);
+    is(res.matches, null, "it does not return anything");
 
     info(`test autocomplete for '["'`);
     res = await client.autocomplete(`["`);
-    is(res.matches.length, 0, "it does not return anything");
-    is(res.isElementAccess, false);
+    is(res.matches, null, "it does not return anything");
 
     info(`test autocomplete for '[;'`);
     res = await client.autocomplete(`[;`);
-    is(res.matches.length, 0, "it does not return anything");
-    is(res.isElementAccess, false);
+    is(res.matches, null, "it does not return anything");
   }
 
   async function doAutocompleteCommands(client) {
     info("test autocomplete for 'c'");
     let matches = (await client.autocomplete("c")).matches;
     ok(matches.includes("cd") && matches.includes("clear"), "commands are returned");
 
     info("test autocomplete for 's'");
@@ -511,33 +510,33 @@
       let matches = (await client.autocomplete(input)).matches;
       ok(matches.includes("foobarObject"), `Expected autocomplete result for ${input}"`);
     }
   }
 
   async function dontAutocompleteAfterDeclaration(client) {
     info("test autocomplete for 'var win'");
     let matches = (await client.autocomplete("var win")).matches;
-    is(matches.length, 0, "no autocompletion on a var declaration");
+    is(matches, null, "no autocompletion on a var declaration");
 
     info("test autocomplete for 'const win'");
     matches = (await client.autocomplete("const win")).matches;
-    is(matches.length, 0, "no autocompletion on a const declaration");
+    is(matches, null, "no autocompletion on a const declaration");
 
     info("test autocomplete for 'let win'");
     matches = (await client.autocomplete("let win")).matches;
-    is(matches.length, 0, "no autocompletion on a let declaration");
+    is(matches, null, "no autocompletion on a let declaration");
 
     info("test autocomplete for 'function win'");
     matches = (await client.autocomplete("function win")).matches;
-    is(matches.length, 0, "no autocompletion on a function declaration");
+    is(matches, null, "no autocompletion on a function declaration");
 
     info("test autocomplete for 'class win'");
     matches = (await client.autocomplete("class win")).matches;
-    is(matches.length, 0, "no autocompletion on a class declaration");
+    is(matches, null, "no autocompletion on a class declaration");
 
     info("test autocomplete for 'const win = win'");
     matches = (await client.autocomplete("const win = win")).matches;
     ok(matches.includes("window"), "autocompletion still happens after the `=` sign");
 
     info("test autocomplete for 'in var'");
     matches = (await client.autocomplete("in var")).matches;
     ok(matches.includes("varify"),
@@ -560,16 +559,60 @@ async function doKeywordsAutocomplete(cl
     "'function' is not returned when doing a property access");
 
   info("test autocomplete for 'window[func'");
   matches = (await client.autocomplete("window[func")).matches;
   ok(!matches.includes("function"),
     "'function' is not returned when doing an element access");
   }
 
+  async function dontAutocomplete(client) {
+    const inputs = [
+      "",
+      "       ",
+      "\n",
+      "\n  ",
+      "  \n  ",
+      "  \n",
+      "true;",
+      "true,",
+      "({key:",
+      "a=",
+      "if(a<",
+      "if(a>",
+      "1+",
+      "1-",
+      "++",
+      "--",
+      "1*",
+      "2**",
+      "1/",
+      "1%",
+      "1|",
+      "1&",
+      "1^",
+      "~",
+      "1<<",
+      "1>>",
+      "1>>>",
+      "false||",
+      "false&&",
+      "x=true?",
+      "x=false?1:",
+      "!",
+      ...RESERVED_JS_KEYWORDS.map(keyword => `${keyword} `),
+      ...RESERVED_JS_KEYWORDS.map(keyword => `${keyword}  `),
+    ];
+    for (const input of inputs) {
+      info(`test autocomplete for "${input}"`);
+      let matches = (await client.autocomplete(input)).matches;
+      is(matches, null, `No autocomplete result for ${input}"`);
+    }
+  }
+
   async function getAutocompleteMatches(client, input) {
     info(`test autocomplete for "${input}"`);
     const res = (await client.autocomplete(input));
     return res.matches;
   }
 </script>
 </body>
 </html>