Refactor ResourceRequestAllowedNotifier EULA checking into a separate class.

Refactor ResourceRequestAllowedNotifier EULA checking into a separate class.

This paves the way for making a EulaAcceptedNotifierAndroid class.

Also enables EULA logic tests on all platforms, not just ChromeOS
(since the EULA criteria are being mocked already).

No functionality changes.

BUG=226757
TEST=Existing unit tests.

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

[Alexei Svitkine (slow), 2013-04-05 15:57:36]

PTAL. I still need to do a bit of testing on a ChromeOS build.

[Alexei Svitkine (slow), 2013-04-05 19:13:05]

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier_chromeos.cc
(right):

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier_chromeos.cc:48: #if
defined(GOOGLE_CHROME_BUILD)
I forgot to check GOOGLE_CHROME_BUILD here earlier.

[SteveT, 2013-04-05 22:16:41]

LGTM. Thanks for doing this refactoring. It's quite nice being able to get rid
of the #ifdef CROS stuff.

A couple questions about next steps / followup:
 * We're going to move RRAN (and also the EulaAcceptedNotifier) into a different
directory in future refactoring, correct? Since it's shared?
 * We're waiting on the change on the Chrome for Android side to expose the pref
we need before we build the EulaAcceptedNotifierMobile? Is there anything else
blocking this work?

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

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.cc:30: 
nit: Could you check to see if there is an extra line here?

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

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.h:11: // when the EULA
is accepted. This abstract away the platform-specific logic
abstract -> abstracts

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier_chromeos.cc
(right):

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier_chromeos.cc:28:
content::NotificationService::AllSources());
nit: align content:: with "this" in the line above.

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
File
chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc
(right):

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:44:
// EulaAcceptedNotifier test class that allows mocking the eula accepted state
eula -> EULA

[Alexei Svitkine (slow), 2013-04-08 14:48:41]

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

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.cc:30: 
On 2013/04/05 22:16:41, SteveT wrote:
> nit: Could you check to see if there is an extra line here?

Done.

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

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier.h:11: // when the EULA
is accepted. This abstract away the platform-specific logic
On 2013/04/05 22:16:41, SteveT wrote:
> abstract -> abstracts

Done.

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/eula_accepted_notifier_chromeos.cc
(right):

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
chrome/browser/metrics/variations/eula_accepted_notifier_chromeos.cc:28:
content::NotificationService::AllSources());
On 2013/04/05 22:16:41, SteveT wrote:
> nit: align content:: with "this" in the line above.

Done.

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
File
chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc
(right):

https://codereview.chromium.org/13620010/diff/50008/chrome/browser/metrics/va...
chrome/browser/metrics/variations/resource_request_allowed_notifier_unittest.cc:44:
// EulaAcceptedNotifier test class that allows mocking the eula accepted state
On 2013/04/05 22:16:41, SteveT wrote:
> eula -> EULA

Done.

[Alexei Svitkine (slow), 2013-04-08 14:50:51]

On 2013/04/05 22:16:41, SteveT wrote:
> LGTM. Thanks for doing this refactoring. It's quite nice being able to get rid
> of the #ifdef CROS stuff.
> 
> A couple questions about next steps / followup:
>  * We're going to move RRAN (and also the EulaAcceptedNotifier) into a
different
> directory in future refactoring, correct? Since it's shared?
>  * We're waiting on the change on the Chrome for Android side to expose the
pref
> we need before we build the EulaAcceptedNotifierMobile? Is there anything else
> blocking this work?

Yep, those are the two next steps. We'll probably have separate
EulaAcceptedNotifierAndroid/IOS classes.

Also, moving this stuff to a different location is lower-priority, so I plan to
do that only after making the android subclass.

[commit-bot: I haz the power, 2013-04-08 14:51:08]

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

[commit-bot: I haz the power, 2013-04-08 14:51:12]

Presubmit check for 13620010-61001 failed and returned exit status 1.

INFO:root:Found 10 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/chrome_browser.gypi

[Alexei Svitkine (slow), 2013-04-08 14:54:34]

tbr sky for the gypi change

[commit-bot: I haz the power, 2013-04-08 14:54:42]

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

[sky, 2013-04-08 14:55:29]

LGTM

[commit-bot: I haz the power, 2013-04-08 14:59:07]

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

[commit-bot: I haz the power, 2013-04-08 15:02:44]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 15:58:19]

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

[commit-bot: I haz the power, 2013-04-08 16:01:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 18:47:55]

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

[commit-bot: I haz the power, 2013-04-08 21:49:51]

Change committed as 192915