Issue 6191 - Update frame structure for Chrome Web Store pages

Issue 6191 - Update frame structure for Chrome Web Store pages

[Manish Jethani, 2017-12-21 13:59:52]

Patch Set 1

The documentation for webRequest.onHeadersReceived [1] states that the event
won't be dispatched for Chrome Web Store. In that case we need to update the
frame structure in webNavigation.onBeforeNavigate similar to non-HTTP(S) URLs.

[1]: https://developer.chrome.com/extensions/webRequest

[kzar, 2018-01-05 16:46:20]

Sorry for being slow with this review as well.

https://codereview.adblockplus.org/29646589/diff/29646590/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29646589/diff/29646590/ext/background.js#n...
ext/background.js:270:
url.href.startsWith("https://chrome.google.com/webstore/"))
I guess we should only do this for Chrome (or Chromium)? I wonder if there
should be a similar exception for Firefox for addons.mozilla.org?

I also worry if checking if the URL starts with that string for every navigation
could slow the extension down, what do you think?

[Manish Jethani, 2018-01-12 07:25:03]

Patch Set 2: Do not create URL object

https://codereview.adblockplus.org/29646589/diff/29646590/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29646589/diff/29646590/ext/background.js#n...
ext/background.js:270:
url.href.startsWith("https://chrome.google.com/webstore/"))
On 2018/01/05 16:46:20, kzar wrote:
> I guess we should only do this for Chrome (or Chromium)?

Ideally yes, but it's probably OK if the frame structure is updated twice
(onBeforeNavigate and onHeadersReceived) on non-Chromium browser only for this
one URL. When we move this code into a module then we can use the info module to
narrow this check.

> I wonder if there
> should be a similar exception for Firefox for addons.mozilla.org?

I checked and apparently it's not required.

> I also worry if checking if the URL starts with that string for every
navigation
> could slow the extension down, what do you think?

I did some benchmarks using this code:

{ 
  let u = [];
  let n = 100000;
  let c = 0;
  for (let i = 0; i < n; i++)
    u.push("https://" + Math.random().toString(36).slice(2));
  let s = performance.now();
  for (let i = 0; i < n; i++)
  {
    let url = new URL(u[i]);
    if (url.protocol != "http:" && url.protocol != "https:" ||
        url.hostname.startsWith("ab"))
    //if (!/^https?:/.test(u[i]) || /^https:\/\/ab/.test(u[i]))
    //if (!u[i].startsWith("http:") && !u[i].startsWith("https:") ||
    //    u[i].startsWith("https://ab"))
      c++;
  }     
  let t = (performance.now() - s) | 0;
  console.log(c + " matches found in " + t + "ms");
}   

Using a URL object is too slow. If we use regular expressions, it's ten times as
fast. It gets even faster if we simply do a string comparison (twice as fast on
Firefox, about ten percent faster on Chrome).

So my most recent change would end up doing the additional comparison for every
single HTTP(S) URL, but at the same time it removes the URL object, which was
also created for every single URL and hence slowed things down. It's a good
bargain, I think.

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js
File ext/background.js (left):

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js#o...
ext/background.js:266: let url = new URL(details.url);
There's no need to create a URL object here, it's too slow.

[Manish Jethani, 2018-01-12 07:31:58]

https://codereview.adblockplus.org/29646589/diff/29646590/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29646589/diff/29646590/ext/background.js#n...
ext/background.js:270:
url.href.startsWith("https://chrome.google.com/webstore/"))
On 2018/01/12 07:25:03, Manish Jethani wrote:
> So my most recent change would end up doing the additional comparison for
every
> single HTTP(S) URL, but at the same time it removes the URL object, which was
> also created for every single URL and hence slowed things down. It's a good
> bargain, I think.

By the way, if it wasn't for Edge, we could have used the additional "filter"
option [1] which I expect would be faster.

[1]:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webNavigation/o...

[Manish Jethani, 2018-01-12 07:32:59]

Also, looking at the Chrome Web Store, it's just a web app, so all the checks we
normally do in onHeadersReceived are not really relevant from what I can tell.
i.e. it's OK that we're skipping those checks (since we don't have the headers)
for the Chrome Web Store. We can assume that onBeforeNavigate will lead to
onCommitted for these URLs.

[kzar, 2018-01-12 12:04:12]

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js
File ext/background.js (left):

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js#o...
ext/background.js:266: let url = new URL(details.url);
On 2018/01/12 07:25:03, Manish Jethani wrote:
> There's no need to create a URL object here, it's too slow.

Acknowledged.

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js#n...
ext/background.js:267: if (!url.startsWith("https:") && !url.startsWith("http:")
||
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:16]

Patch Set 3: Modify check

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js#n...
ext/background.js:267: if (!url.startsWith("https:") && !url.startsWith("http:")
||
On 2018/01/12 12:04:11, kzar wrote:
> 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/"))

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]

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js#n...
ext/background.js:267: if (!url.startsWith("https:") && !url.startsWith("http:")
||
On 2018/01/14 14:46:15, Manish Jethani wrote:
> On 2018/01/12 12:04:11, kzar wrote:
> > 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/"))
> 
> 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?

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]

Patch Set 4: Combine checks

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29646589/diff/29664555/ext/background.js#n...
ext/background.js:267: if (!url.startsWith("https:") && !url.startsWith("http:")
||
On 2018/01/15 10:31:12, kzar wrote:
> On 2018/01/14 14:46:15, Manish Jethani wrote:
> > On 2018/01/12 12:04:11, kzar wrote:
> > > 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/"))
> > 
> > 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?
> 
> 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/"))

Done.

[kzar, 2018-01-15 12:35:32]

LGTM