Move system.* family of APIs to extensions/

Move system.* family of APIs to extensions/

As side effect, this APIs will also be available for the app_shell.
The patch also removes the dependence this API has on the g_browser_process
and thus, no longer will depend on chrome/.

BUG=392842
Committed: https://crrev.com/89e236b4594095c089b53d9cd4d521eb81860721
Cr-Commit-Position: refs/heads/master@{#293512}

[tmpsantos, 2014-07-11 14:00:49]

hongbo.min@intel.com: Please review changes in 

thestig@chromium.org: Please review changes in 

yoz@chromium.org: Please review changes in

[Lei Zhang, 2014-07-11 19:34:51]

On 2014/07/11 14:00:49, tmpsantos wrote:
> mailto:hongbo.min@intel.com: Please review changes in 
> 
> mailto:thestig@chromium.org: Please review changes in 
> 
> mailto:yoz@chromium.org: Please review changes in

You didn't finish your though.

[tmpsantos, 2014-07-13 16:09:28]

On 2014/07/11 19:34:51, Lei Zhang wrote:
> On 2014/07/11 14:00:49, tmpsantos wrote:
> > mailto:hongbo.min@intel.com: Please review changes in 
> > 
> > mailto:thestig@chromium.org: Please review changes in 
> > 
> > mailto:yoz@chromium.org: Please review changes in
> 
> You didn't finish your though.

Sorry, I meant to ping the reviewers with an empty comment, but this was added
by the Chromite Butler and I missed it. :)

This patch is ready to be reviewed.

[Hongbo Min, 2014-07-14 03:04:43]

lgtm for cpu/storage/display/memory part. Thanks for your effort on well-formed
refactoring!

[Lei Zhang, 2014-07-14 19:14:54]

chrome/ and device/ lgtm

[Yoyo Zhou, 2014-07-15 00:39:13]

https://codereview.chromium.org/389633002/diff/80001/apps/shell/browser/shell...
File apps/shell/browser/shell_extensions_browser_client.cc (right):

https://codereview.chromium.org/389633002/diff/80001/apps/shell/browser/shell...
apps/shell/browser/shell_extensions_browser_client.cc:250: // this method.
I'm confused - is this meant to be implemented?

You could work within the assumption that there's only one browser context
(browser_context_) in app_shell, and dispatch it to just its event router.

https://codereview.chromium.org/389633002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/system_display/display_info_provider_chromeos_unittest.cc
(right):

https://codereview.chromium.org/389633002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/system_display/display_info_provider_chromeos_unittest.cc:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
It looks like this file could be moved to src/extensions also.

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

https://codereview.chromium.org/389633002/diff/80001/extensions/browser/exten...
extensions/browser/extensions_browser_client.h:183: // Propage a event to all
the renderers. The implementation must be safe to
typo: Propagate
I would write: "all the renderers in every browser context." (strictly speaking,
not true because of incognito profiles, but I think that's more than we want to
get into in this comment)

[tmpsantos, 2014-07-16 08:41:13]

On 2014/07/15 00:39:13, Yoyo Zhou wrote:
>
https://codereview.chromium.org/389633002/diff/80001/apps/shell/browser/shell...
> File apps/shell/browser/shell_extensions_browser_client.cc (right):
> 
>
https://codereview.chromium.org/389633002/diff/80001/apps/shell/browser/shell...
> apps/shell/browser/shell_extensions_browser_client.cc:250: // this method.
> I'm confused - is this meant to be implemented?
> 
> You could work within the assumption that there's only one browser context
> (browser_context_) in app_shell, and dispatch it to just its event router.
> 

Thanks for the suggestion. Works just fine now and can detect removable storage
events.

>
https://codereview.chromium.org/389633002/diff/80001/chrome/browser/extension...
> File
>
chrome/browser/extensions/api/system_display/display_info_provider_chromeos_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/389633002/diff/80001/chrome/browser/extension...
>
chrome/browser/extensions/api/system_display/display_info_provider_chromeos_unittest.cc:1:
> // Copyright 2013 The Chromium Authors. All rights reserved.
> It looks like this file could be moved to src/extensions also.
> 
>
https://codereview.chromium.org/389633002/diff/80001/extensions/browser/exten...
> File extensions/browser/extensions_browser_client.h (right):
> 
>
https://codereview.chromium.org/389633002/diff/80001/extensions/browser/exten...
> extensions/browser/extensions_browser_client.h:183: // Propage a event to all
> the renderers. The implementation must be safe to
> typo: Propagate
> I would write: "all the renderers in every browser context." (strictly
speaking,
> not true because of incognito profiles, but I think that's more than we want
to
> get into in this comment)

