Consolidate EulaAcceptedNotifier implementations.

Consolidate EulaAcceptedNotifier implementations.

Adds a test and removes the ChromeOS-specific notification
that previously existed only for notifying the
EulaAcceptedNotifierChromeos class.

BUG=245437
TEST=New unit test, no functionality change.

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

[SteveT, 2013-05-31 12:20:08]

Looking good. Nice, clean tests. PTAL.

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier.cc (right):

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.cc:22: #else
Do you think it'd be cleaner if we did

#if (defined(OS_CHROMEOS) && defined(GOOGLE_CHROME_BUILD) || defined(OS_ANDROID)
|| defined(OS_IOS)
  // maybe make the EulaAcceptedNotifier
#else
  return NULL;
#endif

?

The first IF condition is nastier (especially with the extra brackets) and might
wrap, but it means that a reader can easily tell in precisely which situations
the EulaAcceptedNotifier may be created. (I say "may be" here and above because
of your additional FindPreference check below)

Thoughts?

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.cc:26: // tests for
higher-level classes that use the EulaAcceptNotifier.
To be a bit more clear in this comment, is the kEulaAccepted pref not registered
JUST in these tests, or is this is a thing that can happen in a real runtime? A
little bit of clarity on that might be nice.

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier.h (right):

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.h:24: static
EulaAcceptedNotifier* Create();
I don't recall: Do we usually stick these factory methods above the ctor/dtor? I
thought static decls were usually kept at the end of a private/protected/public
section, though I could be wrong.

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier_unittest.cc
(right):

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier_unittest.cc:18: 
Optional nit: Mind adding a comment here?

// testing::Test overrides.

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier_unittest.cc:24: 
Optional nit: Mind adding a comment here?

// EulaAcceptedNotifier::Observer overrides.

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier_unittest.cc:71:
EXPECT_TRUE(notifier()->IsEulaAccepted());
Would it be more consistent to check notifier()->IsEulaAccepted() first, like in
the two tests above?

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier_unittest.cc:73:
EXPECT_TRUE(notifier()->IsEulaAccepted());
Is it worth checking eula_accepted_called() here? Make sure that answer doesn't
change, either?

[Alexei Svitkine (slow), 2013-05-31 15:25:17]

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier.cc (right):

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.cc:22: #else
On 2013/05/31 12:20:08, SteveT wrote:
> Do you think it'd be cleaner if we did
> 
> #if (defined(OS_CHROMEOS) && defined(GOOGLE_CHROME_BUILD) ||
defined(OS_ANDROID)
> || defined(OS_IOS)
>   // maybe make the EulaAcceptedNotifier
> #else
>   return NULL;
> #endif
> 
> ?
> 
> The first IF condition is nastier (especially with the extra brackets) and
might
> wrap, but it means that a reader can easily tell in precisely which situations
> the EulaAcceptedNotifier may be created. (I say "may be" here and above
because
> of your additional FindPreference check below)
> 
> Thoughts?

Done.

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.cc:26: // tests for
higher-level classes that use the EulaAcceptNotifier.
On 2013/05/31 12:20:08, SteveT wrote:
> To be a bit more clear in this comment, is the kEulaAccepted pref not
registered
> JUST in these tests, or is this is a thing that can happen in a real runtime?
A
> little bit of clarity on that might be nice.

Done.

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier.h (right):

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.h:24: static
EulaAcceptedNotifier* Create();
On 2013/05/31 12:20:08, SteveT wrote:
> I don't recall: Do we usually stick these factory methods above the ctor/dtor?
I
> thought static decls were usually kept at the end of a
private/protected/public
> section, though I could be wrong.

Done.

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier_unittest.cc
(right):

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier_unittest.cc:18: 
On 2013/05/31 12:20:08, SteveT wrote:
> Optional nit: Mind adding a comment here?
> 
> // testing::Test overrides.

Done.

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier_unittest.cc:24: 
On 2013/05/31 12:20:08, SteveT wrote:
> Optional nit: Mind adding a comment here?
> 
> // EulaAcceptedNotifier::Observer overrides.

Done.

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier_unittest.cc:71:
EXPECT_TRUE(notifier()->IsEulaAccepted());
On 2013/05/31 12:20:08, SteveT wrote:
> Would it be more consistent to check notifier()->IsEulaAccepted() first, like
in
> the two tests above?

Done.

https://codereview.chromium.org/16129004/diff/11003/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier_unittest.cc:73:
EXPECT_TRUE(notifier()->IsEulaAccepted());
On 2013/05/31 12:20:08, SteveT wrote:
> Is it worth checking eula_accepted_called() here? Make sure that answer
doesn't
> change, either?

Once TRUE, it shouldn't change. There's a check for this in OnEulaAccepted()
above.

[Alexei Svitkine (slow), 2013-06-03 18:53:43]

ping

[SteveT, 2013-06-03 18:58:17]

lgtm - thanks for the refactoring!

https://codereview.chromium.org/16129004/diff/14005/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier.cc (right):

https://codereview.chromium.org/16129004/diff/14005/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.cc:44:
defined(OS_ANDROID) || defined(OS_IOS)
optional nit: I think it would be slightly easier on the eyes if the two
"defined" keyboards were lined up here (one more space indent - I realize you
just used the standard 4).

[Alexei Svitkine (slow), 2013-06-03 20:36:38]

https://codereview.chromium.org/16129004/diff/14005/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier.cc (right):

https://codereview.chromium.org/16129004/diff/14005/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.cc:44:
defined(OS_ANDROID) || defined(OS_IOS)
On 2013/06/03 18:58:18, SteveT wrote:
> optional nit: I think it would be slightly easier on the eyes if the two
> "defined" keyboards were lined up here (one more space indent - I realize you
> just used the standard 4).

That would be misleading, since the defined keyword above is nested differently
(in its own set of parens), while the defined keyword below is outside those.

[commit-bot: I haz the power, 2013-06-03 20:36:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/16129004/14005

[commit-bot: I haz the power, 2013-06-03 20:44:18]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[SteveT, 2013-06-03 20:47:48]

https://codereview.chromium.org/16129004/diff/14005/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier.cc (right):

https://codereview.chromium.org/16129004/diff/14005/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.cc:44:
defined(OS_ANDROID) || defined(OS_IOS)
Ah, you're right. SGTM.

[commit-bot: I haz the power, 2013-06-04 01:18:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/16129004/14005

[Alexei Svitkine (slow), 2013-06-04 01:19:27]

+nkostylev for chrome/browser/chromeos/ OWNERS
+sky for chrome/ OWNERS

[Alexei Svitkine (slow), 2013-06-04 01:19:41]

+nkostylev for chrome/browser/chromeos/ OWNERS
+sky for chrome/ OWNERS

[sky, 2013-06-04 14:50:31]

LGTM

[commit-bot: I haz the power, 2013-06-04 14:54:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/16129004/14005

[Nikita (slow), 2013-06-04 15:03:18]

lgtm

[commit-bot: I haz the power, 2013-06-04 20:37:08]

Change committed as 204031