Move the parsing of homepage_url" and "devtools_page" out of Extension.

Move the parsing of homepage_url" and "devtools_page" out of Extension.

-Created a ManifestURLParser PKS to register the ManifestHandler
for various URL related manifest keys.
-Created a generic ManifestURL ManifestData for different URLs

This CL takes care of only "homepage_url" and "devtools_page" keys.
I will create a follow-up CL for update_url and options_url.

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

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

[pfeldman, 2012-12-22 10:01:15]

Could you share the broader context of what is happening? It sounds like
dependency injection, but the price you are paying for this it is very high.

https://chromiumcodereview.appspot.com/11624036/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/devtools/devtools_api.h (right):

https://chromiumcodereview.appspot.com/11624036/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/devtools/devtools_api.h:15: class DevtoolsAPI :
public ProfileKeyedService {
DevToolsAPI

https://chromiumcodereview.appspot.com/11624036/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/devtools/devtools_api_factory.h (right):

https://chromiumcodereview.appspot.com/11624036/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/devtools/devtools_api_factory.h:15: class
DevtoolsAPIFactory : public ProfileKeyedServiceFactory {
We use DevTools prefix (although  devtools_ in the file name).

https://chromiumcodereview.appspot.com/11624036/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/devtools/devtools_page_handler.cc (right):

https://chromiumcodereview.appspot.com/11624036/diff/1/chrome/common/extensio...
chrome/common/extensions/api/devtools/devtools_page_handler.cc:40: Extension*
extension,
poor indent

[Yoyo Zhou, 2012-12-26 18:08:43]

pfeldman: the context is in the design doc in the bug, specifically this part:
https://docs.google.com/a/chromium.org/document/d/1hSwqniJVtk3he1fTl_PW422_48...

However, I think it's unwieldy to have a separate thing for each URL. The URLs
are meaningful in different places, but it would probably be useful to have a
generic ManifestData for a URL (not specific to devtools). Furthermore, we could
consolidate all of the parsing of homepage_url, update_url, devtools_url, etc.
into a single API.

One detail is that the renderer needs to know about the devtools URL, which
means the manifest handler needs to be registered in the renderer also.
ChromeContentRendererClient (probably RenderThreadStarted) is the best place
I've found to do this.

[Joe Thomas, 2012-12-26 19:20:47]

@pfeldman: Thanks for the review. I will consider your review comments in the
next iteration.


> However, I think it's unwieldy to have a separate thing for each URL. The URLs
> are meaningful in different places, but it would probably be useful to have a
> generic ManifestData for a URL (not specific to devtools). Furthermore, we
could
> consolidate all of the parsing of homepage_url, update_url, devtools_url, etc.
> into a single API.
> 
> One detail is that the renderer needs to know about the devtools URL, which
> means the manifest handler needs to be registered in the renderer also.
> ChromeContentRendererClient (probably RenderThreadStarted) is the best place
> I've found to do this.

Sure. I will make changes as per your suggestions. Thanks.

[Joe Thomas, 2012-12-27 01:46:26]

> However, I think it's unwieldy to have a separate thing for each URL. The URLs
> are meaningful in different places, but it would probably be useful to have a
> generic ManifestData for a URL (not specific to devtools). Furthermore, we
could
> consolidate all of the parsing of homepage_url, update_url, devtools_url, etc.
> into a single API.
> 
> One detail is that the renderer needs to know about the devtools URL, which
> means the manifest handler needs to be registered in the renderer also.
> ChromeContentRendererClient (probably RenderThreadStarted) is the best place
> I've found to do this.

@Yoyo:
Modified the CL as per your suggestions. Please take a look. Thanks.

[Yoyo Zhou, 2012-12-27 17:16:17]

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/browser/exte...
File chrome/browser/extensions/api/url_parser/url_parser_api.h (right):

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/url_parser/url_parser_api.h:15: class URLParserAPI
: public ProfileKeyedService {
Hmm, I'm not sure if this name makes sense. I think I'd prefer this just to be
in chrome/browser/extensions/manifest_url_parser as ManifestURLParser.

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/browser/exte...
File chrome/browser/extensions/api/url_parser/url_parser_api_factory.h (right):

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/browser/exte...
chrome/browser/extensions/api/url_parser/url_parser_api_factory.h:15: class
URLParserAPIFactory : public ProfileKeyedServiceFactory {
Check out the ProfileKeyedAPIFactory.

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/chrome_tests...
File chrome/chrome_tests_unit.gypi (right):

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/chrome_tests...
chrome/chrome_tests_unit.gypi:1448:
'common/extensions/api/url_parser/devtools_page_manifests_unittest.cc',
I don't think these tests need to move.

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/common/exten...
File chrome/common/extensions/api/url_parser/devtools_page_handler.cc (right):

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/common/exten...
chrome/common/extensions/api/url_parser/devtools_page_handler.cc:8: #include
"base/utf_string_conversions.h"
Make sure you only include what you use.

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/common/exten...
File chrome/common/extensions/api/url_parser/url_info.cc (right):

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/common/exten...
chrome/common/extensions/api/url_parser/url_info.cc:22: return GURL();
You can use GURL::EmptyGURL for the missing value case.

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/common/exten...
File chrome/common/extensions/api/url_parser/url_info.h (right):

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/common/exten...
chrome/common/extensions/api/url_parser/url_info.h:16: enum URLType {
I don't think this interface makes sense. Why not
GetDevToolsPage(extension)
GetHomepageURL(extension)
?

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/common/exten...
chrome/common/extensions/api/url_parser/url_info.h:24: static const GURL
url(const Extension* extension, const URLType type);
This should be of type const GURL& to avoid making a copy.

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/common/exten...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/common/exten...
chrome/common/extensions/extension.cc:1583: finished_parsing_manifest_ = true;
Why is this being moved? By the way, Devlin is changing this thread safety check
in

https://chromiumcodereview.appspot.com/11644057/

so it should work better.

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/common/exten...
File chrome/common/extensions/permissions/permission_set.cc (right):

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/common/exten...
chrome/common/extensions/permissions/permission_set.cc:533: if
(!extensions::URLInfo::url(
This is in the extensions namespace.

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/11624036/diff/8001/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:125: void
RegisterManifestHandlers() {
This should probably be called RegisterExtensionManifestHandlers. The comment
should say "all extension ManifestHandlers needed to parse fields used in the
renderer."

[Joe Thomas, 2012-12-30 04:26:00]

https://codereview.chromium.org/11624036/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/url_parser/url_parser_api.h (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/url_parser/url_parser_api.h:15: class URLParserAPI
: public ProfileKeyedService {
On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> Hmm, I'm not sure if this name makes sense. I think I'd prefer this just to be
> in chrome/browser/extensions/manifest_url_parser as ManifestURLParser.

Done.

https://codereview.chromium.org/11624036/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/url_parser/url_parser_api_factory.h (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/url_parser/url_parser_api_factory.h:15: class
URLParserAPIFactory : public ProfileKeyedServiceFactory {
On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> Check out the ProfileKeyedAPIFactory.

Done.

https://codereview.chromium.org/11624036/diff/8001/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:1448:
'common/extensions/api/url_parser/devtools_page_manifests_unittest.cc',
On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> I don't think these tests need to move.

Done. Moved back to its original place.

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_parser/devtools_page_handler.cc (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/url_parser/devtools_page_handler.cc:8: #include
"base/utf_string_conversions.h"
On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> Make sure you only include what you use.

ASCIIToUTF16 which is used in DevToolsPageHandler::Parse function is defined in
base/utf_string_conversions.h

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_parser/url_info.cc (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/url_parser/url_info.cc:22: return GURL();
On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> You can use GURL::EmptyGURL for the missing value case.

Done.

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_parser/url_info.h (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/url_parser/url_info.h:16: enum URLType {
On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> I don't think this interface makes sense. Why not
> GetDevToolsPage(extension)
> GetHomepageURL(extension)
> ?

Done.

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/url_parser/url_info.h:24: static const GURL
url(const Extension* extension, const URLType type);
On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> This should be of type const GURL& to avoid making a copy.

Since we create a temporary GURL object inside GetHomepageURL(extension)
function, a reference cannot be returned for GetHomepageURL.

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1583: finished_parsing_manifest_ = true;
On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> Why is this being moved? By the way, Devlin is changing this thread safety
check
> in
> 
> https://chromiumcodereview.appspot.com/11644057/
> 
> so it should work better.

PermissionSet constructor below queries for "devtools_page" URL through
PermissionSet::InitImplicitExtensionPermissions and hits DCHECK in
Extension::GetManifestData. So I think it's reasonable to move this setter up as
Manifest parsing is already completed. 

I will revisit this change once https://chromiumcodereview.appspot.com/11644057/
lands and move it back.

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/permission_set.cc (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/p...
chrome/common/extensions/permissions/permission_set.cc:533: if
(!extensions::URLInfo::url(
On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> This is in the extensions namespace.

Done.

https://codereview.chromium.org/11624036/diff/8001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:125: void
RegisterManifestHandlers() {
On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> This should probably be called RegisterExtensionManifestHandlers. The comment
> should say "all extension ManifestHandlers needed to parse fields used in the
> renderer."

Done.

[Yoyo Zhou, 2013-01-03 01:45:54]

https://codereview.chromium.org/11624036/diff/8001/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:1448:
'common/extensions/api/url_parser/devtools_page_manifests_unittest.cc',
On 2012/12/30 04:26:00, Joe Thomas wrote:
> On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> > I don't think these tests need to move.
> 
> Done. Moved back to its original place.

They still seem to be deleted in the latest patchset...

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_parser/devtools_page_handler.cc (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/url_parser/devtools_page_handler.cc:8: #include
"base/utf_string_conversions.h"
On 2012/12/30 04:26:00, Joe Thomas wrote:
> On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> > Make sure you only include what you use.
> 
> ASCIIToUTF16 which is used in DevToolsPageHandler::Parse function is defined
in
> base/utf_string_conversions.h

Ah, sorry. Thanks for paying more attention than me.

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_parser/url_info.h (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/url_parser/url_info.h:24: static const GURL
url(const Extension* extension, const URLType type);
On 2012/12/30 04:26:00, Joe Thomas wrote:
> On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> > This should be of type const GURL& to avoid making a copy.
> 
> Since we create a temporary GURL object inside GetHomepageURL(extension)
> function, a reference cannot be returned for GetHomepageURL.

This was my point about GURL::EmptyGURL - you can avoid a temporary.

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1583: finished_parsing_manifest_ = true;
On 2012/12/30 04:26:00, Joe Thomas wrote:
> On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> > Why is this being moved? By the way, Devlin is changing this thread safety
> check
> > in
> > 
> > https://chromiumcodereview.appspot.com/11644057/
> > 
> > so it should work better.
> 
> PermissionSet constructor below queries for "devtools_page" URL through
> PermissionSet::InitImplicitExtensionPermissions and hits DCHECK in
> Extension::GetManifestData. So I think it's reasonable to move this setter up
as
> Manifest parsing is already completed. 
> 
> I will revisit this change once
https://chromiumcodereview.appspot.com/11644057/
> lands and move it back.

Once that lands, you should not need to change this. But your change here is not
a good workaround; finished_parsing_manifest_ tells us that the extension is
done being constructed and safe to read from other threads. But this is not true
if the permissions haven't been set yet.

https://codereview.chromium.org/11624036/diff/19001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://codereview.chromium.org/11624036/diff/19001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_dependency_manager.cc:266:
<extensions::ManifestURLParser>::GetInstance();
I think it's better to break the line after the second :: if possible.

https://codereview.chromium.org/11624036/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/devtools_page_handler.h (right):

https://codereview.chromium.org/11624036/diff/19001/chrome/common/extensions/...
chrome/common/extensions/devtools_page_handler.h:5: #ifndef
CHROME_COMMON_EXTENSIONS_DEVTOOLS_PAGE_HANDLER_H_
I think these are simple enough that it's not worth it to put each in a separate
file. I prefer Devlin's approach to put them in a common
extensions_url_handler.cc; see:
https://codereview.chromium.org/11742005/

https://codereview.chromium.org/11624036/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/manifest_url_info.h (right):

https://codereview.chromium.org/11624036/diff/19001/chrome/common/extensions/...
chrome/common/extensions/manifest_url_info.h:16: struct ManifestURLInfo : public
Extension::ManifestData {
I think you could call this just ManifestURL.

[Joe Thomas, 2013-01-03 19:56:31]

Please take a look at the latest patch. Thanks

https://codereview.chromium.org/11624036/diff/8001/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:1448:
'common/extensions/api/url_parser/devtools_page_manifests_unittest.cc',
On 2013/01/03 01:45:54, Yoyo Zhou wrote:
> On 2012/12/30 04:26:00, Joe Thomas wrote:
> > On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> > > I don't think these tests need to move.
> > 
> > Done. Moved back to its original place.
> 
> They still seem to be deleted in the latest patchset...

It was showing with "D" symbol but side-by-side diff shows the modification. Not
sure whether it was a bug with the system.

I corrected it by uploading the new patchset in two steps.

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_parser/url_info.h (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/url_parser/url_info.h:24: static const GURL
url(const Extension* extension, const URLType type);
On 2013/01/03 01:45:54, Yoyo Zhou wrote:
> On 2012/12/30 04:26:00, Joe Thomas wrote:
> > On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> > > This should be of type const GURL& to avoid making a copy.
> > 
> > Since we create a temporary GURL object inside GetHomepageURL(extension)
> > function, a reference cannot be returned for GetHomepageURL.
> 
> This was my point about GURL::EmptyGURL - you can avoid a temporary.

If you look at ManifestURL::GetHomepageURL definition, apart from creating the
EmptyURL, we need to create one more temporary GURL object with
"extension_urls::GetWebstoreItemDetailURLPrefix() + extension->id()". Because of
that we will not be able to return reference in that one getter function.

https://codereview.chromium.org/11624036/diff/19001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://codereview.chromium.org/11624036/diff/19001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_dependency_manager.cc:266:
<extensions::ManifestURLParser>::GetInstance();
On 2013/01/03 01:45:55, Yoyo Zhou wrote:
> I think it's better to break the line after the second :: if possible.

Done.

https://codereview.chromium.org/11624036/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/devtools_page_handler.h (right):

https://codereview.chromium.org/11624036/diff/19001/chrome/common/extensions/...
chrome/common/extensions/devtools_page_handler.h:5: #ifndef
CHROME_COMMON_EXTENSIONS_DEVTOOLS_PAGE_HANDLER_H_
On 2013/01/03 01:45:55, Yoyo Zhou wrote:
> I think these are simple enough that it's not worth it to put each in a
separate
> file. I prefer Devlin's approach to put them in a common
> extensions_url_handler.cc; see:
> https://codereview.chromium.org/11742005/

Done. I created a manifest_url_handler.cc/h Please let me know whether you
prefer extensions_url_handler over this

https://codereview.chromium.org/11624036/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/manifest_url_info.h (right):

https://codereview.chromium.org/11624036/diff/19001/chrome/common/extensions/...
chrome/common/extensions/manifest_url_info.h:16: struct ManifestURLInfo : public
Extension::ManifestData {
On 2013/01/03 01:45:55, Yoyo Zhou wrote:
> I think you could call this just ManifestURL.

Done.

[Yoyo Zhou, 2013-01-03 22:55:32]

Just a few comments about comments.

https://codereview.chromium.org/11624036/diff/8001/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:1448:
'common/extensions/api/url_parser/devtools_page_manifests_unittest.cc',
On 2013/01/03 19:56:31, Joe Thomas wrote:
> On 2013/01/03 01:45:54, Yoyo Zhou wrote:
> > On 2012/12/30 04:26:00, Joe Thomas wrote:
> > > On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> > > > I don't think these tests need to move.
> > > 
> > > Done. Moved back to its original place.
> > 
> > They still seem to be deleted in the latest patchset...
> 
> It was showing with "D" symbol but side-by-side diff shows the modification.
Not
> sure whether it was a bug with the system.
> 
> I corrected it by uploading the new patchset in two steps.

Ok, that's weird. I filed a bug about this.
https://code.google.com/p/rietveld/issues/detail?id=410

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
File chrome/common/extensions/api/url_parser/url_info.h (right):

https://codereview.chromium.org/11624036/diff/8001/chrome/common/extensions/a...
chrome/common/extensions/api/url_parser/url_info.h:24: static const GURL
url(const Extension* extension, const URLType type);
On 2013/01/03 19:56:31, Joe Thomas wrote:
> On 2013/01/03 01:45:54, Yoyo Zhou wrote:
> > On 2012/12/30 04:26:00, Joe Thomas wrote:
> > > On 2012/12/27 17:16:17, Yoyo Zhou wrote:
> > > > This should be of type const GURL& to avoid making a copy.
> > > 
> > > Since we create a temporary GURL object inside GetHomepageURL(extension)
> > > function, a reference cannot be returned for GetHomepageURL.
> > 
> > This was my point about GURL::EmptyGURL - you can avoid a temporary.
> 
> If you look at ManifestURL::GetHomepageURL definition, apart from creating the
> EmptyURL, we need to create one more temporary GURL object with
> "extension_urls::GetWebstoreItemDetailURLPrefix() + extension->id()". Because
of
> that we will not be able to return reference in that one getter function.

Oh, okay. This will be the same as the old GetHomepageURL then, so that's fine.

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

https://codereview.chromium.org/11624036/diff/33001/chrome/common/extensions/...
chrome/common/extensions/manifest_url_handler.h:15: struct ManifestURL : public
Extension::ManifestData {
Class needs a descriptive comment.

https://codereview.chromium.org/11624036/diff/33001/chrome/common/extensions/...
chrome/common/extensions/manifest_url_handler.h:17: // The URL to one of the
following:
This comment isn't necessary - it'll be redundant with the descriptions of the
getters below.

[Yoyo Zhou, 2013-01-03 23:11:44]

https://codereview.chromium.org/11624036/diff/33001/chrome/common/extensions/...
File
chrome/common/extensions/manifest_tests/extension_manifests_devtools_unittest.cc
(right):

https://codereview.chromium.org/11624036/diff/33001/chrome/common/extensions/...
chrome/common/extensions/manifest_tests/extension_manifests_devtools_unittest.cc:15:
namespace {
By the way, I didn't notice this earlier - I don't think this is common
practice, so I wouldn't do it. (But feel free to point out counterexamples.)

[Joe Thomas, 2013-01-04 00:34:59]

https://codereview.chromium.org/11624036/diff/33001/chrome/common/extensions/...
File
chrome/common/extensions/manifest_tests/extension_manifests_devtools_unittest.cc
(right):

https://codereview.chromium.org/11624036/diff/33001/chrome/common/extensions/...
chrome/common/extensions/manifest_tests/extension_manifests_devtools_unittest.cc:15:
namespace {
On 2013/01/03 23:11:44, Yoyo Zhou wrote:
> By the way, I didn't notice this earlier - I don't think this is common
> practice, so I wouldn't do it. (But feel free to point out counterexamples.)

I remove the namespace. I added that because I remember seeing such a usage
somewhere, but not able to recall.

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

https://codereview.chromium.org/11624036/diff/33001/chrome/common/extensions/...
chrome/common/extensions/manifest_url_handler.h:15: struct ManifestURL : public
Extension::ManifestData {
On 2013/01/03 22:55:33, Yoyo Zhou wrote:
> Class needs a descriptive comment.

Done.

https://codereview.chromium.org/11624036/diff/33001/chrome/common/extensions/...
chrome/common/extensions/manifest_url_handler.h:17: // The URL to one of the
following:
On 2013/01/03 22:55:33, Yoyo Zhou wrote:
> This comment isn't necessary - it'll be redundant with the descriptions of the
> getters below.

Done.

[Yoyo Zhou, 2013-01-04 00:39:04]

LGTM

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

https://codereview.chromium.org/11624036/diff/26008/chrome/common/extensions/...
chrome/common/extensions/manifest_url_handler.h:15: // A structure to hold
various URLs(devtools_page, homepage_url, etc)
You can just delete this parenthetical. (But in the future, put a space before
an open paren and punctuate etc.)

[Joe Thomas, 2013-01-04 00:55:30]

+ben for gyp & chrome/renderer/
+pfeldman for chrome/browser/devtools/
+arv for  chrome/browser/ui/webui/
+mirandac for chrome/browser/profiles/

Please review.

[Joe Thomas, 2013-01-08 05:38:22]

@ben, pfeldman, arv & mirandac,

Ping. Could you please review the changes in your components?   Thanks!

[Miranda Callahan, 2013-01-08 13:57:13]

On 2013/01/08 05:38:22, Joe Thomas wrote:
> @ben, pfeldman, arv & mirandac,
> 
> Ping. Could you please review the changes in your components?   Thanks!

profile_dependency_manager.cc additions LGTM.

[pfeldman, 2013-01-09 09:58:08]

devtools lgtm.

[Ben Goodger (Google), 2013-01-09 17:17:34]

lgtm

[Joe Thomas, 2013-01-10 14:58:52]

@arv, estade,

Looking for OWNERS approval in chrome/browser/ui/webui/. Thanks!

[Evan Stade, 2013-01-10 20:22:37]

lgtm

[commit-bot: I haz the power, 2013-01-10 22:02:38]

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

[commit-bot: I haz the power, 2013-01-11 05:07:25]

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

[commit-bot: I haz the power, 2013-01-11 07:57:58]

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

[commit-bot: I haz the power, 2013-01-11 07:58:03]

Failed to apply patch for chrome/browser/profiles/profile_dependency_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/profiles/profile_dependency_manager.cc
  Hunk #1 FAILED at 35.
  Hunk #2 succeeded at 263 (offset 1 line).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/profiles/profile_dependency_manager.cc.rej

Patch:       chrome/browser/profiles/profile_dependency_manager.cc
Index: chrome/browser/profiles/profile_dependency_manager.cc
diff --git a/chrome/browser/profiles/profile_dependency_manager.cc
b/chrome/browser/profiles/profile_dependency_manager.cc
index
543c71c0f6abcf80c688f2a29d277a968e827d07..cfd54af7d1a71c2c1fe43cd2c0c20f418851ed11
100644
--- a/chrome/browser/profiles/profile_dependency_manager.cc
+++ b/chrome/browser/profiles/profile_dependency_manager.cc
@@ -35,6 +35,7 @@
 #include "chrome/browser/extensions/api/web_navigation/web_navigation_api.h"
 #include "chrome/browser/extensions/app_restore_service_factory.h"
 #include "chrome/browser/extensions/extension_system_factory.h"
+#include "chrome/browser/extensions/manifest_url_parser.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include
"chrome/browser/geolocation/chrome_geolocation_permission_context_factory.h"
 #include "chrome/browser/google/google_url_tracker_factory.h"
@@ -262,6 +263,7 @@ void ProfileDependencyManager::AssertFactoriesBuilt() {
 #endif
   extensions::ManagedModeAPI::GetFactoryInstance();
   extensions::ManagementAPI::GetFactoryInstance();
+  extensions::ManifestURLParser::GetFactoryInstance();
   extensions::MediaGalleriesPrivateAPIFactory::GetInstance();
 #if defined(OS_CHROMEOS)
   extensions::MediaPlayerAPI::GetFactoryInstance();

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

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

[commit-bot: I haz the power, 2013-01-11 11:50:15]

Change committed as 176316