All issues addressed, thanks for reviewing.

[tmpsantos, 2014-07-17 11:39:01]

+derat for incoming /ash dependency.

[Daniel Erat, 2014-07-17 13:51:52]

https://codereview.chromium.org/389633002/diff/200001/extensions/browser/DEPS
File extensions/browser/DEPS (right):

https://codereview.chromium.org/389633002/diff/200001/extensions/browser/DEPS...
extensions/browser/DEPS:2: "+ash",
extensions/browser/ shouldn't depend on ash/. the code that needs this should
probably use a delegate that can be passed in from outside.

[tmpsantos, 2014-07-17 14:39:04]

https://codereview.chromium.org/389633002/diff/200001/extensions/browser/DEPS
File extensions/browser/DEPS (right):

https://codereview.chromium.org/389633002/diff/200001/extensions/browser/DEPS...
extensions/browser/DEPS:2: "+ash",
On 2014/07/17 13:51:52, Daniel Erat wrote:
> extensions/browser/ shouldn't depend on ash/. the code that needs this should
> probably use a delegate that can be passed in from outside.

The idea of the display_info_provider_[platform].cc is to handle the platform
specific bits, that is why it is talking directly to the window manager instead
of going through layers of indirection. If we don't do this here, the same
delegate would need to be implemented for Chrome and app_shell
(win/mac/linux/etc).

If the dependency on ash/ is still not acceptable, is it possible to fix this in
a separated CL? That would require a big refactoring of
display_info_provider_chromeos.cc which is not exactly related to this change.

[Daniel Erat, 2014-07-17 15:40:04]

the ash dependency should be pulled out in a separate change before this one
goes in -- is that what you're suggesting?

http://crbug.com/392842 cites app_shell as the motivation for this change.
app_shell doesn't include or initialize ash, in case that wasn't clear.

[James Cook, 2014-07-17 17:17:10]

On 2014/07/17 15:40:04, Daniel Erat wrote:
> the ash dependency should be pulled out in a separate change before this one
> goes in -- is that what you're suggesting?
> 
> http://crbug.com/392842 cites app_shell as the motivation for this change.
> app_shell doesn't include or initialize ash, in case that wasn't clear.

Yeah, app_shell can't depend on ash/, as one of the goals of app_shell is to be
small on disk. Linking in ash/ will link in all of ui/views/, which is large.
ash also includes many UI resources (images) that app_shell doesn't need.

I'm happy to see the system APIs coming into src/extensions!

[tmpsantos, 2014-07-17 18:55:49]

On 2014/07/17 17:17:10, James Cook wrote:
> On 2014/07/17 15:40:04, Daniel Erat wrote:
> > the ash dependency should be pulled out in a separate change before this one
> > goes in -- is that what you're suggesting?
> > 
> > http://crbug.com/392842 cites app_shell as the motivation for this change.
> > app_shell doesn't include or initialize ash, in case that wasn't clear.
> 
> Yeah, app_shell can't depend on ash/, as one of the goals of app_shell is to
be
> small on disk. Linking in ash/ will link in all of ui/views/, which is large.
> ash also includes many UI resources (images) that app_shell doesn't need.
> 
> I'm happy to see the system APIs coming into src/extensions!

Hi all, thanks for the comments.

If I understood correctly, what I should do to go ahead with this CL is (in a
another CL) remove the direct dependency on ash/ by creating a delegate that
Chromium will implement and app_shell will not (or will have a no-op
implementation). This has the side effect of removing a few features of the API
when building the app_shell with ChromeOS as a target.

Did I get it right?

[Daniel Erat, 2014-07-17 22:53:58]

