Moves extension_icon_image and image_loader to extensions/browser

Moves extension_icon_image and image_loader to extensions/browser

Now extension_icon_image and image_loader are safe to be used
outside of Chrome, like AppShell/Athena.

BUG=159265, 380421
R=yoz@chromium.org, jamescook@chromium.org
TBR=sky@chromium.org, fmalita@chromium.org
TEST=unit_tests, compile succeeds

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

[James Cook, 2014-06-17 22:01:04]

Hooray for extracting more code to src/extensions!  A few comments.

https://codereview.chromium.org/334053003/diff/20001/apps/DEPS
File apps/DEPS (left):

https://codereview.chromium.org/334053003/diff/20001/apps/DEPS#oldcode35
apps/DEPS:35: "+chrome/browser/extensions/extension_icon_image.h",
Hooray!

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/chrome_component_extension_resource_manager.cc
(right):

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_component_extension_resource_manager.cc:87:
void ChromeComponentExtensionResourceManager::AddComponentResourceEntries(
I think there was a comment for this function in the old code.

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/chrome_component_extension_resource_manager.h
(right):

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_component_extension_resource_manager.h:10:
#include "base/basictypes.h"
I think we now prefer

#include <stddef.h> for size_t and
#include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_component_extension_resource_manager.h:18:
class ChromeComponentExtensionResourceManager
nit: class comment please

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_system_impl.cc:324:
ComponentExtensionResourceManager::Set(
I would prefer to do this through ExtensionsBrowserClient, see later comment. 
This will require a bit of changes to tests, but TestExtensionsBrowserClient
could be helpful.

https://codereview.chromium.org/334053003/diff/20001/extensions/browser/compo...
File extensions/browser/component_extension_resource_manager.h (right):

https://codereview.chromium.org/334053003/diff/20001/extensions/browser/compo...
extensions/browser/component_extension_resource_manager.h:15: static void
Set(ComponentExtensionResourceManager* manager);
Rather than using a singleton I would prefer that you get a
ComponentExtensionResourceManager* via the ExtensionsClient or
ExtensionsBrowserClient interface. It's OK if it returns NULL.  That way the
extension system embedder knows that they must consider the issue of component
resources.

[Jun Mukai, 2014-06-17 23:53:02]

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/chrome_component_extension_resource_manager.cc
(right):

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_component_extension_resource_manager.cc:87:
void ChromeComponentExtensionResourceManager::AddComponentResourceEntries(
On 2014/06/17 22:01:04, James Cook wrote:
> I think there was a comment for this function in the old code.

file removed -- also added (copied) the old comment to the new place

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/chrome_component_extension_resource_manager.h
(right):

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_component_extension_resource_manager.h:10:
#include "base/basictypes.h"
On 2014/06/17 22:01:04, James Cook wrote:
> I think we now prefer
> 
> #include <stddef.h> for size_t and
> #include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN

file removed

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_component_extension_resource_manager.h:18:
class ChromeComponentExtensionResourceManager
On 2014/06/17 22:01:04, James Cook wrote:
> nit: class comment please

file removed

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/334053003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_system_impl.cc:324:
ComponentExtensionResourceManager::Set(
On 2014/06/17 22:01:04, James Cook wrote:
> I would prefer to do this through ExtensionsBrowserClient, see later comment. 
> This will require a bit of changes to tests, but TestExtensionsBrowserClient
> could be helpful.

Done.

https://codereview.chromium.org/334053003/diff/20001/extensions/browser/compo...
File extensions/browser/component_extension_resource_manager.h (right):

https://codereview.chromium.org/334053003/diff/20001/extensions/browser/compo...
extensions/browser/component_extension_resource_manager.h:15: static void
Set(ComponentExtensionResourceManager* manager);
On 2014/06/17 22:01:04, James Cook wrote:
> Rather than using a singleton I would prefer that you get a
> ComponentExtensionResourceManager* via the ExtensionsClient or
> ExtensionsBrowserClient interface. It's OK if it returns NULL.  That way the
> extension system embedder knows that they must consider the issue of component
> resources.

Sounds reasonable. Rather than returning an instance, I added a method itself to
ExtensionsBrowserClient.

[James Cook, 2014-06-18 16:29:07]

https://codereview.chromium.org/334053003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/334053003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_extensions_browser_client.cc:86: static const
GritResourceMap kExtraComponentExtensionResources[] = {
I would prefer that all the ComponentExtensions code live in a helper class
owned by ExtensionsBrowserClient. This file is too big.  Sorry I didn't suggest
that before.

Ideally ExtensionsBrowserClient would have one-line implementations of all
functions, and just route the calls into Chrome.

https://codereview.chromium.org/334053003/diff/40001/extensions/browser/exten...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/334053003/diff/40001/extensions/browser/exten...
extensions/browser/extensions_browser_client.h:11: #include
"base/files/file_path.h"
FilePath is already forward declared below.

[Jun Mukai, 2014-06-18 18:38:49]

https://codereview.chromium.org/334053003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/334053003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/chrome_extensions_browser_client.cc:86: static const
GritResourceMap kExtraComponentExtensionResources[] = {
On 2014/06/18 16:29:07, James Cook wrote:
> I would prefer that all the ComponentExtensions code live in a helper class
> owned by ExtensionsBrowserClient. This file is too big.  Sorry I didn't
suggest
> that before.
> 
> Ideally ExtensionsBrowserClient would have one-line implementations of all
> functions, and just route the calls into Chrome.
> 

Done.

https://codereview.chromium.org/334053003/diff/40001/extensions/browser/exten...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/334053003/diff/40001/extensions/browser/exten...
extensions/browser/extensions_browser_client.h:11: #include
"base/files/file_path.h"
On 2014/06/18 16:29:07, James Cook wrote:
> FilePath is already forward declared below.

removed

[James Cook, 2014-06-18 20:29:59]

LGTM!  Thanks for extracting this.

https://codereview.chromium.org/334053003/diff/80001/extensions/browser/compo...
File extensions/browser/component_extension_resource_manager.h (right):

https://codereview.chromium.org/334053003/diff/80001/extensions/browser/compo...
extensions/browser/component_extension_resource_manager.h:8: #include
"base/files/file_path.h"
nit: You can forward declare base::FilePath.

[Jun Mukai, 2014-06-19 16:47:14]

https://codereview.chromium.org/334053003/diff/80001/extensions/browser/compo...
File extensions/browser/component_extension_resource_manager.h (right):

https://codereview.chromium.org/334053003/diff/80001/extensions/browser/compo...
extensions/browser/component_extension_resource_manager.h:8: #include
"base/files/file_path.h"
On 2014/06/18 20:29:58, James Cook wrote:
> nit: You can forward declare base::FilePath.

Done.

[Jun Mukai, 2014-06-19 16:47:59]

yoz, could you review this?

[Yoyo Zhou, 2014-06-19 18:16:06]

LGTM

https://codereview.chromium.org/334053003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_component_extension_resource_manager.h
(right):

https://codereview.chromium.org/334053003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/chrome_component_extension_resource_manager.h:33:
std::map<base::FilePath, int> path_to_resource_id_;
But the comment (see next comment) should be here.

https://codereview.chromium.org/334053003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.h (right):

https://codereview.chromium.org/334053003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.h:111:
std::map<base::FilePath, int> path_to_resource_id_;
This shouldn't be here anymore.

https://codereview.chromium.org/334053003/diff/100001/extensions/browser/comp...
File extensions/browser/component_extension_resource_manager.h (right):

https://codereview.chromium.org/334053003/diff/100001/extensions/browser/comp...
extensions/browser/component_extension_resource_manager.h:14: // This class
manages which extension resources are actually come from
delete either 'are' or 'come'

[Jun Mukai, 2014-06-19 18:26:10]

https://codereview.chromium.org/334053003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_component_extension_resource_manager.h
(right):

https://codereview.chromium.org/334053003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/chrome_component_extension_resource_manager.h:33:
std::map<base::FilePath, int> path_to_resource_id_;
On 2014/06/19 18:16:06, Yoyo Zhou wrote:
> But the comment (see next comment) should be here.

Done.

https://codereview.chromium.org/334053003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.h (right):

https://codereview.chromium.org/334053003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.h:111:
std::map<base::FilePath, int> path_to_resource_id_;
On 2014/06/19 18:16:06, Yoyo Zhou wrote:
> This shouldn't be here anymore.

Done.

https://codereview.chromium.org/334053003/diff/100001/extensions/browser/comp...
File extensions/browser/component_extension_resource_manager.h (right):

https://codereview.chromium.org/334053003/diff/100001/extensions/browser/comp...
extensions/browser/component_extension_resource_manager.h:14: // This class
manages which extension resources are actually come from
On 2014/06/19 18:16:06, Yoyo Zhou wrote:
> delete either 'are' or 'come'

Done.

[Jun Mukai, 2014-06-19 18:27:40]

Code outside of extensions are trivial include changes. Submitting with TBR=
sky: for most of UI changes
fmalita: for adding +skia/ext/image_operations.h to DEPS

[Jun Mukai, 2014-06-19 18:27:46]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-06-19 18:29:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/334053003/120001

[Jun Mukai, 2014-06-19 21:43:55]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-06-19 21:45:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/334053003/140001

[commit-bot: I haz the power, 2014-06-20 04:53:42]

Change committed as 278601