[extensions] Remove unnecessary checks in IsSensitiveURL

[extensions] Remove unnecessary checks in IsSensitiveURL

The current code re-parses the url by removing the query and ref components
before sending the modified URL to IsWebstoreUpdateUrl. This is not
necessary, as IsWebstoreUpdateUrl will return true if the host and path
are identical.

Creating the modified URL takes ~3-4% of the total task time on the IOthread
when loading http://cr.kungfoo.net/loading/1000spacers/

This CL also cleans up checks in the IsSensitiveURL function.

BUG=664174, 664285
Committed: https://crrev.com/b8ec6582c1c86524b3cb47dbf65605213d35930e
Cr-Commit-Position: refs/heads/master@{#431432}

[Charlie Harrison, 2016-11-10 19:32:14]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 19:32:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-11-10 20:02:22]

csharrison@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Charlie Harrison, 2016-11-10 20:02:23]

Devlin, would you mind taking a look at this patch? I feel like removing
unnecessary work is good, but having IsWebstoreUpdateUrl only check the host and
path seems weird.

Note that GetWebstoreUpdateUrl() also takes about an equal amount of work, but
would be a bit harder to refactor away. I may be interested in tackling it in a
follow up, as we really shouldn't be re-parsing constant GURLs at every step in
the resource loader pipeline.

[commit-bot: I haz the power, 2016-11-10 20:10:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-10 20:10:42]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-10 20:57:15]

Your code's all good, but let's make the surrounding code even better! :)

https://codereview.chromium.org/2489233003/diff/1/extensions/browser/api/web_...
File extensions/browser/api/web_request/web_request_permissions.cc (right):

https://codereview.chromium.org/2489233003/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_request/web_request_permissions.cc:33: const
std::string host = url.host();
can this be a StringPiece?

