Deprecate HOSTED_NOT_ALLOWED auth error

Deprecate HOSTED_NOT_ALLOWED auth error

BUG=640958
TEST=Just a code cleanup, shouldn't change any behavior.

Committed: https://crrev.com/23fbe6681a7125701034c6d3fbff6671f6fb8f6a
Cr-Commit-Position: refs/heads/master@{#417847}

[Greg Levin, 2016-09-06 22:20:37]

The CQ bit was checked by glevin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 22:21:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 23:42:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 23:42:49]

Dry run: This issue passed the CQ dry run.

[Greg Levin, 2016-09-07 14:46:04]

Since Hosted accounts no longer exist, I'm cleaning up this error code and a few
related strings.  Please have a look!

alemate@chromium.org
  chrome/app/chromeos_strings.grdp
  chrome/browser/chromeos/login/*
  chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc

rogerta@chromium.org
  chrome/browser/signin/*
  components/signin/core/browser/signin_error_controller_unittest.cc
  google_apis/gaia/*

zea@chromium.org
  chrome/android/java/*
  chrome/browser/sync/sync_ui_util_unittest.cc
  ios/chrome/browser/sync/sync_setup_service.cc

jochen@chromium.org
  tools/metrics/histograms/histograms.xml

[Greg Levin, 2016-09-07 14:48:52]

glevin@chromium.org changed reviewers:
+ alemate@chromium.org, jochen@chromium.org, rogerta@chromium.org,
zea@chromium.org

[Greg Levin, 2016-09-07 14:48:52]

Since Hosted accounts no longer exist, I'm cleaning up this error code and a few
related strings.  Please have a look!

alemate@chromium.org
  chrome/app/chromeos_strings.grdp
  chrome/browser/chromeos/login/*
  chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc

rogerta@chromium.org
  chrome/browser/signin/*
  components/signin/core/browser/signin_error_controller_unittest.cc
  google_apis/gaia/*

zea@chromium.org
  chrome/android/java/*
  chrome/browser/sync/sync_ui_util_unittest.cc
  ios/chrome/browser/sync/sync_setup_service.cc

jochen@chromium.org
  tools/metrics/histograms/histograms.xml

[Alexander Alekseev, 2016-09-07 15:42:39]

lgtm

[Nicolas Zea, 2016-09-07 18:24:34]

sync lgtm

[jochen (gone - plz use gerrit), 2016-09-08 12:47:10]

sorry, I can only approve UseCounter UMA changes.

[Greg Levin, 2016-09-08 13:23:16]

glevin@chromium.org changed reviewers:
+ holte@chromium.org
- jochen@chromium.org

[Greg Levin, 2016-09-08 13:23:16]

holte@ - Could you please have a look at histograms.xml?

[Roger Tawa OOO till Jul 10th, 2016-09-08 13:49:24]

https://codereview.chromium.org/2314913003/diff/1/chrome/browser/signin/signi...
File chrome/browser/signin/signin_error_notifier_ash_unittest.cc (right):

https://codereview.chromium.org/2314913003/diff/1/chrome/browser/signin/signi...
chrome/browser/signin/signin_error_notifier_ash_unittest.cc:231: {
GoogleServiceAuthError::HOSTED_NOT_ALLOWED_DEPRECATED, true },
Should remove this from tests.  Same for other signin unit tests.  I understand
that removing from here means the static assert at line 236 now fails, but I
don't think we should be testing a case that can now never happen in production
code.  (The case was removed from unit tests that don't do such a static assert)

We should either remove the assert at line 236, or maybe assert that the number
is GoogleServiceAuthError::NUM_STATES minus number of deprecated values.

[Greg Levin, 2016-09-08 14:48:52]

The CQ bit was checked by glevin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 14:49:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 15:59:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 15:59:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Greg Levin, 2016-09-08 18:12:32]

The CQ bit was checked by glevin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 18:13:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 19:47:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 19:47:25]

Dry run: This issue passed the CQ dry run.

[Greg Levin, 2016-09-08 19:56:36]

alemate@ - FYI: in your domain, I removed an unused help topic from an enum:
https://codereview.chromium.org/2314913003/diff2/20001:60001/chrome/browser/c...
Sorry, I meant to do this in the initial draft, and just forgot.

rogerta@ - Please have another look!

https://codereview.chromium.org/2314913003/diff/1/chrome/browser/signin/signi...
File chrome/browser/signin/signin_error_notifier_ash_unittest.cc (right):

https://codereview.chromium.org/2314913003/diff/1/chrome/browser/signin/signi...
chrome/browser/signin/signin_error_notifier_ash_unittest.cc:231: {
GoogleServiceAuthError::HOSTED_NOT_ALLOWED_DEPRECATED, true },
On 2016/09/08 13:49:24, Roger Tawa wrote:
> Should remove this from tests.  Same for other signin unit tests.  I
understand
> that removing from here means the static assert at line 236 now fails, but I
> don't think we should be testing a case that can now never happen in
production
> code.  (The case was removed from unit tests that don't do such a static
assert)
> 
> We should either remove the assert at line 236, or maybe assert that the
number
> is GoogleServiceAuthError::NUM_STATES minus number of deprecated values.

Done for two of them, but see note in signin_global_error_unittest.cc.

https://codereview.chromium.org/2314913003/diff/20001/google_apis/gaia/google...
File google_apis/gaia/google_service_auth_error.h (right):

https://codereview.chromium.org/2314913003/diff/20001/google_apis/gaia/google...
google_apis/gaia/google_service_auth_error.h:139: // code in a text message".
Fix typos, replace non-displaying Unicode open/close quotes with ascii quotes.

https://codereview.chromium.org/2314913003/diff/60001/chrome/browser/signin/s...
File chrome/browser/signin/signin_global_error_unittest.cc (right):

https://codereview.chromium.org/2314913003/diff/60001/chrome/browser/signin/s...
chrome/browser/signin/signin_global_error_unittest.cc:156: continue;
I couldn't remove HOSTED_NOT_ALLOWED from this table, because the
histogram_tester requires it to align with the enum in histograms.xml (see test
failures in Patch #2).

This code to skip testing HOSTED_NOT_ALLOWED is crude, but simple seemed okay in
this one-off instance.  I also considered adding a |is_deprecated| member to the
struct, or putting HOSTED_NOT_ALLOWED into a singleton set of entries to be
skipped.  Let me know if one of these, or some other approach, sounds better.

[Alexander Alekseev, 2016-09-08 21:04:38]

lgtm

[Roger Tawa OOO till Jul 10th, 2016-09-08 21:05:58]

lgtm

Thanks for looking into it Greg.  One nit comment below, not sure if it is worth
addressing.

https://codereview.chromium.org/2314913003/diff/60001/chrome/browser/signin/s...
File chrome/browser/signin/signin_global_error_unittest.cc (right):

https://codereview.chromium.org/2314913003/diff/60001/chrome/browser/signin/s...
chrome/browser/signin/signin_global_error_unittest.cc:156: continue;
On 2016/09/08 19:56:35, Greg Levin wrote:
> I couldn't remove HOSTED_NOT_ALLOWED from this table, because the
> histogram_tester requires it to align with the enum in histograms.xml (see
test
> failures in Patch #2).
> 
> This code to skip testing HOSTED_NOT_ALLOWED is crude, but simple seemed okay
in
> this one-off instance.  I also considered adding a |is_deprecated| member to
the
> struct, or putting HOSTED_NOT_ALLOWED into a singleton set of entries to be
> skipped.  Let me know if one of these, or some other approach, sounds better.

Got it, I see what you mean.  I guess the problem is at line 174 below.  I
wonder if there should be a method like:

static bool GoogleServiceAuthError::IsDeprecated(State state);

to use in the if statement here and the other test file?

[Steven Holte, 2016-09-09 00:17:03]

histograms.xml lgtm

[Greg Levin, 2016-09-09 14:07:39]

glevin@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Greg Levin, 2016-09-09 14:07:40]

mariakhomenko@ - Coverage of
chrome/android/java/strings/android_chrome_strings.grd got lost in a reviewer
swap.  Could you please take a quick look at this deletion of a now-unused
string?  Thanks!

rogerta@ - I've put HOSTED_NOT_ALLOWED_DEPRECATED back into the three tables
where array size is being checked.  I got rid of NUM_DEPRECATED_STATES and am
using IsDeprecated() to skip testing HOSTED.  Is this what you had in mind?
(Also picked up a few changes in a merge, sorry.  The three changed unit tests
are
https://codereview.chromium.org/2314913003/diff/80001/chrome/browser/signin/s...
https://codereview.chromium.org/2314913003/diff/80001/chrome/browser/signin/s...
https://codereview.chromium.org/2314913003/diff/80001/components/signin/core/...
)
Let me know if this looks okay.

[Greg Levin, 2016-09-09 14:07:51]

The CQ bit was checked by glevin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 14:08:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roger Tawa OOO till Jul 10th, 2016-09-09 14:18:18]

lgtm

[commit-bot: I haz the power, 2016-09-09 15:18:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 15:18:39]

Dry run: This issue passed the CQ dry run.

[Maria, 2016-09-09 16:24:50]

lgtm

[Greg Levin, 2016-09-10 21:00:38]

The CQ bit was checked by glevin@chromium.org

[Greg Levin, 2016-09-10 21:00:39]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
alemate@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2314913003/#ps80001
(title: "Replace NUM_DEPRECATED_STATES with IsDeprecated()")

[commit-bot: I haz the power, 2016-09-10 21:00:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 22:37:31]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-10 22:39:11]

Description was changed from

==========
Deprecate HOSTED_NOT_ALLOWED auth error

BUG=640958
TEST=Just a code cleanup, shouldn't change any behavior.
==========

to

==========
Deprecate HOSTED_NOT_ALLOWED auth error

BUG=640958
TEST=Just a code cleanup, shouldn't change any behavior.

Committed: https://crrev.com/23fbe6681a7125701034c6d3fbff6671f6fb8f6a
Cr-Commit-Position: refs/heads/master@{#417847}
==========

[commit-bot: I haz the power, 2016-09-10 22:39:14]

Patchset 5 (id:??) landed as
https://crrev.com/23fbe6681a7125701034c6d3fbff6671f6fb8f6a
Cr-Commit-Position: refs/heads/master@{#417847}