Stop initializing url::Origin in extensions OnBeforeURLRequest when not needed

Stop initializing url::Origin in extensions OnBeforeURLRequest when not needed

This patch also removes a GURL copy.

BUG=664174
Committed: https://crrev.com/37cdff911bf891aacbd747c8d9a8be24678e8448
Cr-Commit-Position: refs/heads/master@{#436681}

[Charlie Harrison, 2016-12-02 21:51:51]

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

[commit-bot: I haz the power, 2016-12-02 21:52:52]

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-12-02 21:59:59]

Description was changed from

==========
Stop initializing url::Origin in extensions OnBeforeURLRequest

This patch also removes a GURL copy.

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

to

==========
Stop initializing url::Origin in extensions OnBeforeURLRequest

This patch also removes a GURL copy and a scheme copy.

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

[Charlie Harrison, 2016-12-02 22:00:24]

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

[Charlie Harrison, 2016-12-02 22:00:24]

rdsmith: ptal? This one is pretty trivial.

[Randy Smith (Not in Mondays), 2016-12-02 22:03:55]

LGTM; comment up to you.

https://codereview.chromium.org/2552453002/diff/1/chrome/browser/net/chrome_e...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2552453002/diff/1/chrome/browser/net/chrome_e...
chrome/browser/net/chrome_extensions_network_delegate.cc:176: const GURL&
url(request->url());
nit, suggestion: This is just syntactic sugar that somewhat (not really, but
somewhat) obscures where the data in url is coming from.  Consider just
replacing uses of url below with request->url()?

[Charlie Harrison, 2016-12-02 22:12:58]

https://codereview.chromium.org/2552453002/diff/1/chrome/browser/net/chrome_e...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2552453002/diff/1/chrome/browser/net/chrome_e...
chrome/browser/net/chrome_extensions_network_delegate.cc:176: const GURL&
url(request->url());
On 2016/12/02 22:03:55, Randy Smith - Not in Mondays wrote:
> nit, suggestion: This is just syntactic sugar that somewhat (not really, but
> somewhat) obscures where the data in url is coming from.  Consider just
> replacing uses of url below with request->url()?

Eh I prefer not, 4 uses of request->url() gets kind of noisy imo.

[Charlie Harrison, 2016-12-02 22:13:03]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2016-12-02 22:13:03]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-12-02 22:13:18]

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-12-02 22:50:02]

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

[commit-bot: I haz the power, 2016-12-02 22:50:03]

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-12-02 23:06:38]

Looks like a real bug filesystem schemes, will re-upload a fix

[Charlie Harrison, 2016-12-03 07:01:07]

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

[commit-bot: I haz the power, 2016-12-03 07:01:16]

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-12-03 08:28:47]

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

[commit-bot: I haz the power, 2016-12-03 08:28:48]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-12-03 16:52:16]

On 2016/12/02 23:06:38, Charlie Harrison wrote:
> Looks like a real bug filesystem schemes, will re-upload a fix

The code needs the origin special casing for scheme in the condition. It's a
little ugly but I think this is the cleanest way to do that.

[Charlie Harrison, 2016-12-05 22:52:33]

Oh, and PTAL?

[Randy Smith (Not in Mondays), 2016-12-06 16:09:20]

I'd suggest you expand the issue description a bit ("stop initializing ... in
cases where it isn't needed"?).  But LGTM; both this and the below note are
suggestions.

https://codereview.chromium.org/2552453002/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2552453002/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:193: (origin =
url::Origin(url)).scheme() == extensions::kExtensionScheme &&
Suggestion: I understand why you're doing this, and I feel the pull of terseness
myself, but I think looking at this code it both takes a moment to understand
what's actually happening and is actively hard to understand why it was written
this way (so that the origin copy doesn't happen unless the previous elements of
the conditional).  I'd suggest making this a doubly nested conditional with the
origin declaration (and thus assignment) in between the two conditionals.

Alternatively, you could explore turning this into an "early return" function by
hoisting the case at the end of it up to this point at which point you don't
need to worry about nesting conditionals at all.

[Charlie Harrison, 2016-12-06 18:22:50]

Description was changed from

==========
Stop initializing url::Origin in extensions OnBeforeURLRequest

This patch also removes a GURL copy and a scheme copy.

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

to

==========
Stop initializing url::Origin in extensions OnBeforeURLRequest when not needed

This patch also removes a GURL copy and a scheme copy.

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

[Charlie Harrison, 2016-12-06 18:23:24]

Description was changed from

==========
Stop initializing url::Origin in extensions OnBeforeURLRequest when not needed

This patch also removes a GURL copy and a scheme copy.

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

to

==========
Stop initializing url::Origin in extensions OnBeforeURLRequest when not needed

This patch also removes a GURL copy.

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

[Charlie Harrison, 2016-12-06 18:23:38]

Thanks!

https://codereview.chromium.org/2552453002/diff/20001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2552453002/diff/20001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:193: (origin =
url::Origin(url)).scheme() == extensions::kExtensionScheme &&
On 2016/12/06 16:09:20, Randy Smith - Not in Mondays wrote:
> Suggestion: I understand why you're doing this, and I feel the pull of
terseness
> myself, but I think looking at this code it both takes a moment to understand
> what's actually happening and is actively hard to understand why it was
written
> this way (so that the origin copy doesn't happen unless the previous elements
of
> the conditional).  I'd suggest making this a doubly nested conditional with
the
> origin declaration (and thus assignment) in between the two conditionals.
> 
> Alternatively, you could explore turning this into an "early return" function
by
> hoisting the case at the end of it up to this point at which point you don't
> need to worry about nesting conditionals at all.

Moved it to a nested conditional and added a justification comment.

[Charlie Harrison, 2016-12-06 18:23:45]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-12-06 18:23:46]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2552453002/#ps40001
(title: "nested conditional")

[commit-bot: I haz the power, 2016-12-06 18:24:28]

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-12-06 19:20:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481048625894150,
"parent_rev": "08bf4722e10ec5a929c5ce3ebe7f33779db1126f", "commit_rev":
"5543db75953bee7c88caa42e171e9a5e4513d19d"}

[commit-bot: I haz the power, 2016-12-06 19:21:25]

Description was changed from

==========
Stop initializing url::Origin in extensions OnBeforeURLRequest when not needed

This patch also removes a GURL copy.

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

to

==========
Stop initializing url::Origin in extensions OnBeforeURLRequest when not needed

This patch also removes a GURL copy.

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

[commit-bot: I haz the power, 2016-12-06 19:21:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-06 19:25:01]

Description was changed from

==========
Stop initializing url::Origin in extensions OnBeforeURLRequest when not needed

This patch also removes a GURL copy.

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

to

==========
Stop initializing url::Origin in extensions OnBeforeURLRequest when not needed

This patch also removes a GURL copy.

BUG=664174

Committed: https://crrev.com/37cdff911bf891aacbd747c8d9a8be24678e8448
Cr-Commit-Position: refs/heads/master@{#436681}
==========

[commit-bot: I haz the power, 2016-12-06 19:25:02]

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