Merge branch 'master' of github.com:philc/vimium into feature/vomnibar-results-favicon-size-setting

Author: abelfubu

Diff for: background_scripts/completion.js

@@ -273,10 +273,10 @@ class BookmarkCompleter {
     if (queryTerms.length > 0) {
       results = this.bookmarks.filter((bookmark) => {
         const suggestionTitle = usePathAndTitle ? bookmark.pathAndTitle : bookmark.title;
-        if (bookmark.hasJavascriptPrefix == null) {
-          bookmark.hasJavascriptPrefix = Utils.hasJavascriptPrefix(bookmark.url);
+        if (bookmark.hasJavascriptProtocol == null) {
+          bookmark.hasJavascriptProtocol = UrlUtils.hasJavascriptProtocol(bookmark.url);
         }
-        if (bookmark.hasJavascriptPrefix && bookmark.shortUrl == null) {
+        if (bookmark.hasJavascriptProtocol && bookmark.shortUrl == null) {
           bookmark.shortUrl = "javascript:...";
         }
         const suggestionUrl = bookmark.shortUrl != null ? bookmark.shortUrl : bookmark.url;
@@ -477,7 +477,7 @@ class DomainCompleter {
 
   // Return something like "http://www.example.com" or false.
   parseDomainAndScheme(url) {
-    return UrlUtils.hasFullUrlPrefix(url) && !UrlUtils.hasChromePrefix(url) &&
+    return UrlUtils.urlHasProtocol(url) && !UrlUtils.hasChromeProtocol(url) &&
       url.split("/", 3).join("/");
   }
 }

Diff for: background_scripts/completion_engines.js

@@ -40,7 +40,7 @@ class BaseEngine {
     return Utils.matchesAnyRegexp(this.regexps, searchUrl);
   }
   getUrl(queryTerms) {
-    return UrlUtils.createSearchUrl(queryTerms, this.engineUrl);
+    return UrlUtils.createSearchUrl(queryTerms.join(" "), this.engineUrl);
   }
 }
 

Diff for: background_scripts/completion_search.js

@@ -99,7 +99,7 @@ const CompletionSearch = {
 
     // We don't complete regular URLs or Javascript URLs.
     if (queryTerms.length == 1 && await UrlUtils.isUrl(query)) return [];
-    if (UrlUtils.hasJavascriptPrefix(query)) return [];
+    if (UrlUtils.hasJavascriptProtocol(query)) return [];
 
     const engine = this.lookupEngine(searchUrl);
     if (!engine) return [];

Diff for: background_scripts/tab_operations.js

@@ -11,7 +11,7 @@ async function openUrlInCurrentTab(request) {
   // Note that when injecting JavaScript, it's subject to the site's CSP. Sites with strict CSPs
   // (like github.com, developer.mozilla.org) will raise an error when we try to run this code. See
   // https://github.com/philc/vimium/issues/4331.
-  if (UrlUtils.hasJavascriptPrefix(request.url)) {
+  if (UrlUtils.hasJavascriptProtocol(request.url)) {
     const scriptingArgs = {
       target: { tabId: request.tabId },
       func: (text) => {

Diff for: lib/chrome_api_stubs.js

@@ -37,6 +37,9 @@ if (shouldInstallStubs) {
         addListener() {},
       },
       sendMessage(message) {
+        // TODO(philc): This stub should return a an empty Promise, not the length of the
+        // chromeMessages array. Some portion fo the dom_tests.html setup depends on this value, so
+        // the tests break. Fix.
         return chromeMessages.unshift(message);
       },
       getManifest() {

Diff for: lib/url_utils.js

@@ -40,7 +40,7 @@ const UrlUtils = {
     if (str.includes(" ")) return false;
 
     // Starts with a scheme: URL
-    if (this.hasFullUrlPrefix(str)) return true;
+    if (this.urlHasProtocol(str)) return true;
 
     // More or less RFC compliant URL host part parsing. This should be sufficient for our needs
     const urlRegex = new RegExp(
@@ -91,40 +91,31 @@ const UrlUtils = {
     string = string.trim();
 
     // Special-case about:[url], view-source:[url] and the like
-    if (this.hasChromePrefix(string)) {
+    if (this.hasChromeProtocol(string)) {
       return string;
-    } else if (this.hasJavascriptPrefix(string)) {
+    } else if (this.hasJavascriptProtocol(string)) {
       return string;
     } else if (await this.isUrl(string)) {
-      return this.createFullUrl(string);
+      return this.urlHasProtocol(string) ? string : `http://${string}`;
     } else {
       const message = `convertToUrl: can't convert "${string}" into a URL. This shouldn't happen.`;
       console.error(message);
       return null;
     }
   },
 
-  hasChromePrefix: (function () {
-    const chromePrefixes = ["about:", "view-source:", "extension:", "chrome-extension:", "data:"];
-    return (url) => chromePrefixes.some((prefix) => url.startsWith(prefix));
-  })(),
+  _chromePrefixes: ["about:", "view-source:", "extension:", "chrome-extension:", "data:"],
+  hasChromeProtocol(url) {
+    return this._chromePrefixes.some((prefix) => url.startsWith(prefix));
+  },
 
-  hasJavascriptPrefix(url) {
+  hasJavascriptProtocol(url) {
     return url.startsWith("javascript:");
   },
 
-  hasFullUrlPrefix: (function () {
-    const urlPrefix = new RegExp("^[a-z][-+.a-z0-9]{2,}://.");
-    return (url) => urlPrefix.test(url);
-  })(),
-
-  // Completes a partial URL (without scheme)
-  createFullUrl(partialUrl) {
-    if (this.hasFullUrlPrefix(partialUrl)) {
-      return partialUrl;
-    } else {
-      return ("http://" + partialUrl);
-    }
+  _urlPrefix: new RegExp("^[a-z][-+.a-z0-9]{2,}://."),
+  urlHasProtocol(url) {
+    return this._urlPrefix.test(url);
   },
 
   // Create a search URL from the given :query using the provided search URL.
@@ -133,16 +124,11 @@ const UrlUtils = {
       searchUrl += "%s";
     }
     searchUrl = searchUrl.replace(/%S/g, query);
-    return searchUrl.replace(/%s/g, this.createSearchQuery(query));
-  },
 
-  // Map a search query to its URL encoded form. The query may be either a string or an array of
-  // strings. E.g. "BBC Sport" -> "BBC%20Sport".
-  createSearchQuery(query) {
-    if (typeof query === "string") {
-      query = query.split(/\s+/);
-    }
-    return query.map(encodeURIComponent).join("%20");
+    // Map a search query to its URL encoded form. E.g. "BBC Sport" -> "BBC%20Sport".
+    const parts = query.split(/\s+/);
+    const encodedQuery = parts.map(encodeURIComponent).join("%20");
+    return searchUrl.replace(/%s/g, encodedQuery);
   },
 };
 

Diff for: lib/utils.js

@@ -78,11 +78,6 @@ const Utils = {
     return () => id += 1;
   })(),
 
-  // TODO(philc): Move to UrlUrls?
-  hasJavascriptPrefix(url) {
-    return url.startsWith("javascript:");
-  },
-
   // Decode valid escape sequences in a URI. This is intended to mimic the best-effort decoding
   // Chrome itself seems to apply when a Javascript URI is enetered into the omnibox (or clicked).
   // See https://code.google.com/p/chromium/issues/detail?id=483000, #1611 and #1636.
@@ -419,7 +414,7 @@ const UserSearchEngines = {
       const keyword = tokens[0].split(":")[0];
       const url = tokens[1];
       const description = tokens.length > 2 ? tokens.slice(2).join(" ") : `search (${keyword})`;
-      if (UrlUtils.hasFullUrlPrefix(url) || UrlUtils.hasJavascriptPrefix(url)) {
+      if (UrlUtils.urlHasProtocol(url) || UrlUtils.hasJavascriptProtocol(url)) {
         results[keyword] = new UserSearchEngine({ keyword, url, description });
       }
     }

Diff for: make.js

@@ -65,12 +65,12 @@ function createFirefoxManifest(manifest) {
   // Firefox supports SVG icons.
   Object.assign(manifest, {
     "icons": {
-        "16": "icons/icon.svg",
-        "32": "icons/icon.svg",
-        "48": "icons/icon.svg",
-        "64": "icons/icon.svg",
-        "96": "icons/icon.svg",
-        "128": "icons/icon.svg"
+      "16": "icons/icon.svg",
+      "32": "icons/icon.svg",
+      "48": "icons/icon.svg",
+      "64": "icons/icon.svg",
+      "96": "icons/icon.svg",
+      "128": "icons/icon.svg",
     },
   });
 
@@ -130,7 +130,10 @@ async function buildStorePackage() {
   const firefoxManifest = createFirefoxManifest(chromeManifest);
   await writeDistManifest(firefoxManifest);
   // Exclude PNG icons from the Firefox build, because we use the SVG directly.
-  await shell("bash", ["-c", `${zipCommand} ../firefox/vimium-firefox-${version}.zip . -x icons/*.png`]);
+  await shell("bash", [
+    "-c",
+    `${zipCommand} ../firefox/vimium-firefox-${version}.zip . -x icons/*.png`,
+  ]);
 
   // Build the Chrome Store package.
   await writeDistManifest(chromeManifest);
@@ -163,49 +166,55 @@ const runUnitTests = async () => {
   return await shoulda.run();
 };
 
-const runDomTests = async (port) => {
-  const testUrl = `http://localhost:${port}/tests/dom_tests/dom_tests.html`;
-
-  const browser = await puppeteer.launch();
-  const page = await browser.newPage();
-  let receivedErrorOutput = false;
-
+function setupPuppeteerPageForTests(page) {
+  // The "console" event emitted has arguments which are promises. To obtain the values to be
+  // printed, we must resolve those promises. However, if many console messages are emitted at once,
+  // resolving the promises often causes the console.log messages to be printed out of order. Here,
+  // we use a queue to strictly enforce that the messages appear in the order in which they were
+  // logged.
+  const messageQueue = [];
+  let processing = false;
+  const processMessageQueue = async () => {
+    while (messageQueue.length > 0) {
+      const values = await Promise.all(messageQueue.shift());
+      console.log(...values);
+    }
+    processing = false;
+  };
   page.on("console", async (msg) => {
-    const args = await Promise.all(msg.args().map((a) => a.jsonValue()));
-    console.log(...args);
+    const values = msg.args().map((a) => a.jsonValue());
+    messageQueue.push(values);
+    if (!processing) {
+      processing = true;
+      processMessageQueue();
+    }
   });
+
   page.on("error", (err) => {
-    // As far as I can tell, this handler never gets executed.
+    // NOTE(philc): As far as I can tell, this handler never gets executed.
     console.error(err);
   });
   // pageerror catches the same events that window.onerror would, like JavaScript parsing errors.
   page.on("pageerror", (error) => {
-    receivedErrorOutput = true;
+    // This is an arbitrary field we're writing to the page object.
+    page.receivedErrorOutput = true;
     // Whatever type error is, it requires toString() to print the message.
     console.log(error.toString());
   });
-  page.on(
-    "requestfailed",
-    (request) => console.log(console.log(`${request.failure().errorText} ${request.url()}`)),
-  );
-
-  page.goto(testUrl);
+  page.on("requestfailed", (request) => {
+    console.log(`${request.failure().errorText} ${request.url()}`);
+  });
+}
 
+// Navigates the Puppeteer `page` to `url` and invokes shoulda.run().
+async function runPuppeteerTest(page, url) {
+  page.goto(url);
   await page.waitForNavigation({ waitUntil: "load" });
-
   const success = await page.evaluate(async () => {
     return await shoulda.run();
   });
-
-  // NOTE(philc): At one point in development, I noticed that the output from Deno would suddenly
-  // pause, prior to the tests fully finishing, so closing the browser here may be racy. If it
-  // occurs again, we may need to add "await delay(200)".
-  await browser.close();
-  if (receivedErrorOutput) {
-    throw new Error("The tests fail because there was a page-level error.");
-  }
   return success;
-};
+}
 
 desc("Download and parse list of top-level domains (TLDs)");
 task("fetch-tlds", [], async () => {
@@ -228,8 +237,7 @@ task("test-unit", [], async () => {
   }
 });
 
-desc("Run DOM tests");
-task("test-dom", [], async () => {
+async function testDom() {
   const port = await getAvailablePort();
   let served404 = false;
   const httpServer = Deno.serve({ port }, async (req) => {
@@ -247,15 +255,41 @@ task("test-dom", [], async () => {
     }
   });
 
-  const success = await runDomTests(port);
-  if (served404) {
-    console.log("Tests failed because a background or content script requested a missing file.");
+  const files = ["dom_tests.html", "vomnibar_test.html"];
+  const browser = await puppeteer.launch();
+  let success = true;
+  for (const file of files) {
+    const page = await browser.newPage();
+    console.log("Running", file);
+    setupPuppeteerPageForTests(page);
+    const url = `http://localhost:${port}/tests/dom_tests/${file}?dom_tests=true`;
+    const result = await runPuppeteerTest(page, url);
+    success = success && result;
+    if (served404) {
+      console.log(`${file} failed: a background or content script requested a missing file.`);
+    }
+    if (page.receivedErrorOutput) {
+      console.log(`${file} failed: there was a page level error.`);
+      success = false;
+    }
+    // If we close the puppeteer page (tab) via page.close(), we can get innocuous but noisy output
+    // like this:
+    // net::ERR_ABORTED http://localhost:43524/pages/hud.html?dom_tests=true
+    // There's probably a way to prevent that, but as a work around, we avoid closing the page.
+    // browser.close() will close all of its owned pages.
   }
+  // NOTE(philc): At one point in development, I noticed that the output from Deno would suddenly
+  // pause, prior to the tests fully finishing, so closing the browser here may be racy. If it
+  // occurs again, we may need to add "await delay(200)".
+  await browser.close();
   await httpServer.shutdown();
   if (served404 || !success) {
     abort("test-dom failed.");
   }
-});
+}
+
+desc("Run DOM tests");
+task("test-dom", [], testDom);
 
 desc("Run unit and DOM tests");
 task("test", ["test-unit", "test-dom"]);

Diff for: pages/vomnibar.html

@@ -3,14 +3,7 @@
   <head>
     <title>Vomnibar</title>
     <meta name="color-scheme" content="light dark">
-    <script src="../lib/utils.js"></script>
-    <script src="../lib/url_utils.js"></script>
-    <script src="../lib/settings.js"></script>
-    <script src="../lib/keyboard_utils.js"></script>
-    <script src="../lib/dom_utils.js"></script>
-    <script src="../lib/handler_stack.js"></script>
-    <script src="ui_component_server.js"></script>
-    <script src="vomnibar.js"></script>
+    <script type="module" src="vomnibar.js"></script>
     <link rel="stylesheet" type="text/css" href="../content_scripts/vimium.css" />
     <link rel="stylesheet" type="text/css" href="vomnibar.css" />
   </head>