Avoid parsing the webstore base url so much.

Avoid parsing the webstore base url so much.

This is a followup to [1] which modified the extension url API to vend
GURLs rather than std::string for webstore update urls.

This patch does the same for the webstore base url
(the webstore "launch" url). We only parse this url in
ChromeResourceDispatcherHostDelegate::OnResponseStarted so there is less
of a performance win than [1], but still we should avoid doing needless
work.

Parsing this URL takes ~12% of the CPU time of
ResourceLoader::CompleteResponseStarted.

[1] https://codereview.chromium.org/2493053002/

BUG=664174
Committed: https://crrev.com/5d1f214db0f7996f3c17cd87093d439ce4c7f8f1
Cr-Commit-Position: refs/heads/master@{#433963}

[Charlie Harrison, 2016-11-15 21:41:59]

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

[commit-bot: I haz the power, 2016-11-15 21:42:45]

Dry run: 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-15 22:01:20]

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Harrison, 2016-11-15 23:17:50]

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

[commit-bot: I haz the power, 2016-11-15 23:18:50]

Dry run: 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-15 23:36:31]

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

[commit-bot: I haz the power, 2016-11-15 23:36:31]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Harrison, 2016-11-16 01:31:13]

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

[commit-bot: I haz the power, 2016-11-16 01:32:28]

Dry run: 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-16 02:20:03]

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

[commit-bot: I haz the power, 2016-11-16 02:20:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2016-11-16 18:28:05]

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

[commit-bot: I haz the power, 2016-11-16 18:28:47]

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-16 18:43:32]

Description was changed from

==========
Avoid parsing the webstore base url so much.

This is a followup to [1] which modified the extension url API to vend
GURLs rather than std::string for webstore update urls.

This patch does the same for the webstore base url
(the webstore "launch" url). We only parse this url in
ChromeResourceDispatcherHostDelegate::OnResponseStarted so there is less
of a performance win than [1], but still we should avoid doing needless
work.

Parsing this URL takes ~12% of the CPU time of
ResourceLoader::CompleteResponseStarted.

[1] https://codereview.chromium.org/2493053002/

BUG=
==========

to

==========
Avoid parsing the webstore base url so much.

This is a followup to [1] which modified the extension url API to vend
GURLs rather than std::string for webstore update urls.

This patch does the same for the webstore base url
(the webstore "launch" url). We only parse this url in
ChromeResourceDispatcherHostDelegate::OnResponseStarted so there is less
of a performance win than [1], but still we should avoid doing needless
work.

Parsing this URL takes ~12% of the CPU time of
ResourceLoader::CompleteResponseStarted.

[1] https://codereview.chromium.org/2493053002/

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

[Charlie Harrison, 2016-11-16 18:50:51]

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

[Charlie Harrison, 2016-11-16 18:50:52]

Devlin, here's the followup to the webstore update url optimization. Review sent
to you because you have the relevant context, but feel free to punt to another
reviewer because I've been asking you for lots of reviews recently.

Note there was some odd logic to strip the "/" from the end of the command line
base URL. I couldn't tell why this was necessary via code archaeology and it
doesn't seem needed.

Also, operator==(const GURL&, const StringPiece&) now DCHECKs that the piece is
canonicalized. So I've converted those checks to use possibly_invalid_spec().

[Devlin, 2016-11-16 19:11:52]

On 2016/11/16 18:50:52, Charlie Harrison wrote:
> Devlin, here's the followup to the webstore update url optimization. Review
sent
> to you because you have the relevant context, but feel free to punt to another
> reviewer because I've been asking you for lots of reviews recently.

I don't mind :)  And given the amount of skeletons we've found here, I'm happy
to keep looking at them!

https://codereview.chromium.org/2506713002/diff/60001/chrome/common/extension...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/2506713002/diff/60001/chrome/common/extension...
chrome/common/extensions/chrome_extensions_client.cc:281: } else if
(webstore_base_url_.is_empty()) {
Is the savings from lazily populating webstore_base_url_ worth the cost of the
constant if-branch here?  I'd think we could just instantiate it at construction
of the client.

https://codereview.chromium.org/2506713002/diff/60001/extensions/common/exten...
File extensions/common/extension_urls.cc (right):

https://codereview.chromium.org/2506713002/diff/60001/extensions/common/exten...
extensions/common/extension_urls.cc:44: return GetWebstoreLaunchURL().spec() +
"/category/extensions";
Can we a) Add a TODO to convert these to return GURL and b) make this
GetWebstoreLaunchUrl().Resolve("category/extensions").spec() (and same for
GetWebstoreItemDetailURLPrefix())?

[commit-bot: I haz the power, 2016-11-16 19:53:56]

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

[commit-bot: I haz the power, 2016-11-16 19:53:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2016-11-16 21:39:54]

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

[commit-bot: I haz the power, 2016-11-16 21:40:58]

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-16 21:45:19]

Thanks!

