sync: Disable network change notifications for unauthenticated users.

sync: Disable network change notifications for unauthenticated users.

BUG=90944
TEST=none

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

[tim (not reviewing), 2011-07-29 02:24:36]

bug# coming

[lipalani1, 2011-07-29 02:30:38]

lgtm.
2 comments.

http://codereview.chromium.org/7530023/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7530023/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:1631: // TODO(tim): Trying to debug
401-request-loop, temporarily adding this
Not sure if this stmt is needed. This looks like a permanent fix even if the bug
is not this...

http://codereview.chromium.org/7530023/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:2272: if (!observing_ip_address_changes_)
Can you add a log stmt here to say that it was dropped.

[tim (not reviewing), 2011-07-29 02:41:35]

done

[akalin, 2011-07-29 03:08:57]

Drive-by.

http://codereview.chromium.org/7530023/diff/2002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7530023/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1632: bool observing_ip_address_changes_;
I think having 'bool has_valid_credentials_;' is clearer, since that's what
actually matters.

http://codereview.chromium.org/7530023/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:1777: observing_ip_address_changes_ =
true;
shouldn't this remain false?  We're initing, but we don't yet have valid creds,
do we?

http://codereview.chromium.org/7530023/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:2272: VLOG(1) << "IP address change
dropped.";
VLOG can also say something like "IP address changed but no valid credentials;
ignoring"

[lipalani, 2011-07-29 03:13:41]

Just addressing your second comment. I thought in similar lines but did not
think it was worth changing because having it true would mimic the current
behavior closely. If we are changing that we have to consider sync
scheduler's server connection OK variable initialization closely.
On Jul 28, 2011 8:08 PM, <akalin@chromium.org> wrote:
> Drive-by.
>
>
>
http://codereview.chromium.org/7530023/diff/2002/chrome/browser/sync/engine/s...
> File chrome/browser/sync/engine/syncapi.cc (right):
>
>
http://codereview.chromium.org/7530023/diff/2002/chrome/browser/sync/engine/s...
> chrome/browser/sync/engine/syncapi.cc:1632: bool
> observing_ip_address_changes_;
> I think having 'bool has_valid_credentials_;' is clearer, since that's
> what actually matters.
>
>
http://codereview.chromium.org/7530023/diff/2002/chrome/browser/sync/engine/s...
> chrome/browser/sync/engine/syncapi.cc:1777:
> observing_ip_address_changes_ = true;
> shouldn't this remain false? We're initing, but we don't yet have valid
> creds, do we?
>
>
http://codereview.chromium.org/7530023/diff/2002/chrome/browser/sync/engine/s...
> chrome/browser/sync/engine/syncapi.cc:2272: VLOG(1) << "IP address
> change dropped.";
> VLOG can also say something like "IP address changed but no valid
> credentials; ignoring"
>
> http://codereview.chromium.org/7530023/