Make the "Tabs" item on about:sync non-yellow

Make the "Tabs" item on about:sync non-yellow

This was achieved by changing ProfileSyncService::GetTypeStatusMap() to
have a special case for proxy types. A passive proxy type gets assigned
status "ok" instead of "warning" which makes it appear green on the
about:sync page.

BUG=380442
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275561

[stanisc, 2014-06-05 20:27:56]

This is a trivial fix for issue 380442 to help me get familiar with the
end-to-end bug fix workflow.

[rlarocque, 2014-06-05 21:21:11]

I've started some trybots on your behalf, since your 'git cl try' seems to have
failed because of permissions issues.

https://codereview.chromium.org/312333005/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/312333005/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1986: // Set status to ok for a
passive proxy type (which is always passive);
nit: I would rephrase this so that the check for proxy types is the in the outer
if statement, rather than making this a special case of the passive state.

I believe proxy types exist in only one of two states: enabled (== passive) or
disabled.  There's no point in letting them traverse through all these other if
branches.  Better to filter them out earlier.

https://codereview.chromium.org/312333005/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1988: if (type >=
syncer::FIRST_PROXY_TYPE && type <= syncer::LAST_PROXY_TYPE) {
Is there no IsProxyType() helper function in model_type.cc?  If not, there
probably should be.

[stanisc, 2014-06-06 01:15:44]

Second iteration: Added IsProxyType function (and corresponding unit test) and
restructured ProfileSyncService::GetTypeStatusMap as suggested.

https://codereview.chromium.org/312333005/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/312333005/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1986: // Set status to ok for a
passive proxy type (which is always passive);
On 2014/06/05 21:21:11, rlarocque wrote:
> nit: I would rephrase this so that the check for proxy types is the in the
outer
> if statement, rather than making this a special case of the passive state.
> 
> I believe proxy types exist in only one of two states: enabled (== passive) or
> disabled.  There's no point in letting them traverse through all these other
if
> branches.  Better to filter them out earlier.

Done.

https://codereview.chromium.org/312333005/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1988: if (type >=
syncer::FIRST_PROXY_TYPE && type <= syncer::LAST_PROXY_TYPE) {
On 2014/06/05 21:21:11, rlarocque wrote:
> Is there no IsProxyType() helper function in model_type.cc?  If not, there
> probably should be.

I searched for FIRST_PROXY_TYPE and LAST_PROXY_TYPE and didn't get any hits
except in one unit test. I'll add one.

https://codereview.chromium.org/312333005/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1988: if (type >=
syncer::FIRST_PROXY_TYPE && type <= syncer::LAST_PROXY_TYPE) {
On 2014/06/05 21:21:11, rlarocque wrote:
> Is there no IsProxyType() helper function in model_type.cc?  If not, there
> probably should be.

Done.

[rlarocque, 2014-06-06 01:24:36]

LGTM.

I've started a bunch more trybots for you latest patch.  Since this is such a
safe patch, it's very likely that any failures you see will be not your fault
(ie. flakes).  This happens fairly often.  It's good practice to investigate
flakes and make sure they're unrelated to your changes, but you should also keep
in mind that not all trybot failures represent a problem in your patch.

With my LGTM, you should now be able to check the "commit" check box.  This will
enqueue the CL for commit.  You can do that today or wait until tomorrow when
there's more support around to help if something goes wrong.

[stanisc, 2014-06-06 19:01:15]

The CQ bit was checked by stanisc@chromium.org

[stanisc, 2014-06-06 19:01:33]

The CQ bit was unchecked by stanisc@chromium.org

[stanisc, 2014-06-06 19:02:21]

The CQ bit was checked by stanisc@chromium.org

[commit-bot: I haz the power, 2014-06-06 19:03:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stanisc@chromium.org/312333005/20001

[commit-bot: I haz the power, 2014-06-07 00:01:52]

Change committed as 275561