https://codereview.chromium.org/2506713002/diff/60001/chrome/common/extension...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/2506713002/diff/60001/chrome/common/extension...
chrome/common/extensions/chrome_extensions_client.cc:281: } else if
(webstore_base_url_.is_empty()) {
On 2016/11/16 19:11:52, Devlin wrote:
> Is the savings from lazily populating webstore_base_url_ worth the cost of the
> constant if-branch here?  I'd think we could just instantiate it at
construction
> of the client.

That's a better solution. I don't think lazy initialization wins us much. I'll
move the update url to this pattern too.

These are moved to Initalize().

https://codereview.chromium.org/2506713002/diff/60001/extensions/common/exten...
File extensions/common/extension_urls.cc (right):

https://codereview.chromium.org/2506713002/diff/60001/extensions/common/exten...
extensions/common/extension_urls.cc:44: return GetWebstoreLaunchURL().spec() +
"/category/extensions";
On 2016/11/16 19:11:52, Devlin wrote:
> Can we a) Add a TODO to convert these to return GURL and b) make this
> GetWebstoreLaunchUrl().Resolve("category/extensions").spec() (and same for
> GetWebstoreItemDetailURLPrefix())?

Done, though using Resolve() is more expensive, because it does
canonicalization. I don't think these are bottlenecks though, so it's fine I
think.

[Devlin, 2016-11-16 22:07:53]

lgtm

[commit-bot: I haz the power, 2016-11-16 22:36:18]

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

[commit-bot: I haz the power, 2016-11-16 22:36:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2016-11-21 01:21:24]

https://codereview.chromium.org/2506713002/diff/80001/extensions/common/exten...
File extensions/common/extension_urls.cc (right):

https://codereview.chromium.org/2506713002/diff/80001/extensions/common/exten...
extensions/common/extension_urls.cc:47: return
GetWebstoreLaunchURL().Resolve("/category/extensions").spec();
Using Resolve() here and below is causing subtle errors with the tests. Fixing
the root causes is becoming troublesome.

The problem is that Resolving with relative URL with a leading "/" means we
consider it an absolute URL, so we remove the existing path component of the
launch url.

To resolve right, we also need the launch URL (with a path) to end with a "/".
Unfortunately for some tests (e.g. WebstoreStartupInstallUnpackFailureTest),
this is a bit tricky to do, as test infrastructure relies in a few places on it
not having a trailing slash.

Would you mind if I reverted these changes to use Resolve?

[Devlin, 2016-11-21 15:39:32]

https://codereview.chromium.org/2506713002/diff/80001/extensions/common/exten...
File extensions/common/extension_urls.cc (right):

https://codereview.chromium.org/2506713002/diff/80001/extensions/common/exten...
extensions/common/extension_urls.cc:47: return
GetWebstoreLaunchURL().Resolve("/category/extensions").spec();
On 2016/11/21 01:21:24, Charlie Harrison wrote:
> Using Resolve() here and below is causing subtle errors with the tests. Fixing
> the root causes is becoming troublesome.
> 
> The problem is that Resolving with relative URL with a leading "/" means we
> consider it an absolute URL, so we remove the existing path component of the
> launch url.
> 
> To resolve right, we also need the launch URL (with a path) to end with a "/".
> Unfortunately for some tests (e.g. WebstoreStartupInstallUnpackFailureTest),
> this is a bit tricky to do, as test infrastructure relies in a few places on
it
> not having a trailing slash.
> 
> Would you mind if I reverted these changes to use Resolve?

Reverting is fine.  Can you add a TODO(devlin) to use some GURL method rather
than doing the string addition though?

[Charlie Harrison, 2016-11-21 16:37:57]

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

[commit-bot: I haz the power, 2016-11-21 16:38:30]

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-21 16:39:11]

Thanks, I added the TODO and reverted the Resolve() calls.

I'll wait for a final l-g-t-m before landing

[commit-bot: I haz the power, 2016-11-21 17:21:49]

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

[commit-bot: I haz the power, 2016-11-21 17:21:50]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Charlie Harrison, 2016-11-21 20:18:07]

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

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

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

[Devlin, 2016-11-21 22:10:53]

lgtm

https://codereview.chromium.org/2506713002/diff/120001/extensions/common/exte...
File extensions/common/extension_urls.cc (right):

https://codereview.chromium.org/2506713002/diff/120001/extensions/common/exte...
extensions/common/extension_urls.cc:43: // TODO(csharrison,rdevlin.cronin):
Migrate the following methods to return
optional nit: It's more common for my TODOs to just be "devlin" (and means that
you can fit more on a line).  (I also have devlin@, and am going to try to
migrate over soon...but it's a lot of migration :P)

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

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

[commit-bot: I haz the power, 2016-11-21 22:22:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2016-11-22 02:26:00]

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

[commit-bot: I haz the power, 2016-11-22 02:26:43]

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-22 03:19:19]

