Implement systemInfo.storage.onChanged event

Implement systemInfo.storage.onChanged event. It provides extra option to specify
which storage to be listened and the change threshold for free space change.


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

[Hongbo Min, 2012-12-03 02:27:04]

benwells@, a new patch for storage.onChanged event implementation. Pls have a
review.

Nico, please review the gyp change.

[Hongbo Min, 2012-12-04 23:07:11]

ping benwells@... Thanks.

[benwells, 2012-12-06 00:19:30]

My reading of this is that there are two sets of listeners / observers. One in
the provider doesn't know about the threshold, the one in the event router does.
Why not just have one set of listeners in the provider that does know about
threshold?

[Hongbo Min, 2012-12-06 01:17:00]

On 2012/12/06 00:19:30, benwells wrote:
> My reading of this is that there are two sets of listeners / observers. One in
> the provider doesn't know about the threshold, the one in the event router
does.
> Why not just have one set of listeners in the provider that does know about
> threshold?

I ever considered using one set of listeners. My initial thought is, since the
threshold is something about watching policy, and for StorageInfoProvider, its
responsibility is just to provide the info data without being aware of  any
policy about how to deal with the free space changes.

Yes, using one set of listeners would make the code simple.

[Hongbo Min, 2012-12-06 06:47:45]

On 2012/12/06 01:17:00, Hongbo Min wrote:
> On 2012/12/06 00:19:30, benwells wrote:
> > My reading of this is that there are two sets of listeners / observers. One
in
> > the provider doesn't know about the threshold, the one in the event router
> does.
> > Why not just have one set of listeners in the provider that does know about
> > threshold?
> 
> I ever considered using one set of listeners. My initial thought is, since the
> threshold is something about watching policy, and for StorageInfoProvider, its
> responsibility is just to provide the info data without being aware of  any
> policy about how to deal with the free space changes.
> 
> Yes, using one set of listeners would make the code simple.

benwells@, I have just spent several time to re-consider the current
implementation and the possibility to combine one set of listeners. The reason
why they are two set is their differences and responsibility.

