Fix initial offline state response handling

Fix initial offline state response handling
to read a Variant of uint32 instead of a uint32,
as specified by the Properties.Get interface.

R=satorux@chromium.org,willchan@chromium.org
BUG=104637
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110569

[satorux1, 2011-11-17 17:40:15]

http://codereview.chromium.org/8591021/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/8591021/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_linux.cc:166: if (!reader.HasMoreData() ||
!reader.PopVariantOfUint32(&state)) {
You don't need !reader.HasMoreData(). PopVariantOfUint32() will return false, if
the content is empty.

http://codereview.chromium.org/8591021/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_linux.cc:176:
offline_state_initialized_.Signal();
Calling offline_state_initialized_.Signal() from three places looks a bit
error-prone. You might want to introduce OnResponseInternal() to do most of the
stuff, and then change the function to be

  OnResponseInternal(response);
  offline_state_initialized_.Signal();

http://codereview.chromium.org/8591021/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_linux.cc:183: if (!reader.HasMoreData() ||
!reader.PopUint32(&state)) {
You can remove !reader.HasMoreData() from here too.

[adamk, 2011-11-17 18:58:23]

Thanks for the quick review!

http://codereview.chromium.org/8591021/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/8591021/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_linux.cc:166: if (!reader.HasMoreData() ||
!reader.PopVariantOfUint32(&state)) {
On 2011/11/17 17:40:15, satorux1 wrote:
> You don't need !reader.HasMoreData(). PopVariantOfUint32() will return false,
if
> the content is empty.

Good to know, done.

http://codereview.chromium.org/8591021/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_linux.cc:176:
offline_state_initialized_.Signal();
On 2011/11/17 17:40:15, satorux1 wrote:
> Calling offline_state_initialized_.Signal() from three places looks a bit
> error-prone. You might want to introduce OnResponseInternal() to do most of
the
> stuff, and then change the function to be
> 
>   OnResponseInternal(response);
>   offline_state_initialized_.Signal();

Yeah, I agree; this is covered by the unittests, but if someone were to add
another exit point it'd be bad.  I've renamed the callback to OnInitialResponse
and the parser to HandleResponse.

http://codereview.chromium.org/8591021/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_linux.cc:183: if (!reader.HasMoreData() ||
!reader.PopUint32(&state)) {
On 2011/11/17 17:40:15, satorux1 wrote:
> You can remove !reader.HasMoreData() from here too.

Done.

[satorux1, 2011-11-17 19:02:18]

LGTM

[willchan no longer on Chromium, 2011-11-17 19:05:08]

rubberstamp lgtm

On Thu, Nov 17, 2011 at 2:02 PM, <satorux@chromium.org> wrote:

> LGTM
>
>
http://codereview.chromium.**org/8591021/<http://codereview.chromium.org/8591...
>