Done. Had to fix a minor issue in webstore_provider_browsertest.cc for chromeos
but I also fixed your TODOs. Thanks!

[commit-bot: I haz the power, 2016-11-22 03:20:23]

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

[commit-bot: I haz the power, 2016-11-22 03:20:24]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-11-22 03:37:36]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-11-22 03:37:37]

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/2506713002/#ps140001
(title: "fix chromeos")

[commit-bot: I haz the power, 2016-11-22 03:38:01]

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-22 03:45:06]

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

[commit-bot: I haz the power, 2016-11-22 03:45:07]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Charlie Harrison, 2016-11-22 04:17:21]

csharrison@chromium.org changed reviewers:
+ derat@chromium.org, msw@chromium.org

[Charlie Harrison, 2016-11-22 04:17:21]

msw@chromium.org: Please review changes in 
chrome/browser/ui

derat@chromium.org: Please review changes in chromeos/app_mode/kiosk_app_data.cc

Thanks!

[Daniel Erat, 2016-11-22 15:46:21]

lgtm for c/b/chromeos

[msw, 2016-11-22 18:30:22]

c/b/ui lgtm

[Charlie Harrison, 2016-11-22 18:39:36]

Thanks everyone

[Charlie Harrison, 2016-11-22 18:39:44]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-11-22 18:40:09]

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-22 18:46:44]

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

[commit-bot: I haz the power, 2016-11-22 18:46:46]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Charlie Harrison, 2016-11-22 20:00:31]

csharrison@chromium.org changed reviewers:
+ mmenke@chromium.org

[Charlie Harrison, 2016-11-22 20:00:32]

+mmenke for prerender browsertest

[mmenke, 2016-11-22 20:12:39]

On 2016/11/22 20:00:32, Charlie Harrison wrote:
> +mmenke for prerender browsertest

prerender/ LGTM

[Charlie Harrison, 2016-11-22 20:13:07]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-11-22 20:13:47]

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-22 20:25:50]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1479845587899300,
"parent_rev": "96213cc78b788401e9e3ca35afd3702da7316416", "commit_rev":
"b20074f663cfcec1e82f8060661f6fba850f8e75"}

[commit-bot: I haz the power, 2016-11-22 20:26:24]

Description was changed from

==========
Avoid parsing the webstore base url so much.

This is a followup to [1] which modified the extension url API to vend
GURLs rather than std::string for webstore update urls.

This patch does the same for the webstore base url
(the webstore "launch" url). We only parse this url in
ChromeResourceDispatcherHostDelegate::OnResponseStarted so there is less
of a performance win than [1], but still we should avoid doing needless
work.

Parsing this URL takes ~12% of the CPU time of
ResourceLoader::CompleteResponseStarted.

[1] https://codereview.chromium.org/2493053002/

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

to

==========
Avoid parsing the webstore base url so much.

This is a followup to [1] which modified the extension url API to vend
GURLs rather than std::string for webstore update urls.

This patch does the same for the webstore base url
(the webstore "launch" url). We only parse this url in
ChromeResourceDispatcherHostDelegate::OnResponseStarted so there is less
of a performance win than [1], but still we should avoid doing needless
work.

Parsing this URL takes ~12% of the CPU time of
ResourceLoader::CompleteResponseStarted.

[1] https://codereview.chromium.org/2493053002/

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

[commit-bot: I haz the power, 2016-11-22 20:26:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-22 20:29:05]

Description was changed from

==========
Avoid parsing the webstore base url so much.

This is a followup to [1] which modified the extension url API to vend
GURLs rather than std::string for webstore update urls.

This patch does the same for the webstore base url
(the webstore "launch" url). We only parse this url in
ChromeResourceDispatcherHostDelegate::OnResponseStarted so there is less
of a performance win than [1], but still we should avoid doing needless
work.

Parsing this URL takes ~12% of the CPU time of
ResourceLoader::CompleteResponseStarted.

[1] https://codereview.chromium.org/2493053002/

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

to

==========
Avoid parsing the webstore base url so much.

This is a followup to [1] which modified the extension url API to vend
GURLs rather than std::string for webstore update urls.

This patch does the same for the webstore base url
(the webstore "launch" url). We only parse this url in
ChromeResourceDispatcherHostDelegate::OnResponseStarted so there is less
of a performance win than [1], but still we should avoid doing needless
work.

Parsing this URL takes ~12% of the CPU time of
ResourceLoader::CompleteResponseStarted.

[1] https://codereview.chromium.org/2493053002/

BUG=664174

Committed: https://crrev.com/5d1f214db0f7996f3c17cd87093d439ce4c7f8f1
Cr-Commit-Position: refs/heads/master@{#433963}
==========

[commit-bot: I haz the power, 2016-11-22 20:29:07]

Patchset 8 (id:??) landed as
https://crrev.com/5d1f214db0f7996f3c17cd87093d439ce4c7f8f1
Cr-Commit-Position: refs/heads/master@{#433963}