Suppress bubble view for CONNECTION_FAILED error

Suppress bubble view for CONNECTION_FAILED error

If there was a connection failed error then we woud display an error bubble view and wrench menu item.

We're not supposed to display error messages for CONNECTION_FAILED errors so all the labels in the bubble view and wrench menu error item were blank.

The problem was that we would decide when to show the error UI based on the auth error state.

To fix this I simplified the code so that we show the error UI based directly on the presence of the error labels. This makes the code significantly simpler.

I also added unit tests to make sure that we're returning labels when we should not not returning them when we shouldn't.

BUG=100451
TEST=


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

[Andrew T Wilson (Slow), 2011-10-17 04:08:54]

http://codereview.chromium.org/8308005/diff/1/chrome/browser/sync/sync_global...
File chrome/browser/sync/sync_global_error.cc (right):

http://codereview.chromium.org/8308005/diff/1/chrome/browser/sync/sync_global...
chrome/browser/sync/sync_global_error.cc:97: bubble_accept_label.empty()) ||
I think the logic in the DCHECK is off? Did you test this with an error label?
It seems like that should fail the dcheck.

Should we add a test for this?

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

http://codereview.chromium.org/8308005/diff/1/chrome/browser/sync/sync_ui_uti...
chrome/browser/sync/sync_ui_util.cc:350: 
Since we aren't returning MessageType any more, I think you need to clear out
menu_label, etc here.

[sail, 2011-10-17 15:21:22]

http://codereview.chromium.org/8308005/diff/1/chrome/browser/sync/sync_global...
File chrome/browser/sync/sync_global_error.cc (right):

http://codereview.chromium.org/8308005/diff/1/chrome/browser/sync/sync_global...
chrome/browser/sync/sync_global_error.cc:97: bubble_accept_label.empty()) ||
On 2011/10/17 04:08:55, Andrew T Wilson wrote:
> I think the logic in the DCHECK is off? Did you test this with an error label?
> It seems like that should fail the dcheck.
> 
> Should we add a test for this?

Oops. Fixed.
I'm testing in Release mode so maybe that's why I didn't see this.
I don't think it's worth adding a new test for this since
VerifySyncGlobalErrorResult() already tests the exact same thing.

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

http://codereview.chromium.org/8308005/diff/1/chrome/browser/sync/sync_ui_uti...
chrome/browser/sync/sync_ui_util.cc:350: 
On 2011/10/17 04:08:55, Andrew T Wilson wrote:
> Since we aren't returning MessageType any more, I think you need to clear out
> menu_label, etc here.

Done.

[Andrew T Wilson (Slow), 2011-10-17 15:54:44]

Tim, are you OK with no tests for sync_global_error? If so, LGTM.

[tim (not reviewing), 2011-10-17 16:12:27]

@sail - I may be missing something but I don't see how
VerifySyncGlobalErrorResult tests the OnStateChanged path?

http://codereview.chromium.org/8308005/diff/4001/chrome/browser/sync/sync_glo...
File chrome/browser/sync/sync_global_error.cc (right):

http://codereview.chromium.org/8308005/diff/4001/chrome/browser/sync/sync_glo...
chrome/browser/sync/sync_global_error.cc:20: using namespace sync_ui_util;
a using-directive isn't allowed by the style guide, can you inline sync_ui_util?
(since I think it's only needed for the code you're already changing below)

[sail, 2011-10-17 16:27:52]

http://codereview.chromium.org/8308005/diff/4001/chrome/browser/sync/sync_glo...
File chrome/browser/sync/sync_global_error.cc (right):

http://codereview.chromium.org/8308005/diff/4001/chrome/browser/sync/sync_glo...
chrome/browser/sync/sync_global_error.cc:20: using namespace sync_ui_util;
On 2011/10/17 16:12:27, timsteele wrote:
> a using-directive isn't allowed by the style guide, can you inline
sync_ui_util?
> (since I think it's only needed for the code you're already changing below)

Done.

[sail, 2011-10-17 16:28:47]

On 2011/10/17 16:12:27, timsteele wrote:
> @sail - I may be missing something but I don't see how
> VerifySyncGlobalErrorResult tests the OnStateChanged path?

It doesn't. I meant that VerifySyncGlobalErrorResult() tests the same thing as
the faulty DCHECK that atwilson pointed out. I'll add a unit_test for
SyncGlobalError explicitly if you want.

[tim (not reviewing), 2011-10-17 16:33:44]

On 2011/10/17 16:28:47, sail wrote:
> On 2011/10/17 16:12:27, timsteele wrote:
> > @sail - I may be missing something but I don't see how
> > VerifySyncGlobalErrorResult tests the OnStateChanged path?
> 
> It doesn't. I meant that VerifySyncGlobalErrorResult() tests the same thing as
> the faulty DCHECK that atwilson pointed out. I'll add a unit_test for
> SyncGlobalError explicitly if you want.

Sorry to be a stickler :/  But I think it's a good idea as it can come in handy
in the future, particularly if ui_util changes / goes away or we start using SGE
in more places.

[sail, 2011-10-17 17:53:45]

Added test for SyncGlobalError. Please take another look.

Thanks

[tim (not reviewing), 2011-10-17 20:57:06]

Thanks!
LGTM

[commit-bot: I haz the power, 2011-10-17 20:58:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8308005/10002

[commit-bot: I haz the power, 2011-10-17 22:38:56]

Change committed as 105944