On 2014/07/17 18:55:49, tmpsantos wrote:
> On 2014/07/17 17:17:10, James Cook wrote:
> > On 2014/07/17 15:40:04, Daniel Erat wrote:
> > > the ash dependency should be pulled out in a separate change before this
one
> > > goes in -- is that what you're suggesting?
> > > 
> > > http://crbug.com/392842 cites app_shell as the motivation for this change.
> > > app_shell doesn't include or initialize ash, in case that wasn't clear.
> > 
> > Yeah, app_shell can't depend on ash/, as one of the goals of app_shell is to
> be
> > small on disk. Linking in ash/ will link in all of ui/views/, which is
large.
> > ash also includes many UI resources (images) that app_shell doesn't need.
> > 
> > I'm happy to see the system APIs coming into src/extensions!
> 
> Hi all, thanks for the comments.
> 
> If I understood correctly, what I should do to go ahead with this CL is (in a
> another CL) remove the direct dependency on ash/ by creating a delegate that
> Chromium will implement and app_shell will not (or will have a no-op
> implementation). This has the side effect of removing a few features of the
API
> when building the app_shell with ChromeOS as a target.
> 
> Did I get it right?

james may have opinions about whether this is still the right approach to take,
but in the past, we've used the ExtensionsBrowserClient interface to provide
alternate chrome and app_shell implementations of methods needed by code under
extensions/. it's a bit of a dumping ground at this point, though, so perhaps
it's no longer the best choice...

[tmpsantos, 2014-08-30 00:09:34]

On 2014/07/17 22:53:58, Daniel Erat wrote:
> On 2014/07/17 18:55:49, tmpsantos wrote:
> > On 2014/07/17 17:17:10, James Cook wrote:
> > > On 2014/07/17 15:40:04, Daniel Erat wrote:
> > > > the ash dependency should be pulled out in a separate change before this
> one
> > > > goes in -- is that what you're suggesting?
> > > > 
> > > > http://crbug.com/392842 cites app_shell as the motivation for this
change.
> > > > app_shell doesn't include or initialize ash, in case that wasn't clear.
> > > 
> > > Yeah, app_shell can't depend on ash/, as one of the goals of app_shell is
to
> > be
> > > small on disk. Linking in ash/ will link in all of ui/views/, which is
> large.
> > > ash also includes many UI resources (images) that app_shell doesn't need.
> > > 
> > > I'm happy to see the system APIs coming into src/extensions!
> > 
> > Hi all, thanks for the comments.
> > 
> > If I understood correctly, what I should do to go ahead with this CL is (in
a
> > another CL) remove the direct dependency on ash/ by creating a delegate that
> > Chromium will implement and app_shell will not (or will have a no-op
> > implementation). This has the side effect of removing a few features of the
> API
> > when building the app_shell with ChromeOS as a target.
> > 
> > Did I get it right?
> 
> james may have opinions about whether this is still the right approach to
take,
> but in the past, we've used the ExtensionsBrowserClient interface to provide
> alternate chrome and app_shell implementations of methods needed by code under
> extensions/. it's a bit of a dumping ground at this point, though, so perhaps
> it's no longer the best choice...

New patch for review. This time with no dependency on ash/

[Daniel Erat, 2014-08-30 01:44:20]

lgtm

https://codereview.chromium.org/389633002/diff/240001/extensions/browser/api/...
File extensions/browser/api/system_memory/memory_info_provider.h (right):

https://codereview.chromium.org/389633002/diff/240001/extensions/browser/api/...
extensions/browser/api/system_memory/memory_info_provider.h:18: const
core_api::system_memory::MemoryInfo& memory_info() const;
nit: inline simple accessors like this one

https://codereview.chromium.org/389633002/diff/240001/extensions/shell/browse...
File extensions/shell/browser/shell_extensions_browser_client.cc (right):

https://codereview.chromium.org/389633002/diff/240001/extensions/shell/browse...
extensions/shell/browser/shell_extensions_browser_client.cc:216:
scoped_ptr<extensions::Event> event(
nit: you shouldn't need "extensions::" here or on the next two lines

[Lei Zhang, 2014-08-30 03:11:49]

chrome/ and device/ lgtm

[Lei Zhang, 2014-08-30 03:12:28]

On 2014/08/30 03:11:49, Lei Zhang wrote:
> chrome/ and device/ lgtm

Just like before. :)

[tmpsantos, 2014-09-02 13:58:05]

thiago.santos@intel.com changed reviewers:
+ jamescook@chromium.org

[tmpsantos, 2014-09-03 21:23:17]

+jamescook, PTAL

[miket_OOO, 2014-09-03 21:43:03]

lgtm

