Add the basic code skeleton for system info event router

Add the basic code skeleton for system info event router.

BUG=136519
TEST=browser_tests --gtest_filter=SystemInfoStorageApiTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154770

[Hongbo Min, 2012-08-20 15:56:45]

This patch is to add a general SystemInfoWatcher interface to watch the system
info change events.

It provides a mock implementation for api test. Currently, only
onAvailableCapacityChanged is supported now. But in future, all events in
systemInfo namespace can be supported in SystemInfoWatcher implementations for
different platforms.

Pls have a review. Thanks a lot.

[Hongbo Min, 2012-08-23 00:29:19]

On 2012/08/20 23:33:57, Hongbo Min wrote:

Mihai,

Could you have a time to review this patch? Thanks.

[Mihai Parparita -not on Chrome, 2012-08-23 00:45:52]

As I mentioned in the other code review, it seems like SystemInfoWatcher is very
similar to SystemMonitor. Is there a reason why you can't use the latter?

http://codereview.chromium.org/10836341/diff/6001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc
(right):

http://codereview.chromium.org/10836341/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:15:
virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
Nit: add an empty newline before each new non-trivial inline method.

http://codereview.chromium.org/10836341/diff/6001/chrome/browser/extensions/e...
File chrome/browser/extensions/event_router.cc (right):

http://codereview.chromium.org/10836341/diff/6001/chrome/browser/extensions/e...
chrome/browser/extensions/event_router.cc:166:
event_names::kOnStorageAvailableCapacityChanged) == 0 ||
You may want to encapsulate this in a
SystemInfoEvent::IsSystemInfoEvent(event_name) method, so that this class
doesn't have to be touched every time a new event is added.

[Hongbo Min, 2012-08-23 15:06:36]

On 2012/08/23 00:45:52, Mihai Parparita wrote:
> As I mentioned in the other code review, it seems like SystemInfoWatcher is
very
> similar to SystemMonitor. Is there a reason why you can't use the latter?
> 
>
http://codereview.chromium.org/10836341/diff/6001/chrome/browser/extensions/a...
> File
>
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc
> (right):
> 
>
http://codereview.chromium.org/10836341/diff/6001/chrome/browser/extensions/a...
>
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:15:
> virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
> Nit: add an empty newline before each new non-trivial inline method.
> 
>
http://codereview.chromium.org/10836341/diff/6001/chrome/browser/extensions/e...
> File chrome/browser/extensions/event_router.cc (right):
> 
>
http://codereview.chromium.org/10836341/diff/6001/chrome/browser/extensions/e...
> chrome/browser/extensions/event_router.cc:166:
> event_names::kOnStorageAvailableCapacityChanged) == 0 ||
> You may want to encapsulate this in a
> SystemInfoEvent::IsSystemInfoEvent(event_name) method, so that this class
> doesn't have to be touched every time a new event is added.

Yes, we can leverage the SystemMonitor for systemInfo API. I will update the
patch soon. Thanks.

[Hongbo Min, 2012-08-27 15:08:23]

Mihai,

The changes in the latest patch are:
1. Remove SystemInfoWatcher interface and implement the
StartWatching/StopWatching in StorageInfoProvider.

2. Reuse SystemMonitor for observing media device arrival/removal event. 

3. Provide an implementation on Windows for detecting storage free space changes
event. It is not a functionality covered by SystemMonitor.

TODOs in the source code are the next things I need to complete. Please have a
review. Thanks a lot.

http://codereview.chromium.org/10836341/diff/12009/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h
(right):

http://codereview.chromium.org/10836341/diff/12009/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:29: :
public base::SystemMonitor::DevicesChangedObserver,
Reuse SystemMonitor to detect the media device arrival/removal event.

http://codereview.chromium.org/10836341/diff/12009/chrome/browser/extensions/...
File
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.h
(right):

http://codereview.chromium.org/10836341/diff/12009/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider_win.h:20:
public:
Unit test for StorageInfoProviderWin will be submitted soon.

[Mihai Parparita -not on Chrome, 2012-08-28 01:00:11]

On Mon, Aug 27, 2012 at 8:08 AM, <hongbo.min@intel.com> wrote:

> 3. Provide an implementation on Windows for detecting storage free space
> changes
> event. It is not a functionality covered by SystemMonitor.
>

Why not extend SystemMonitor with that capability? It seems odd to have the
storage monitoring be split across two classes in very different code
locations.

Mihai

[Mihai Parparita -not on Chrome, 2012-08-28 01:02:16]

