AU: Implement getting of tracks through GetTrack.

AU: Implement getting of tracks through GetTrack.

Add SetTrack and GetTrack to UpdateEngine.xml as well.

BUG=chromium-os:8104
TEST=tested on device with update_engine_client

Change-Id: I10ef2552e9280524b3a8d5f232d104a81a114e06

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

[petkov, 2010-10-26 17:02:21]

Thanks for implementing this and for updating the .conf file -- I should have
tested as chronos rather than root. A few comments below, mostly style nits.

http://codereview.chromium.org/4181001/diff/1/2
File UpdateEngine.conf (right):

http://codereview.chromium.org/4181001/diff/1/2#newcode28
UpdateEngine.conf:28: <allow send_destination="org.chromium.UpdateEngine"
Could you please keep these sorted alphabetically by member name? I.e., move
GetTrack to after GetStatus.

http://codereview.chromium.org/4181001/diff/1/3
File dbus_service.cc (right):

http://codereview.chromium.org/4181001/diff/1/3#newcode131
dbus_service.cc:131: return track_str.empty() ? FALSE : TRUE;
It seems that if you return FALSE, you should set *error=NULL and maybe *track
to NULL.

I'd change the API to always return TRUE, and set *track to an empty string if
necessary. Then let the caller deal with the empty string. This is fair because
update_engine will actually use an empty string as the track it sends to Omaha.

http://codereview.chromium.org/4181001/diff/1/4
File dbus_service.h (right):

http://codereview.chromium.org/4181001/diff/1/4#newcode69
dbus_service.h:69: gboolean update_engine_service_set_track(UpdateEngineService*
self,
Is this a duplicate declaration? See declaration above.

http://codereview.chromium.org/4181001/diff/1/4#newcode73
dbus_service.h:73: gboolean update_engine_service_get_track(UpdateEngineService*
self,
Let's keep these sorted -- move to after get_status.

http://codereview.chromium.org/4181001/diff/1/5
File omaha_request_params.cc (right):

http://codereview.chromium.org/4181001/diff/1/5#newcode95
omaha_request_params.cc:95: std::string
OmahaRequestDeviceParams::GetDeviceTrack() {
s/std::string/string/

http://codereview.chromium.org/4181001/diff/1/5#newcode97
omaha_request_params.cc:97: if (!params.Init("", "")) {
This seems like a one liner?

return params.Init("", "") ? params.app_track : "";

http://codereview.chromium.org/4181001/diff/1/7
File update_engine.xml (right):

http://codereview.chromium.org/4181001/diff/1/7#newcode26
update_engine.xml:26: <method name="GetTrack">
Keep these sorted -- move to after GetStatus.

http://codereview.chromium.org/4181001/diff/1/8
File update_engine_client.cc (right):

http://codereview.chromium.org/4181001/diff/1/8#newcode31
update_engine_client.cc:31: DEFINE_bool(show_track, false, "Show the update
track.");
Keep flag declaration sorted -- move to after reboot.

http://codereview.chromium.org/4181001/diff/1/8#newcode202
update_engine_client.cc:202: char *track = NULL;
star to the left for consistency.

[satorux1, 2010-10-26 22:08:24]

Darin,

Thank you for the valuable feedback. I think Ive address all of your comments.
Please take another look.

http://codereview.chromium.org/4181001/diff/1/2
File UpdateEngine.conf (right):

http://codereview.chromium.org/4181001/diff/1/2#newcode28
UpdateEngine.conf:28: <allow send_destination="org.chromium.UpdateEngine"
On 2010/10/26 17:02:21, petkov wrote:
> Could you please keep these sorted alphabetically by member name? I.e., move
> GetTrack to after GetStatus.
> 
> 

Done.

http://codereview.chromium.org/4181001/diff/1/3
File dbus_service.cc (right):

http://codereview.chromium.org/4181001/diff/1/3#newcode131
dbus_service.cc:131: return track_str.empty() ? FALSE : TRUE;
On 2010/10/26 17:02:21, petkov wrote:
> It seems that if you return FALSE, you should set *error=NULL and maybe *track
> to NULL.
> 
> I'd change the API to always return TRUE, and set *track to an empty string if
> necessary. Then let the caller deal with the empty string. This is fair
because
> update_engine will actually use an empty string as the track it sends to
Omaha.
> 

Good idea. Done.

http://codereview.chromium.org/4181001/diff/1/4
File dbus_service.h (right):

http://codereview.chromium.org/4181001/diff/1/4#newcode69
dbus_service.h:69: gboolean update_engine_service_set_track(UpdateEngineService*
self,
On 2010/10/26 17:02:21, petkov wrote:
> Is this a duplicate declaration? See declaration above.
> 

Good catch. Removed

http://codereview.chromium.org/4181001/diff/1/4#newcode73
dbus_service.h:73: gboolean update_engine_service_get_track(UpdateEngineService*
self,
On 2010/10/26 17:02:21, petkov wrote:
> Let's keep these sorted -- move to after get_status.
> 

Done.

http://codereview.chromium.org/4181001/diff/1/5
File omaha_request_params.cc (right):

http://codereview.chromium.org/4181001/diff/1/5#newcode95
omaha_request_params.cc:95: std::string
OmahaRequestDeviceParams::GetDeviceTrack() {
On 2010/10/26 17:02:21, petkov wrote:
> s/std::string/string/
> 

Done.

http://codereview.chromium.org/4181001/diff/1/5#newcode97
omaha_request_params.cc:97: if (!params.Init("", "")) {
On 2010/10/26 17:02:21, petkov wrote:
> This seems like a one liner?
> 
> return params.Init("", "") ? params.app_track : "";
> 

Done.

http://codereview.chromium.org/4181001/diff/1/7
File update_engine.xml (right):

http://codereview.chromium.org/4181001/diff/1/7#newcode26
update_engine.xml:26: <method name="GetTrack">
On 2010/10/26 17:02:21, petkov wrote:
> Keep these sorted -- move to after GetStatus.
> 

Done.

http://codereview.chromium.org/4181001/diff/1/8
File update_engine_client.cc (right):

http://codereview.chromium.org/4181001/diff/1/8#newcode31
update_engine_client.cc:31: DEFINE_bool(show_track, false, "Show the update
track.");
On 2010/10/26 17:02:21, petkov wrote:
> Keep flag declaration sorted -- move to after reboot.
> 

Done.

http://codereview.chromium.org/4181001/diff/1/8#newcode202
update_engine_client.cc:202: char *track = NULL;
On 2010/10/26 17:02:21, petkov wrote:
> star to the left for consistency.
> 

Done.

[petkov, 2010-10-26 22:13:33]

LGTM -- thanks for working on this.