[James Cook, 2014-09-03 22:22:58]

Looks good but a couple questions about init/shutdown.

https://codereview.chromium.org/389633002/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/browser_context_keyed_service_factories.cc
(right):

https://codereview.chromium.org/389633002/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/browser_context_keyed_service_factories.cc:153:
extensions::SystemInfoAPI::GetFactoryInstance();
Should this go in extensions/browser/browser_context_keyed_service_factories.cc?
 It probably should live there if app_shell is going to use it some day.

https://codereview.chromium.org/389633002/diff/320001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/389633002/diff/320001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:137:
SystemInfoAPI::GetFactoryInstance();
This should probably go in
extensions::EnsureBrowserContextKeyedServiceFactoriesBuilt().

https://codereview.chromium.org/389633002/diff/320001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:164:
storage_monitor::StorageMonitor::Create();
It kinda feels to me like this should be a little earlier.
browser_main_delegate_->Start() seems like it should be more-or-less the last
thing this function does.

https://codereview.chromium.org/389633002/diff/320001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:191:
storage_monitor::StorageMonitor::Destroy();
Please destroy things in the opposite order of creation. Either StorageMonitor
should be created earlier in startup, or it should be destroyed earlier in
shutdown.

[tmpsantos, 2014-09-04 12:51:58]

Thanks for reviewing!

https://codereview.chromium.org/389633002/diff/320001/chrome/browser/extensio...
File chrome/browser/extensions/browser_context_keyed_service_factories.cc
(right):

https://codereview.chromium.org/389633002/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/browser_context_keyed_service_factories.cc:153:
extensions::SystemInfoAPI::GetFactoryInstance();
On 2014/09/03 22:22:58, James Cook wrote:
> Should this go in
extensions/browser/browser_context_keyed_service_factories.cc?
>  It probably should live there if app_shell is going to use it some day.

It totally should!

https://codereview.chromium.org/389633002/diff/320001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/389633002/diff/320001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:137:
SystemInfoAPI::GetFactoryInstance();
On 2014/09/03 22:22:58, James Cook wrote:
> This should probably go in
> extensions::EnsureBrowserContextKeyedServiceFactoriesBuilt().

Acknowledged.

https://codereview.chromium.org/389633002/diff/320001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:164:
storage_monitor::StorageMonitor::Create();
On 2014/09/03 22:22:58, James Cook wrote:
> It kinda feels to me like this should be a little earlier.
> browser_main_delegate_->Start() seems like it should be more-or-less the last
> thing this function does.

Acknowledged.

https://codereview.chromium.org/389633002/diff/320001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:191:
storage_monitor::StorageMonitor::Destroy();
On 2014/09/03 22:22:58, James Cook wrote:
> Please destroy things in the opposite order of creation. Either StorageMonitor
> should be created earlier in startup, or it should be destroyed earlier in
> shutdown.

I will create it on app_shell before the DesktopController and destroy it right
after DesktopController is destroyed. The creation time is not very important
for this component because it is done asynchronously and when used for the first
time, you still have to subscribe to StorageMonitor::EnsureInitialized. But yes,
it is a good practice to have the init order consistent (and I should probably
fix that on chrome in a separated patch). Thanks for pointing it out.

[James Cook, 2014-09-04 16:49:53]

LGTM

[tmpsantos, 2014-09-05 08:46:57]

The CQ bit was checked by thiago.santos@intel.com

[commit-bot: I haz the power, 2014-09-05 08:47:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thiago.santos@intel.com/389633002/360001

[commit-bot: I haz the power, 2014-09-05 08:56:57]

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

[commit-bot: I haz the power, 2014-09-05 08:56:59]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/52066)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tmpsantos, 2014-09-05 12:28:03]

The CQ bit was checked by thiago.santos@intel.com

[commit-bot: I haz the power, 2014-09-05 12:28:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thiago.santos@intel.com/389633002/380001

[commit-bot: I haz the power, 2014-09-05 13:30:38]

Committed patchset #12 (id:380001) as bf713ae07c7c6711d80f0d78067b91e7e96bef4e

[commit-bot: I haz the power, 2014-09-10 03:39:14]

Patchset 12 (id:??) landed as
https://crrev.com/89e236b4594095c089b53d9cd4d521eb81860721
Cr-Commit-Position: refs/heads/master@{#293512}