Cashew: implement backend usage API

Cashew: implement backend usage API

BUG=chromium-os:7330
TEST=libcros/monitor_network and manual testing

Change-Id: I65e0035c0cc83ee3343b9e66e9a76ad553fe04ba

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=b7ed6d3

[Jason Glasgow, 2010-10-11 18:56:40]

Here are the comments I have so far.  Nothing more than tiny nits.  Thanks for
commenting the code well.  It makes it much easier to review.  I'll finish up
real soon.

http://codereview.chromium.org/3528016/diff/13002/31003
File src/cashew_server.cc (right):

http://codereview.chromium.org/3528016/diff/13002/31003#newcode21
src/cashew_server.cc:21: ServiceManager * const service_manager)
I'm not sure why const is important here (but the style guide seems to recommend
it).

http://codereview.chromium.org/3528016/diff/13002/31006
File src/data_plan.h (right):

http://codereview.chromium.org/3528016/diff/13002/31006#newcode106
src/data_plan.h:106: const char* TypeToLibcrosString(Type) const;
Should this give a name to the parameter?  (Type type)  It is more consistent
with other function declarations.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

All parameters should be named, with identical names in the declaration and
implementation.

http://codereview.chromium.org/3528016/diff/13002/31007
File src/data_plan_provider.cc (right):

http://codereview.chromium.org/3528016/diff/13002/31007#newcode124
src/data_plan_provider.cc:124: Value *parsed_usage_update =
We often use scoped_ptr<> to ensure that delete is called.

http://codereview.chromium.org/3528016/diff/13002/31008
File src/data_plan_provider.h (right):

http://codereview.chromium.org/3528016/diff/13002/31008#newcode77
src/data_plan_provider.h:77: HttpFetcher * const fetcher_;
ScopedPtr<> ?  It will simplify the destructor.  But either way is fine.

http://codereview.chromium.org/3528016/diff/13002/31014
File src/libcurl_http_fetcher.cc (right):

http://codereview.chromium.org/3528016/diff/13002/31014#newcode34
src/libcurl_http_fetcher.cc:34: url_ = url;
Shouldn't url_ and url be the same?  If doesn't make sense to resume if they are
different.  (Hmm, I just noticed that resume is used from BeginTransfer).

[Jason Glasgow, 2010-10-11 20:17:26]

LGTM

http://codereview.chromium.org/3528016/diff/13002/31018
File src/policy.h (right):

http://codereview.chromium.org/3528016/diff/13002/31018#newcode26
src/policy.h:26: virtual guint GetUpdateTimerIdleSecs() const = 0;
Theoretically, I'm not sure that the idle timer should be constant over the life
of the service.  When the user has 5G left in a plan, we might do this
infrequently, and when the user has 50M left, we might request updates more
frequently.  But lets go with this design until we have a compelling practical
reason to change.

[Vince Laviano, 2010-10-11 22:03:09]

http://codereview.chromium.org/3528016/diff/13002/31003
File src/cashew_server.cc (right):

http://codereview.chromium.org/3528016/diff/13002/31003#newcode21
src/cashew_server.cc:21: ServiceManager * const service_manager)
On 2010/10/11 18:56:40, Jason Glasgow wrote:
> I'm not sure why const is important here (but the style guide seems to
recommend
> it).

The style guide prohibits non-const references as args (and a const reference
wouldn't do since ServiceManager::SetCashewServer is not a const method), so the
const pointer to a non-const Service Manager is a way of ensuring that
CashewServer never NULLs out this pointer after its initial assignment here in
the constructor.

http://codereview.chromium.org/3528016/diff/13002/31006
File src/data_plan.h (right):

http://codereview.chromium.org/3528016/diff/13002/31006#newcode106
src/data_plan.h:106: const char* TypeToLibcrosString(Type) const;
On 2010/10/11 18:56:40, Jason Glasgow wrote:
> Should this give a name to the parameter?  (Type type)  It is more consistent
> with other function declarations.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> All parameters should be named, with identical names in the declaration and
> implementation.

Yes, that was an unintended oversight.  Thanks for pointing it out.  Fixed.

http://codereview.chromium.org/3528016/diff/13002/31007
File src/data_plan_provider.cc (right):

http://codereview.chromium.org/3528016/diff/13002/31007#newcode124
src/data_plan_provider.cc:124: Value *parsed_usage_update =
On 2010/10/11 18:56:40, Jason Glasgow wrote:
> We often use scoped_ptr<> to ensure that delete is called.

Done.

http://codereview.chromium.org/3528016/diff/13002/31008
File src/data_plan_provider.h (right):

http://codereview.chromium.org/3528016/diff/13002/31008#newcode77
src/data_plan_provider.h:77: HttpFetcher * const fetcher_;
On 2010/10/11 18:56:40, Jason Glasgow wrote:
> ScopedPtr<> ?  It will simplify the destructor.  But either way is fine.

Done.

http://codereview.chromium.org/3528016/diff/13002/31014
File src/libcurl_http_fetcher.cc (right):

http://codereview.chromium.org/3528016/diff/13002/31014#newcode34
src/libcurl_http_fetcher.cc:34: url_ = url;
On 2010/10/11 18:56:40, Jason Glasgow wrote:
> Shouldn't url_ and url be the same?  If doesn't make sense to resume if they
are
> different.  (Hmm, I just noticed that resume is used from BeginTransfer).

I should start out by saying that I lifted http_fetcher.h,
libcurl_http_fetcher.h, and libcurl_http_fetcher.cc from the update_engine
package, so I'm not the original author of this code.

My reading is that this is a no-op in the resume case, but it's the way that
|url_| is set initially in the begin case.

http://codereview.chromium.org/3528016/diff/13002/31018
File src/policy.h (right):

http://codereview.chromium.org/3528016/diff/13002/31018#newcode26
src/policy.h:26: virtual guint GetUpdateTimerIdleSecs() const = 0;
On 2010/10/11 20:17:27, Jason Glasgow wrote:
> Theoretically, I'm not sure that the idle timer should be constant over the
life
> of the service.  When the user has 5G left in a plan, we might do this
> infrequently, and when the user has 50M left, we might request updates more
> frequently.  But lets go with this design until we have a compelling practical
> reason to change.

I changed the interface to take the current data plans list so that we can do
what you suggest, but the default policy implementation ignores this arg and
returns a constant for the time being.

[jglasgow, 2010-10-11 22:07:23]

LGTM