Issue 6191 - Update frame structure for Chrome Web Store pages

ext/background.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 246 matching lines @@
   },
   {types: ["main_frame", "sub_frame"], urls: ["http://*/*", "https://*/*"]},
   ["responseHeaders"]);
 
   browser.webNavigation.onBeforeNavigate.addListener(details =>
   {
     // Since we can only listen for HTTP(S) responses using
     // webRequest.onHeadersReceived we must update the page structure here for
     // other navigations.
     let {url} = details;
-    if (!url.startsWith("https:") && !url.startsWith("http:") ||

[kzar, 2018/01/12 12:04:11]

I like this new logic, but it seems kind of a shame we will often check for the
"http" prefix multiple times. We could avoid that (untested) but I don't know if
it's worth it. What do you think?

// We can only listen for HTTP(S) responses using webRequest.onHeadersReceived,
// so we must update the page structure here for other navigations. Note that
// event doesn't fire for responses from the Chrome web store either in Chrome,
// so we must update the page structure for Chrome web store responses here too.
if (url.length < 6 || !url.startsWith("http") ||
    !(url[4] == ":" || url[4] == "s" && url[5] == ":" &&
      url.substr(6) != "//chrome.google.com/webstore/"))

[Manish Jethani, 2018/01/14 14:46:15]

I like this sort of optimization in general, but in this particular case it
doesn't seem to help. I tried the following in both Chrome and Firefox:

{
  let u = [];
  let n = 10000000;
  let c = 0;
  for (let i = 0; i < n; i++) {
    if (i % 1000 == 0)
      u.push("https://chrome.google.com/webstore/apps");
    else
      u.push("https://" + Math.random().toString(36).slice(2));
  }
  let s = performance.now();
  for (let i = 0; i < n; i++)
  {
    //if (!u[i].startsWith("http:") &&
    //    (!u[i].startsWith("https:") ||
    //     u[i].startsWith("https://chrome.google.com/webstore/")))
    if (u[i].length < 8 || !u[i].startsWith("http") ||
        (u[i][4] != ":" &&
         (u[i][4] != "s" || u[i][5] != ":" ||
          u[i].substr(8, 27) == "chrome.google.com/webstore/")))
      c++;
  }
  let t = (performance.now() - s) | 0;
  console.log(c + " matches found in " + t + "ms");
}

On Chrome the first check is slightly faster, whereas on Firefox it reduces the
execution time by almost 40%.

So given that the first one is also easier to read, I'm in favor of going with
that one.

What do you think?

[kzar, 2018/01/15 10:31:12]

Agreed, but maybe at least combine the checks? (untested)

// We can only listen for HTTP(S) responses using webRequest.onHeadersReceived,
// so we must update the page structure here for other navigations. Note that
// event doesn't fire for responses from the Chrome web store either in Chrome,
// so we must update the page structure for Chrome web store responses here too.
if (!(url.startsWith("http:") || urlstartsWith("https:") &&
      !url.startsWith("https://chrome.google.com/webstore/"))

[Manish Jethani, 2018/01/15 12:08:34]

Done.

-        // Chrome doesn't dispatch webRequest.onHeadersReceived
-        // for Web Store URLs.
-        url.startsWith("https://chrome.google.com/webstore/"))
+    if (!(url.startsWith("http:") ||
+          url.startsWith("https:") &&
+          // Chrome doesn't dispatch webRequest.onHeadersReceived
+          // for Web Store URLs.
+          !url.startsWith("https://chrome.google.com/webstore/")))
     {
       updatePageFrameStructure(details.frameId, details.tabId, url,
                                details.parentFrameId);
     }
   });
 
   function forgetTab(tabId)
   {
     ext.pages.onRemoved._dispatch(tabId);
 
@@ skipping 408 matching lines @@
   ext.windows = {
     create(createData, callback)
     {
       browser.windows.create(createData, createdWindow =>
       {
         afterTabLoaded(callback)(createdWindow.tabs[0]);
       });
     }
   };
 }