Move the parsing of "chrome_url_overrides" out of Extension.

Move the parsing of "chrome_url_overrides" out of Extension.

Moved a bunch of stuff out of ExtensionService.

This CL depends on https://chromiumcodereview.appspot.com/11624036/

Patch from Joe Thomas <mhx348@motorola.com>;.

BUG=159265
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176532

[Joe Thomas, 2012-12-21 20:17:05]

https://codereview.chromium.org/11660016/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11660016/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:83: #include
"chrome/common/extensions/api/url_overrides/url_overrides_handler.h"
I will remove this.

[Joe Thomas, 2012-12-22 01:25:57]

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/url_overrides/url_overrides_api.h (right):

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/url_overrides/url_overrides_api.h:18: public
content::NotificationObserver {

There are no URLOverrides APIs in extensions, but probably we need a similar
class to pull the url_overrides stuff out of ExtensionService and place it in
c/b/e/ instead of c/b/e/api/.

@Yoyo: what do you think?

[Yoyo Zhou, 2012-12-28 03:59:45]

My general comment on placement of things: if there's no well-defined API for a
manifest feature, it can be in chrome/browser/extensions (which is currently a
mess, but one goal is to be able to move non-browser-related things to the
top-level extensions directory), which is the general home for extensions stuff
with dependency on the browser.

We might eventually want something like chrome/browser/extensions/manifest to
hold all of this stuff, depending on how much there ends up being.

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/url_overrides/url_overrides_api.h (right):

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/url_overrides/url_overrides_api.h:18: public
content::NotificationObserver {
On 2012/12/22 01:25:57, Joe Thomas wrote:
> 
> There are no URLOverrides APIs in extensions, but probably we need a similar
> class to pull the url_overrides stuff out of ExtensionService and place it in
> c/b/e/ instead of c/b/e/api/.
> 
> @Yoyo: what do you think?

Seems reasonable to have this be separated out, but I would probably leave it in
chrome/browser/extensions as it's something that belongs to the browser.

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/url_overrides/url_overrides_api_factory.cc
(right):

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/url_overrides/url_overrides_api_factory.cc:5:
#include
"chrome/browser/extensions/api/url_overrides/url_overrides_api_factory.h"
Switch to ProfileKeyedAPIFactory

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
File chrome/common/extensions/api/url_overrides/url_overrides_handler.cc
(right):

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
chrome/common/extensions/api/url_overrides/url_overrides_handler.cc:28:
URLOverridesInfo::GetChromeURLOverrides(const Extension* extension) {
This reads somewhat redundantly. Can this Info be combined into the URLInfo from
your other CL?

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
File
chrome/common/extensions/manifest_tests/extension_manifests_override_unittest.cc
(left):

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
chrome/common/extensions/manifest_tests/extension_manifests_override_unittest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
I'd say that this file could also be left in its current place.

[Joe Thomas, 2012-12-29 14:44:28]

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_overrides/url_overrides_handler.cc
(right):

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/url_overrides/url_overrides_handler.cc:28:
URLOverridesInfo::GetChromeURLOverrides(const Extension* extension) {
On 2012/12/28 03:59:45, Yoyo Zhou wrote:
> This reads somewhat redundantly. Can this Info be combined into the URLInfo
from
> your other CL?

Yes we can combine, but I think that will increase the size of URLInfo struct.
URLInfo stores data in a GURL object whereas we need a std::map<const
std::string, GURL> to hold the data in URLOverridesInfo. Is it fine to keep both
these objects side-by-side in URLInfo or do you think there is a better way to
do this?

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_overrides/url_overrides_handler.h (right):

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/url_overrides/url_overrides_handler.h:25: static
const URLOverrideMap GetChromeURLOverrides(const Extension* extension);

I think it's better to pass a pointer to URLOverridesInfo to avoid making a copy
and handle the null case at the caller side. I will make that change.

[Joe Thomas, 2012-12-30 07:09:38]

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_overrides/url_overrides_handler.h (right):

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/url_overrides/url_overrides_handler.h:25: static
const URLOverrideMap GetChromeURLOverrides(const Extension* extension);
On 2012/12/29 14:44:28, Joe Thomas wrote:
> 
> I think it's better to pass a pointer to URLOverridesInfo to avoid making a
copy
> and handle the null case at the caller side. I will make that change.

We dont need a pointer to be returned. I will make use of base::LazyInstance and
return a reference here.

[Joe Thomas, 2013-01-03 03:54:47]

+ Devlin

[Yoyo Zhou, 2013-01-03 22:36:03]

https://codereview.chromium.org/11660016/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/url_overrides/url_overrides_api.h (right):

https://codereview.chromium.org/11660016/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/url_overrides/url_overrides_api.h:18: public
content::NotificationObserver {
On 2012/12/28 03:59:45, Yoyo Zhou wrote:
> On 2012/12/22 01:25:57, Joe Thomas wrote:
> > 
> > There are no URLOverrides APIs in extensions, but probably we need a similar
> > class to pull the url_overrides stuff out of ExtensionService and place it
in
> > c/b/e/ instead of c/b/e/api/.
> > 
> > @Yoyo: what do you think?
> 
> Seems reasonable to have this be separated out, but I would probably leave it
in
> chrome/browser/extensions as it's something that belongs to the browser.

My vote is still to leave this in chrome/browser/extensions.

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_overrides/url_overrides_handler.cc
(right):

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/url_overrides/url_overrides_handler.cc:28:
URLOverridesInfo::GetChromeURLOverrides(const Extension* extension) {
On 2012/12/29 14:44:28, Joe Thomas wrote:
> On 2012/12/28 03:59:45, Yoyo Zhou wrote:
> > This reads somewhat redundantly. Can this Info be combined into the URLInfo
> from
> > your other CL?
> 
> Yes we can combine, but I think that will increase the size of URLInfo struct.
> URLInfo stores data in a GURL object whereas we need a std::map<const
> std::string, GURL> to hold the data in URLOverridesInfo. Is it fine to keep
both
> these objects side-by-side in URLInfo or do you think there is a better way to
> do this?
> 

As I see it, URLInfo is a general-purpose thing for associating various URLs to
an extension. These are also various URLs that we'd like to associate with an
extension.

I don't necessarily mean that URLInfo should now contain a map of strings->URLs
rather than a single URL as it does now; but since these are related concepts,
it'd make sense to put them together.

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_overrides/url_overrides_handler.h (right):

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/url_overrides/url_overrides_handler.h:25: static
const URLOverrideMap GetChromeURLOverrides(const Extension* extension);
On 2012/12/30 07:09:39, Joe Thomas wrote:
> On 2012/12/29 14:44:28, Joe Thomas wrote:
> > 
> > I think it's better to pass a pointer to URLOverridesInfo to avoid making a
> copy
> > and handle the null case at the caller side. I will make that change.
> 
> We dont need a pointer to be returned. I will make use of base::LazyInstance
and
> return a reference here.

I'm not sure I understand how to use LazyInstance here. Can you explain what you
mean?

[Joe Thomas, 2013-01-04 23:36:11]

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/url_overrides/url_overrides_api.h (right):

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/url_overrides/url_overrides_api.h:18: public
content::NotificationObserver {
On 2013/01/03 22:36:03, Yoyo Zhou wrote:
> On 2012/12/28 03:59:45, Yoyo Zhou wrote:
> > On 2012/12/22 01:25:57, Joe Thomas wrote:
> > > 
> > > There are no URLOverrides APIs in extensions, but probably we need a
similar
> > > class to pull the url_overrides stuff out of ExtensionService and place it
> in
> > > c/b/e/ instead of c/b/e/api/.
> > > 
> > > @Yoyo: what do you think?
> > 
> > Seems reasonable to have this be separated out, but I would probably leave
it
> in
> > chrome/browser/extensions as it's something that belongs to the browser.
> 
> My vote is still to leave this in chrome/browser/extensions.

I made it part of ManifestURLParser in chrome/browser/extensions/.

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
File chrome/common/extensions/api/url_overrides/url_overrides_handler.cc
(right):

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
chrome/common/extensions/api/url_overrides/url_overrides_handler.cc:28:
URLOverridesInfo::GetChromeURLOverrides(const Extension* extension) {
On 2013/01/03 22:36:03, Yoyo Zhou wrote:
> On 2012/12/29 14:44:28, Joe Thomas wrote:
> > On 2012/12/28 03:59:45, Yoyo Zhou wrote:
> > > This reads somewhat redundantly. Can this Info be combined into the
URLInfo
> > from
> > > your other CL?
> > 
> > Yes we can combine, but I think that will increase the size of URLInfo
struct.
> > URLInfo stores data in a GURL object whereas we need a std::map<const
> > std::string, GURL> to hold the data in URLOverridesInfo. Is it fine to keep
> both
> > these objects side-by-side in URLInfo or do you think there is a better way
to
> > do this?
> > 
> 
> As I see it, URLInfo is a general-purpose thing for associating various URLs
to
> an extension. These are also various URLs that we'd like to associate with an
> extension.
> 
> I don't necessarily mean that URLInfo should now contain a map of
strings->URLs
> rather than a single URL as it does now; but since these are related concepts,
> it'd make sense to put them together.

Ah Ok. I misunderstood your previous comment. Done.

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
File chrome/common/extensions/api/url_overrides/url_overrides_handler.h (right):

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
chrome/common/extensions/api/url_overrides/url_overrides_handler.h:25: static
const URLOverrideMap GetChromeURLOverrides(const Extension* extension);
On 2013/01/03 22:36:03, Yoyo Zhou wrote:
> On 2012/12/30 07:09:39, Joe Thomas wrote:
> > On 2012/12/29 14:44:28, Joe Thomas wrote:
> > > 
> > > I think it's better to pass a pointer to URLOverridesInfo to avoid making
a
> > copy
> > > and handle the null case at the caller side. I will make that change.
> > 
> > We dont need a pointer to be returned. I will make use of base::LazyInstance
> and
> > return a reference here.
> 
> I'm not sure I understand how to use LazyInstance here. Can you explain what
you
> mean?

I was referring to creating a Lazy instance like below

struct EmptyURLOverrideMap {
  URLOverrideMap url_overrides;
};

static base::LazyInstance<EmptyURLOverrideMap> g_empty_overrides =
LAZY_INSTANCE_INITIALIZER;

and return g_empty_overrides.Get().url_overrides.

But since you mentioned that the above approach is not desirable in
https://chromiumcodereview.appspot.com/11741014/, I am returning a pointer here.

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
File
chrome/common/extensions/manifest_tests/extension_manifests_override_unittest.cc
(left):

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
chrome/common/extensions/manifest_tests/extension_manifests_override_unittest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2012/12/28 03:59:45, Yoyo Zhou wrote:
> I'd say that this file could also be left in its current place.

Done.

[Yoyo Zhou, 2013-01-05 00:46:11]

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
File chrome/common/extensions/api/url_overrides/url_overrides_handler.h (right):

https://chromiumcodereview.appspot.com/11660016/diff/3001/chrome/common/exten...
chrome/common/extensions/api/url_overrides/url_overrides_handler.h:25: static
const URLOverrideMap GetChromeURLOverrides(const Extension* extension);
On 2013/01/04 23:36:11, Joe Thomas wrote:
> On 2013/01/03 22:36:03, Yoyo Zhou wrote:
> > On 2012/12/30 07:09:39, Joe Thomas wrote:
> > > On 2012/12/29 14:44:28, Joe Thomas wrote:
> > > > 
> > > > I think it's better to pass a pointer to URLOverridesInfo to avoid
making
> a
> > > copy
> > > > and handle the null case at the caller side. I will make that change.
> > > 
> > > We dont need a pointer to be returned. I will make use of
base::LazyInstance
> > and
> > > return a reference here.
> > 
> > I'm not sure I understand how to use LazyInstance here. Can you explain what
> you
> > mean?
> 
> I was referring to creating a Lazy instance like below
> 
> struct EmptyURLOverrideMap {
>   URLOverrideMap url_overrides;
> };
> 
> static base::LazyInstance<EmptyURLOverrideMap> g_empty_overrides =
> LAZY_INSTANCE_INITIALIZER;
> 
> and return g_empty_overrides.Get().url_overrides.
> 
> But since you mentioned that the above approach is not desirable in
> https://chromiumcodereview.appspot.com/11741014/, I am returning a pointer
here.

Ah, I have since commented on the other CL that I didn't object to the
LazyInstance but to the extra struct. You should be able to just have a
LazyInstance<URLOverrideMap>, and that would be fine.

https://chromiumcodereview.appspot.com/11660016/diff/15001/chrome/common/exte...
File chrome/common/extensions/manifest_url_handler.h (right):

https://chromiumcodereview.appspot.com/11660016/diff/15001/chrome/common/exte...
chrome/common/extensions/manifest_url_handler.h:33: 
extraneous newline

[Joe Thomas, 2013-01-05 02:33:45]

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_overrides/url_overrides_handler.h (right):

https://codereview.chromium.org/11660016/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/url_overrides/url_overrides_handler.h:25: static
const URLOverrideMap GetChromeURLOverrides(const Extension* extension);
On 2013/01/05 00:46:11, Yoyo Zhou wrote:
> On 2013/01/04 23:36:11, Joe Thomas wrote:
> > On 2013/01/03 22:36:03, Yoyo Zhou wrote:
> > > On 2012/12/30 07:09:39, Joe Thomas wrote:
> > > > On 2012/12/29 14:44:28, Joe Thomas wrote:
> > > > > 
> > > > > I think it's better to pass a pointer to URLOverridesInfo to avoid
> making
> > a
> > > > copy
> > > > > and handle the null case at the caller side. I will make that change.
> > > > 
> > > > We dont need a pointer to be returned. I will make use of
> base::LazyInstance
> > > and
> > > > return a reference here.
> > > 
> > > I'm not sure I understand how to use LazyInstance here. Can you explain
what
> > you
> > > mean?
> > 
> > I was referring to creating a Lazy instance like below
> > 
> > struct EmptyURLOverrideMap {
> >   URLOverrideMap url_overrides;
> > };
> > 
> > static base::LazyInstance<EmptyURLOverrideMap> g_empty_overrides =
> > LAZY_INSTANCE_INITIALIZER;
> > 
> > and return g_empty_overrides.Get().url_overrides.
> > 
> > But since you mentioned that the above approach is not desirable in
> > https://chromiumcodereview.appspot.com/11741014/, I am returning a pointer
> here.
> 
> Ah, I have since commented on the other CL that I didn't object to the
> LazyInstance but to the extra struct. You should be able to just have a
> LazyInstance<URLOverrideMap>, and that would be fine.

Done.

https://codereview.chromium.org/11660016/diff/15001/chrome/common/extensions/...
File chrome/common/extensions/manifest_url_handler.h (right):

https://codereview.chromium.org/11660016/diff/15001/chrome/common/extensions/...
chrome/common/extensions/manifest_url_handler.h:33: 
On 2013/01/05 00:46:11, Yoyo Zhou wrote:
> extraneous newline

Removed

[Joe Thomas, 2013-01-08 01:12:10]

+estade for chrome/browser/ui/webui/

[Joe Thomas, 2013-01-08 01:15:01]

@Yoyo, Can you please look at the updated patch? Thanks.

[Yoyo Zhou, 2013-01-08 01:21:04]

LGTM. Sorry about the wait.

[Evan Stade, 2013-01-08 02:06:10]

webui rslgtm

[Joe Thomas, 2013-01-08 05:39:37]

On 2013/01/08 01:21:04, Yoyo Zhou wrote:
> LGTM. Sorry about the wait.

No problems. Thanks for the review.

[commit-bot: I haz the power, 2013-01-11 20:11:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11660016/32001

[commit-bot: I haz the power, 2013-01-12 13:14:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11660016/32001

[commit-bot: I haz the power, 2013-01-12 15:58:02]

Change committed as 176532