BTW, system_info_event_router.cc/h appears to have disappeared from this patch
(but it's still referenced).

[Hongbo Min, 2012-08-28 01:10:27]

On 2012/08/28 01:02:16, Mihai Parparita wrote:
> BTW, system_info_event_router.cc/h appears to have disappeared from this patch
> (but it's still referenced).

Sorry, forget to git add these two files.

[Hongbo Min, 2012-08-28 01:31:00]

On 2012/08/28 01:00:11, Mihai Parparita wrote:
> On Mon, Aug 27, 2012 at 8:08 AM, <mailto:hongbo.min@intel.com> wrote:
> 
> > 3. Provide an implementation on Windows for detecting storage free space
> > changes
> > event. It is not a functionality covered by SystemMonitor.
> >
> 
> Why not extend SystemMonitor with that capability? It seems odd to have the
> storage monitoring be split across two classes in very different code
> locations.
> 
> Mihai

Actually, SystemMonitor doesn't perform any monitoring work, it is just a
message processor via exposing several observer interfaces and Process* methods
to notify all kinds of Observers.

The chrome/browser/media_gallery component is monitoring the system and watching
the device change events. Once it receives a device change events
(arrival/removal), it will call SystemMonitor::Process* method to notify the all
observers, like StorageInfoProvider here.

[Hongbo Min, 2012-08-29 10:21:41]

Mihaip,

Since it depends on the http://crbug.com/145400, this updated patch is just to
provide a basic code skeleton for SystemInfoEventRouter.

Pls have a review.

[Hongbo Min, 2012-08-31 00:08:45]

On 2012/08/29 23:55:40, Hongbo Min wrote:

Mihai,

What is your opinion about this patch? Although there is a dependency on
SystemMonitor refactor, I think it is independent of SystemMonitor now. The
event in CPU info api needs to SystemInfoEventRouter. Do you have suggestions
about how to go ahead for system info event? Thanks.

[Mihai Parparita -not on Chrome, 2012-08-31 01:00:59]

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc
(right):

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:18:
const int kDefaultInterval = 200; // default interval is 200ms.
Instead of needing a comment, can you make the constant name kDefaultIntervalMs
so that it's more obvious from the name what the units are?

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:19:
const int kAvailableCapacityBase = 10000;
Ditto here (add a "Bytes" suffix to the name) and below.

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:65:
delete timer_;
Since you only call StopWatching from the destructor, it's preferable to make
timer_ into a scoped_ptr and just let its destruction stop the timer.

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:81:
bool  is_watching_;
Nit: extra space after bool.

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/system_info_event_router.cc (right):

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/system_info_event_router.cc:91:
base::Unretained(this), id, available_capacity));
Add an early return here, so that the rest of the body of the function doesn't
have to be indented.

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/system_info_event_router.h (right):

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/system_info_event_router.h:19: //
http://crbug.com/145400 is fixed, We need to update SystemInfoEventRouter
Nit: lowercase the "We".

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/system_info_event_router.h:48: typedef
std::map<Profile*, std::set<std::string> > EventListenerMap;
I'm concerned about holding on to Profile pointers, since profiles can be loaded
and unloaded.

Instead of that optimization, why not get the ExtensionEventRouter for the
default profile (which is always loaded) and then call DispatchEventToRenderers
without a restricted profile, which will dispatch that event to all extensions
in all profiles that have a listener attached (similar to
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...).
EventRouter::DispatchEventImpl ends up doing filtering before sending IPCs, so
it's not inefficient to send it to all profiles from your code.

[Hongbo Min, 2012-08-31 15:29:53]

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc
(right):

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:18:
const int kDefaultInterval = 200; // default interval is 200ms.
On 2012/08/31 01:01:00, Mihai Parparita wrote:
> Instead of needing a comment, can you make the constant name
kDefaultIntervalMs
> so that it's more obvious from the name what the units are?

Done.

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:19:
const int kAvailableCapacityBase = 10000;
On 2012/08/31 01:01:00, Mihai Parparita wrote:
> Ditto here (add a "Bytes" suffix to the name) and below.

Done.

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:65:
delete timer_;
On 2012/08/31 01:01:00, Mihai Parparita wrote:
> Since you only call StopWatching from the destructor, it's preferable to make
> timer_ into a scoped_ptr and just let its destruction stop the timer.

Done.

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:81:
bool  is_watching_;
On 2012/08/31 01:01:00, Mihai Parparita wrote:
> Nit: extra space after bool.

Done.

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/system_info_event_router.cc (right):

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/system_info_event_router.cc:91:
base::Unretained(this), id, available_capacity));
On 2012/08/31 01:01:00, Mihai Parparita wrote:
> Add an early return here, so that the rest of the body of the function doesn't
> have to be indented.

Done.

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/system_info_event_router.h (right):

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/system_info_event_router.h:19: //
http://crbug.com/145400 is fixed, We need to update SystemInfoEventRouter
On 2012/08/31 01:01:00, Mihai Parparita wrote:
> Nit: lowercase the "We".

