Delete lots of sync ServerConnectionManager code

Delete lots of sync ServerConnectionManager code

This removes several pieces of internal state and functions from the
ServerConnectionManager and related classes, including:

* SyncManager::Summary.authenticated: This variable name was very misleading.
AllStatus managed it by setting it to true iff the last attempt to hit the
sync server resulting in a 200 OK response.  This is especially misleading
since CheckServerReachable()/CheckTime() do not validate the server's
credentials in any way.

* AllStatus::HandleAuthWatcherEvent(): This function was not defined.  It
should have been removed from the header file ages ago.

* AllStatus::CalcStatusChanges()'s 'online' flag: This was based on unreliable
flags such as server_reachable, server_up and server_broken.  It is now
ard-coded to 'true'.  This makes the "offline" states (OFFLINE,
OFFLINE_UNUSABLE, etc.) unreachable.

* ServerConnectionManager's server_up and server_reachable: No important
decisions were made based on the status of these variables.  The way in which
they were maintained was questionable as well.  The server_reachable flag, in
particular, had an effect on our ability to retry on connection failures that
seems almost arbitrary.  We would retry to connect *once* after an IP address
change, then never again until the IP adddress change again.  However, if that
first ping did succeed, server_reachable would remain set (and we would
continue retrying to contact the server with exponential backoff) indefinitely.

Also removed in this chage are SyncManager::Summary's server_up and
server_reachable, as well as most of the code that depends on them.

* ServerConnectionManager::CheckServerReachable(): The only effect these
functions could have would be to alter the value of server_reachable and the
server_status.  Neither of these variables have been put to good use, so we
should be able to remove this code without ill effect.

* ServerConnectionManager::CheckTime(): There are some comments in this
function claiming it protects against WiFi redirects.  This may have been
true at some point, but these days we use HTTPS.  If a router can mangle
server responses, we have bigger problems than sync not working.

* ServerConnectionManager::IsUserAuthenticated(): I believe this is not
used anywhere.

* ServerConnectoinManager::SetServerParameters(): Also not used.

* SyncScheduler's ServerConnectionManager observing functionality has been
removed.  All 'pings' that arrived through this function were either a direct
result of SyncScheduler activity (ie. the scheduler tried to run a sync
cycle and it failed or succeeded unexpectedly), a change in user credentials,
during startup, or during an IP address change.  It is much simpler to
have the events that should cause the SyncScheduler run a canary job
notify the scheduler directly, rather than threading it (incorrectly) through
the ServerConnectionManager.  Doing it this way also removes the need for
pinging the server to check the time, which isn't really necessary anway.

The one big behavioural change in this patch is that we *never* give up in
trying to contact the server.  There is no longer a server_reachable variable
and we act as tough it were always true.  This isn't too much different from
the old behaviour, where server_reachable would get set once and almost never
get unset.  It does mean that we won't give up so easily in contacting the
server when we start offline, though.

BUG=98346
TEST=Tests involving enabling and disabling network access would be especially valuable here.

[rlarocque, 2012-01-19 02:17:28]

This post is FYI only; I'm not looking to commit it yet.

This is in response to Lingesh's patches to ServerConnectionManager.  I don't
see anything wrong with what he's proposing, but I did want to publish some of
the more drastic changes that I think should be made.  I don't mind if we
duplicate some of this work in order to get it into M18 (since I doubt this
change will be ready in time).  

This patch is still in rough shape.  I recently rebased it against trunk to make
posting this code review possible.  I bypassed a lot of intermediate patches in
the process, so it wouldn't surprise me if this patch was slightly broken.  

It will need a bit more polish and a proper crbug before I commit this.  I think
that it's polished enough that it makes sense to skim over it now, though.

[rlarocque, 2012-01-19 02:23:05]

http://codereview.chromium.org/9251035/diff/1/chrome/browser/sync/sync_ui_uti...
File chrome/browser/sync/sync_ui_util.cc (left):

http://codereview.chromium.org/9251035/diff/1/chrome/browser/sync/sync_ui_uti...
chrome/browser/sync/sync_ui_util.cc:294: } else if (!status.authenticated) {
Since status.authenticated correlates mostly with "did the last connection
attempt return 200?", this branch doesn't make a lot of sense to me.  Especially
since this is in the startup case, where a CheckServerReachable() could be
affecting the SCM's state. 

Fred suggested that this might be dead code.  I'm not convinced that it is, but
I'm not sure I care.  I'd rather remove it.

TODO: Figure out if IDS_SYNC_ACCOUNT_DETAILS_NOT_ENTERED is used anywhere else
in order to determine if it should be removed, too.

[tim (not reviewing), 2012-01-19 23:13:52]

RE: * SyncManager::Summary.authenticated: This variable name was very
misleading.
AllStatus managed it by setting it to true iff the last attempt to hit the
sync server resulting in a 200 OK response. This is especially misleading
since CheckServerReachable()/CheckTime() do not validate the server's
credentials in any way.


Now that we have the last response code for individual request types, can't we
do this properly?