https://codereview.chromium.org/2489233003/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_request/web_request_permissions.cc:36: if
(base::EndsWith(host, kGoogleCom, base::CompareCase::SENSITIVE)) {
Can we use url.DomainIs(kGoogleCom)?  I know it would work for
DomainIs("google.com"), and I *think* it does for ".google.com", but it'd be
worth double-checking.

https://codereview.chromium.org/2489233003/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_request/web_request_permissions.cc:56:
base::EndsWith(url.host(), ".clients.google.com",
ditto, DomainIs?

https://codereview.chromium.org/2489233003/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_request/web_request_permissions.cc:58: url.host() ==
"sb-ssl.google.com" ||
host_piece() here and below

[Charlie Harrison, 2016-11-10 22:03:53]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 22:04:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-11-10 22:08:25]

Description was changed from

==========
[extensions] Remove unnecessary checks in IsSensitiveURL

The current code re-parses the url by removing the query and ref components
before sending the modified URL to IsWebstoreUpdateUrl. This is not
necessary, as IsWebstoreUpdateUrl will return true if the host and path
are identical.

Creating the modified URL takes ~3-4% of the total task time on the IOthread
when loading http://cr.kungfoo.net/loading/1000spacers/

BUG=664174
==========

to

==========
[extensions] Remove unnecessary checks in IsSensitiveURL

The current code re-parses the url by removing the query and ref components
before sending the modified URL to IsWebstoreUpdateUrl. This is not
necessary, as IsWebstoreUpdateUrl will return true if the host and path
are identical.

Creating the modified URL takes ~3-4% of the total task time on the IOthread
when loading http://cr.kungfoo.net/loading/1000spacers/

BUG=664174,664285
==========

[Charlie Harrison, 2016-11-10 22:09:24]

Description was changed from

==========
[extensions] Remove unnecessary checks in IsSensitiveURL

The current code re-parses the url by removing the query and ref components
before sending the modified URL to IsWebstoreUpdateUrl. This is not
necessary, as IsWebstoreUpdateUrl will return true if the host and path
are identical.

Creating the modified URL takes ~3-4% of the total task time on the IOthread
when loading http://cr.kungfoo.net/loading/1000spacers/

BUG=664174,664285
==========

to

==========
[extensions] Remove unnecessary checks in IsSensitiveURL

The current code re-parses the url by removing the query and ref components
before sending the modified URL to IsWebstoreUpdateUrl. This is not
necessary, as IsWebstoreUpdateUrl will return true if the host and path
are identical.

Creating the modified URL takes ~3-4% of the total task time on the IOthread
when loading http://cr.kungfoo.net/loading/1000spacers/

This CL also cleans up checks in the IsSensitiveURL function.

BUG=664174,664285
==========

[Charlie Harrison, 2016-11-10 22:10:20]

PTAL. My main concern is with changing the test extension client method, and
whether or not you care about exposing the bare function for unit testing.

https://codereview.chromium.org/2489233003/diff/1/extensions/browser/api/web_...
File extensions/browser/api/web_request/web_request_permissions.cc (right):

https://codereview.chromium.org/2489233003/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_request/web_request_permissions.cc:36: if
(base::EndsWith(host, kGoogleCom, base::CompareCase::SENSITIVE)) {
On 2016/11/10 20:57:14, Devlin (slow) wrote:
> Can we use url.DomainIs(kGoogleCom)?  I know it would work for
> DomainIs("google.com"), and I *think* it does for ".google.com", but it'd be
> worth double-checking.

I think we should just make kGoogleCom = "google.com"

https://codereview.chromium.org/2489233003/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_request/web_request_permissions.cc:56:
base::EndsWith(url.host(), ".clients.google.com",
On 2016/11/10 20:57:15, Devlin (slow) wrote:
> ditto, DomainIs?

Done.

https://codereview.chromium.org/2489233003/diff/1/extensions/browser/api/web_...
extensions/browser/api/web_request/web_request_permissions.cc:58: url.host() ==
"sb-ssl.google.com" ||
On 2016/11/10 20:57:15, Devlin (slow) wrote:
> host_piece() here and below

Done.

https://codereview.chromium.org/2489233003/diff/20001/extensions/test/test_ex...
File extensions/test/test_extensions_client.cc (right):

https://codereview.chromium.org/2489233003/diff/20001/extensions/test/test_ex...
extensions/test/test_extensions_client.cc:159: return false;
unit tests pass with this but I'm not sure if browser tests will. Does this seem
safe to you or should I find another way to test this function?

[Devlin, 2016-11-10 22:26:40]

lgtm

https://codereview.chromium.org/2489233003/diff/20001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_permissions.cc (right):

https://codereview.chromium.org/2489233003/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_permissions.cc:72:
url.host_piece() == "sb-ssl.google.com" ||
hmm... maybe also just DomainIs() for these.  It would mean that we filter out
foo.sb-ssl.google.com and foo.chrome.google.com/webstore, but I think that's
probably fine.  WDYT?  Otherwise, we need change the "==" to
"base::StartsWith(), or similar.

https://codereview.chromium.org/2489233003/diff/20001/extensions/test/test_ex...
File extensions/test/test_extensions_client.cc (right):

https://codereview.chromium.org/2489233003/diff/20001/extensions/test/test_ex...
extensions/test/test_extensions_client.cc:159: return false;
On 2016/11/10 22:10:20, Charlie Harrison wrote:
> unit tests pass with this but I'm not sure if browser tests will. Does this
seem
> safe to you or should I find another way to test this function?

I think this should be fine.  Not sure why the default would be true.

[Charlie Harrison, 2016-11-10 22:35:36]

Thanks!

https://codereview.chromium.org/2489233003/diff/20001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_permissions.cc (right):

https://codereview.chromium.org/2489233003/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_permissions.cc:72:
url.host_piece() == "sb-ssl.google.com" ||
On 2016/11/10 22:26:39, Devlin (slow) wrote:
> hmm... maybe also just DomainIs() for these.  It would mean that we filter out
> http://foo.sb-ssl.google.com and foo.chrome.google.com/webstore, but I think
that's
> probably fine.  WDYT?  Otherwise, we need change the "==" to
> "base::StartsWith(), or similar.

SGTM. Done.

[Charlie Harrison, 2016-11-10 22:36:30]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-11-10 22:36:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2489233003/#ps40001
(title: "more DomainIs")

[commit-bot: I haz the power, 2016-11-10 22:36:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 23:18:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-10 23:18:59]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Charlie Harrison, 2016-11-11 00:49:48]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-11-11 00:50:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 01:05:40]

Description was changed from

==========
[extensions] Remove unnecessary checks in IsSensitiveURL

The current code re-parses the url by removing the query and ref components
before sending the modified URL to IsWebstoreUpdateUrl. This is not
necessary, as IsWebstoreUpdateUrl will return true if the host and path
are identical.

Creating the modified URL takes ~3-4% of the total task time on the IOthread
when loading http://cr.kungfoo.net/loading/1000spacers/

This CL also cleans up checks in the IsSensitiveURL function.

BUG=664174,664285
==========

to

==========
[extensions] Remove unnecessary checks in IsSensitiveURL

The current code re-parses the url by removing the query and ref components
before sending the modified URL to IsWebstoreUpdateUrl. This is not
necessary, as IsWebstoreUpdateUrl will return true if the host and path
are identical.

Creating the modified URL takes ~3-4% of the total task time on the IOthread
when loading http://cr.kungfoo.net/loading/1000spacers/

This CL also cleans up checks in the IsSensitiveURL function.

BUG=664174,664285
==========

[commit-bot: I haz the power, 2016-11-11 01:05:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-11 01:10:14]

Description was changed from

==========
[extensions] Remove unnecessary checks in IsSensitiveURL

The current code re-parses the url by removing the query and ref components
before sending the modified URL to IsWebstoreUpdateUrl. This is not
necessary, as IsWebstoreUpdateUrl will return true if the host and path
are identical.

Creating the modified URL takes ~3-4% of the total task time on the IOthread
when loading http://cr.kungfoo.net/loading/1000spacers/

This CL also cleans up checks in the IsSensitiveURL function.

BUG=664174,664285
==========

to

==========
[extensions] Remove unnecessary checks in IsSensitiveURL

The current code re-parses the url by removing the query and ref components
before sending the modified URL to IsWebstoreUpdateUrl. This is not
necessary, as IsWebstoreUpdateUrl will return true if the host and path
are identical.

Creating the modified URL takes ~3-4% of the total task time on the IOthread
when loading http://cr.kungfoo.net/loading/1000spacers/

This CL also cleans up checks in the IsSensitiveURL function.

BUG=664174,664285

Committed: https://crrev.com/b8ec6582c1c86524b3cb47dbf65605213d35930e
Cr-Commit-Position: refs/heads/master@{#431432}
==========

[commit-bot: I haz the power, 2016-11-11 01:10:15]

Patchset 3 (id:??) landed as
https://crrev.com/b8ec6582c1c86524b3cb47dbf65605213d35930e
Cr-Commit-Position: refs/heads/master@{#431432}