The listener used in event router is different from the observer in storage info
provider. The listener in event router represents a watcher from render process
which is managed by SystemInfoEventRouter. Since the current implementation for
event filter is encoded into event name (onChanged#<id>#threshold), which should
be transparent to storage info provider.

The observer used in storage info provider represents an object that shows
interests in free space change and provide a callback at the time the free space
change.

One optimization for SystemInfoEventRouter::OnStorageFreeSpaceChanged  is to
reduce the unnecessary rounds from FILE thread to UI thread if the change is
less than threshold.

Do you think it is worthy combining the listener and the observer?

[benwells, 2012-12-06 06:54:23]

On 2012/12/06 06:47:45, Hongbo Min wrote:
> On 2012/12/06 01:17:00, Hongbo Min wrote:
> > On 2012/12/06 00:19:30, benwells wrote:
> > > My reading of this is that there are two sets of listeners / observers.
One
> in
> > > the provider doesn't know about the threshold, the one in the event router
> > does.
> > > Why not just have one set of listeners in the provider that does know
about
> > > threshold?
> > 
> > I ever considered using one set of listeners. My initial thought is, since
the
> > threshold is something about watching policy, and for StorageInfoProvider,
its
> > responsibility is just to provide the info data without being aware of  any
> > policy about how to deal with the free space changes.
> > 
> > Yes, using one set of listeners would make the code simple.
> 
> benwells@, I have just spent several time to re-consider the current
> implementation and the possibility to combine one set of listeners. The reason
> why they are two set is their differences and responsibility.
> 
> The listener used in event router is different from the observer in storage
info
> provider. The listener in event router represents a watcher from render
process
> which is managed by SystemInfoEventRouter. Since the current implementation
for
> event filter is encoded into event name (onChanged#<id>#threshold), which
should
> be transparent to storage info provider.
> 
> The observer used in storage info provider represents an object that shows
> interests in free space change and provide a callback at the time the free
space
> change.
> 
> One optimization for SystemInfoEventRouter::OnStorageFreeSpaceChanged  is to
> reduce the unnecessary rounds from FILE thread to UI thread if the change is
> less than threshold.
> 
> Do you think it is worthy combining the listener and the observer?

If it makes sense separate, keep it separate. I'll take another look.

[Hongbo Min, 2012-12-06 08:07:30]

benwells@, I am also refactoring the implementation. It might be better if you
could delay the review until I upload a new patch set.

[Hongbo Min, 2012-12-06 15:29:43]

benwells@, the patch is updated.

The main change is to move StorageEventListener out of event router and add
FreeSpaceWatcher into storage info provider. The motivation is to avoid
unnecessary rounds from FILE thread to UI thread for posting a task to notify
the free space changes. The StorageInfoObserver receives free space change
notification only if the change bytes is equal to or greater than threshold.

The browser_tests and unit_tests both are passed on my machine.

[Hongbo Min, 2012-12-07 03:12:49]

Refined patch. Pls have a review, benwells. Thanks.

[Hongbo Min, 2012-12-11 04:36:47]

On 2012/12/07 03:12:49, Hongbo Min wrote:
> Refined patch. Pls have a review, benwells. Thanks.

ping benwells@...

[benwells, 2012-12-11 04:40:40]

Apologies for the slow review.

https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:97:
FreeSpaceWatcher watcher(id, threshold);
This can move to line 111.

https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:101:
if (it->threshold == threshold)
What if two extensions start watching using the same threshold? The second one
will be ignored: is that a problem?

https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:177:
it->threshold /* change threshold */);
Why do we pass the threshold back in the event?

https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h
(right):

https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:69:
class FreeSpaceWatcher {
May as well be a struct.

https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:75:
std::string id;
Is the id needed in this class / struct?

https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:79:
double recent_free_space;
I think this is actually the last free space reported to the listener. If it is,
please name the variable appropriately (e.g. last_reported_free_space).

https://codereview.chromium.org/11419279/diff/21001/chrome/common/extensions/...
File chrome/common/extensions/api/experimental_system_info_storage.idl (right):

https://codereview.chromium.org/11419279/diff/21001/chrome/common/extensions/...
chrome/common/extensions/api/experimental_system_info_storage.idl:60: // In
addtional to specifying a callback function, you have to specify
nit: addition

[Hongbo Min, 2012-12-11 05:08:50]

Thanks for your comment. Will update it later.

https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:101:
if (it->threshold == threshold)
On 2012/12/11 04:40:40, benwells wrote:
> What if two extensions start watching using the same threshold? The second one
> will be ignored: is that a problem?

Good catch! It will lead to the second one can not receive event after the first
one stops watching.

https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:177:
it->threshold /* change threshold */);
On 2012/12/11 04:40:40, benwells wrote:
> Why do we pass the threshold back in the event?

The SystemInfoEventRouter needs it to marshal the event name being dispatched to
renderer process.

[benwells, 2013-05-15 22:28:56]

On 2012/12/11 05:08:50, Hongbo Min wrote:
> Thanks for your comment. Will update it later.
> 
>
https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
> File
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
> (right):
> 
>
https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
>
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:101:
> if (it->threshold == threshold)
> On 2012/12/11 04:40:40, benwells wrote:
> > What if two extensions start watching using the same threshold? The second
one
> > will be ignored: is that a problem?
> 
> Good catch! It will lead to the second one can not receive event after the
first
> one stops watching.
> 
>
https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
>
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:177:
> it->threshold /* change threshold */);
> On 2012/12/11 04:40:40, benwells wrote:
> > Why do we pass the threshold back in the event?
> 
> The SystemInfoEventRouter needs it to marshal the event name being dispatched
to
> renderer process.

What's happening with this CL?

[Hongbo Min, 2013-05-16 01:17:33]

On 2013/05/15 22:28:56, benwells wrote:
> On 2012/12/11 05:08:50, Hongbo Min wrote:
> > Thanks for your comment. Will update it later.
> > 
> >
>
https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
> > File
> chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
> >
>
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:101:
> > if (it->threshold == threshold)
> > On 2012/12/11 04:40:40, benwells wrote:
> > > What if two extensions start watching using the same threshold? The second
> one
> > > will be ignored: is that a problem?
> > 
> > Good catch! It will lead to the second one can not receive event after the
> first
> > one stops watching.
> > 
> >
>
https://codereview.chromium.org/11419279/diff/21001/chrome/browser/extensions...
> >
>
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:177:
> > it->threshold /* change threshold */);
> > On 2012/12/11 04:40:40, benwells wrote:
> > > Why do we pass the threshold back in the event?
> > 
> > The SystemInfoEventRouter needs it to marshal the event name being
dispatched
> to
> > renderer process.
> 
> What's happening with this CL?

It was already done in another CL. I will close it.