Done.

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/system_info_event_router.h:48: typedef
std::map<Profile*, std::set<std::string> > EventListenerMap;
On 2012/08/31 01:01:00, Mihai Parparita wrote:
> I'm concerned about holding on to Profile pointers, since profiles can be
loaded
> and unloaded.
> 
> Instead of that optimization, why not get the ExtensionEventRouter for the
> default profile (which is always loaded) and then call
DispatchEventToRenderers
> without a restricted profile, which will dispatch that event to all extensions
> in all profiles that have a listener attached (similar to
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...).
> EventRouter::DispatchEventImpl ends up doing filtering before sending IPCs, so
> it's not inefficient to send it to all profiles from your code.

The ProfileManager::GetDefaultProfile is deprecated as commented in
profile_manager.h.

According to
http://www.chromium.org/developers/design-documents/multiple-profiles, it says: 
"You should no longer use it to get a "Default" profile, because this concept
will in the future no longer exist."


So I use a map to record which events are listened on each Profile instance
loaded, and dispatch event using event router for each Profile instead of using
that for Default Profile.

I add IsValideProfile to check whether the profile is valid to avoid referencing
a unloaded Profile instance you concerned.

[Mihai Parparita -not on Chrome, 2012-08-31 21:37:38]

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
File chrome/browser/extensions/system_info_event_router.h (right):

http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
chrome/browser/extensions/system_info_event_router.h:48: typedef
std::map<Profile*, std::set<std::string> > EventListenerMap;
On 2012/08/31 15:29:53, Hongbo Min wrote:
> The ProfileManager::GetDefaultProfile is deprecated as commented in
> profile_manager.h.

True. However, I'd still like to not worry about Profiles if at all possible. It
looks like the better way is to use EventRouterForwarder, which has a
BroadcastEventToRenderers method.

(you can get at the event router via
g_browser_process->extension_event_router_forwarder()

[Hongbo Min, 2012-09-01 00:06:36]

On 2012/08/31 21:37:38, Mihai Parparita wrote:
>
http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
> File chrome/browser/extensions/system_info_event_router.h (right):
> 
>
http://codereview.chromium.org/10836341/diff/28001/chrome/browser/extensions/...
> chrome/browser/extensions/system_info_event_router.h:48: typedef
> std::map<Profile*, std::set<std::string> > EventListenerMap;
> On 2012/08/31 15:29:53, Hongbo Min wrote:
> > The ProfileManager::GetDefaultProfile is deprecated as commented in
> > profile_manager.h.
> 
> True. However, I'd still like to not worry about Profiles if at all possible.
It
> looks like the better way is to use EventRouterForwarder, which has a
> BroadcastEventToRenderers method.
> 
> (you can get at the event router via
> g_browser_process->extension_event_router_forwarder()

Thanks for your hint. The patch is already updated. Pls review again.

[Mihai Parparita -not on Chrome, 2012-09-01 00:20:53]

LGTM

http://codereview.chromium.org/10836341/diff/37001/chrome/browser/extensions/...
File chrome/browser/extensions/system_info_event_router.h (right):

http://codereview.chromium.org/10836341/diff/37001/chrome/browser/extensions/...
chrome/browser/extensions/system_info_event_router.h:45: typedef
std::map<std::string, int> EventWatchingMap;
Nit: I think this can just be a std::multiset<std::string>.

http://codereview.chromium.org/10836341/diff/37001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/systeminfo/storage/test_storage_api.js
(right):

http://codereview.chromium.org/10836341/diff/37001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/systeminfo/storage/test_storage_api.js:32:
if (++ numOfChangedEvent > 5)
Nit: no space after ++.

[commit-bot: I haz the power, 2012-09-02 02:37:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10836341/28011

[commit-bot: I haz the power, 2012-09-02 02:38:00]

Presubmit check for 10836341-28011 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Hongbo Min, 2012-09-02 02:47:12]

Nico, need your LGTM for gyp changes. Thanks.

http://codereview.chromium.org/10836341/diff/37001/chrome/browser/extensions/...
File chrome/browser/extensions/system_info_event_router.h (right):

http://codereview.chromium.org/10836341/diff/37001/chrome/browser/extensions/...
chrome/browser/extensions/system_info_event_router.h:45: typedef
std::map<std::string, int> EventWatchingMap;
On 2012/09/01 00:20:53, Mihai Parparita wrote:
> Nit: I think this can just be a std::multiset<std::string>.

Done.

http://codereview.chromium.org/10836341/diff/37001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/systeminfo/storage/test_storage_api.js
(right):

http://codereview.chromium.org/10836341/diff/37001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/systeminfo/storage/test_storage_api.js:32:
if (++ numOfChangedEvent > 5)
On 2012/09/01 00:20:53, Mihai Parparita wrote:
> Nit: no space after ++.

Done.

[Nico, 2012-09-04 13:43:31]

gypi ligtm

[Nico, 2012-09-04 13:43:42]

On 2012/09/04 13:43:31, Nico wrote:
> gypi ligtm

*lgtm

[commit-bot: I haz the power, 2012-09-04 14:53:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10836341/28011

[commit-bot: I haz the power, 2012-09-04 17:37:39]

Change committed as 154770