Cache the Windows Parental Controls Platform Answer

Cache the Windows Parental Controls Platform Answer

Every time we assess if incognito is available on the system on Windows, we hit
COM which could potentially do some hidden I/O on the UI thread. We already
cache the answer for the I/O thread use.

This delta brings both codepaths together so that the UI thread always uses
the cached answer.

BUG=458388

[robliao, 2015-02-23 19:34:28]

robliao@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2015-02-24 20:39:48]

lgtm w/ enum

+ a few considerations being calling issue 458388 fixed.

Thanks!
Gab

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:27: bool g_parental_control_on =
false;
How about an
enum class ParentalControlState {
  UNKNOWN,
  ENABLED,
  DISABLED,
};
rather than two variables?

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:106: void
IncognitoModePrefs::InitializePlatformParentalControls() {
Also, should we not instead lazily initialize this (or perhaps a mix of
lazy/delay initialization -- i.e. either initialize it asynchronously
post-startup with something like PostTaskAndReplyWithResult() to the
BlockingPool() or lazily when it's requested -- whichever kicks in first).

Doesn't make sense to me to call this unconditionally from
ChromeBrowserMainPartsWin::PreMainMessageLoopStart().

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:107: g_parental_control_on =
base::win::IsParentalControlActivityLoggingOn();
Any idea why this call is even in base::win:: (and specifically in
base/win/metro.cc)? Seems to only be called from here.

[gab, 2015-02-24 20:40:03]

On 2015/02/24 20:39:48, gab wrote:
> lgtm w/ enum
> 
> + a few considerations being calling issue 458388 fixed.

s/being/before ...

> 
> Thanks!
> Gab
> 
>
https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
> File chrome/browser/prefs/incognito_mode_prefs.cc (right):
> 
>
https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
> chrome/browser/prefs/incognito_mode_prefs.cc:27: bool g_parental_control_on =
> false;
> How about an
> enum class ParentalControlState {
>   UNKNOWN,
>   ENABLED,
>   DISABLED,
> };
> rather than two variables?
> 
>
https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
> chrome/browser/prefs/incognito_mode_prefs.cc:106: void
> IncognitoModePrefs::InitializePlatformParentalControls() {
> Also, should we not instead lazily initialize this (or perhaps a mix of
> lazy/delay initialization -- i.e. either initialize it asynchronously
> post-startup with something like PostTaskAndReplyWithResult() to the
> BlockingPool() or lazily when it's requested -- whichever kicks in first).
> 
> Doesn't make sense to me to call this unconditionally from
> ChromeBrowserMainPartsWin::PreMainMessageLoopStart().
> 
>
https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
> chrome/browser/prefs/incognito_mode_prefs.cc:107: g_parental_control_on =
> base::win::IsParentalControlActivityLoggingOn();
> Any idea why this call is even in base::win:: (and specifically in
> base/win/metro.cc)? Seems to only be called from here.

[robliao, 2015-02-24 23:39:42]

Patchset #2 (id:20001) has been deleted

[robliao, 2015-02-24 23:43:01]

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:27: bool g_parental_control_on =
false;
On 2015/02/24 20:39:47, gab wrote:
> How about an
> enum class ParentalControlState {
>   UNKNOWN,
>   ENABLED,
>   DISABLED,
> };
> rather than two variables?

sgtm. Done

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:106: void
IncognitoModePrefs::InitializePlatformParentalControls() {
On 2015/02/24 20:39:47, gab wrote:
> Also, should we not instead lazily initialize this (or perhaps a mix of
> lazy/delay initialization -- i.e. either initialize it asynchronously
> post-startup with something like PostTaskAndReplyWithResult() to the
> BlockingPool() or lazily when it's requested -- whichever kicks in first).
> 
> Doesn't make sense to me to call this unconditionally from
> ChromeBrowserMainPartsWin::PreMainMessageLoopStart().

It's oddly enough the best place for it since we need the value right when we
fire off the renderers during startup. Separating I/O from the main thread at
startup will help as too many places check to see if incognito is available on
the UI thread.

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:107: g_parental_control_on =
base::win::IsParentalControlActivityLoggingOn();
On 2015/02/24 20:39:47, gab wrote:
> Any idea why this call is even in base::win:: (and specifically in
> base/win/metro.cc)? Seems to only be called from here.

In a change a long, long, time ago, this was going to be enabled for Win8 only,
hence metro.cc. Later on, Chrome decided to allow this for Vista and Win7.

[robliao, 2015-02-25 18:41:12]

Patchset #3 (id:60001) has been deleted

[gab, 2015-02-25 20:23:52]

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:106: void
IncognitoModePrefs::InitializePlatformParentalControls() {
On 2015/02/24 23:43:01, robliao wrote:
> On 2015/02/24 20:39:47, gab wrote:
> > Also, should we not instead lazily initialize this (or perhaps a mix of
> > lazy/delay initialization -- i.e. either initialize it asynchronously
> > post-startup with something like PostTaskAndReplyWithResult() to the
> > BlockingPool() or lazily when it's requested -- whichever kicks in first).
> > 
> > Doesn't make sense to me to call this unconditionally from
> > ChromeBrowserMainPartsWin::PreMainMessageLoopStart().
> 
> It's oddly enough the best place for it since we need the value right when we
> fire off the renderers during startup. Separating I/O from the main thread at
> startup will help as too many places check to see if incognito is available on
> the UI thread.

I see, since this state is const once initialized, we could consider using
LazyInstance here with an object which initializes this const state in its
constructor.

LazyInstance being thread-safe, we could kick it off the initial initialization
early on a background thread and it would only be waited upon by the main thread
if/when the main thread needs it for real?

https://codereview.chromium.org/950053002/diff/40001/chrome/browser/prefs/inc...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/950053002/diff/40001/chrome/browser/prefs/inc...
chrome/browser/prefs/incognito_mode_prefs.cc:27: enum ParentalControlsState {
Consider using C++11 enum class, (I typically prefer them for the extra type
safety provided).

Also, since the enum becomes an explicit namespace, you can shorten the enum
values (e.g., UNINITIALIZED, DISABLED, ENABLED).

https://codereview.chromium.org/950053002/diff/40001/chrome/browser/prefs/inc...
chrome/browser/prefs/incognito_mode_prefs.cc:35: ParentalControlsState
g_parental_controls_state =
Merge with end of enum instead of re-declaring type, e.g.:

enum {
 ...
} g_parental_controls_state = PARENTAL_CONTROLS_STATE_UNKNOWN;

https://codereview.chromium.org/950053002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right):

https://codereview.chromium.org/950053002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_context_menu_model_unittest.cc:57:
IncognitoModePrefs::InitializePlatformParentalControls();
IMO this belongs in SetUpTestCase() in the test fixture (ref:
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...).

https://codereview.chromium.org/950053002/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/950053002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_unittest.cc:122:
IncognitoModePrefs::InitializePlatformParentalControls();
Consider putting this in SetUpTestCase() as it's once per process.

(same in all tests below)

[robliao, 2015-02-25 23:38:07]

Patchset #4 (id:100001) has been deleted

[robliao, 2015-02-26 00:47:33]

Patchset #4 (id:120001) has been deleted

[robliao, 2015-02-26 21:03:52]

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/950053002/diff/1/chrome/browser/prefs/incogni...
chrome/browser/prefs/incognito_mode_prefs.cc:106: void
IncognitoModePrefs::InitializePlatformParentalControls() {
On 2015/02/25 20:23:51, gab wrote:
> On 2015/02/24 23:43:01, robliao wrote:
> > On 2015/02/24 20:39:47, gab wrote:
> > > Also, should we not instead lazily initialize this (or perhaps a mix of
> > > lazy/delay initialization -- i.e. either initialize it asynchronously
> > > post-startup with something like PostTaskAndReplyWithResult() to the
> > > BlockingPool() or lazily when it's requested -- whichever kicks in first).
> > > 
> > > Doesn't make sense to me to call this unconditionally from
> > > ChromeBrowserMainPartsWin::PreMainMessageLoopStart().
> > 
> > It's oddly enough the best place for it since we need the value right when
we
> > fire off the renderers during startup. Separating I/O from the main thread
at
> > startup will help as too many places check to see if incognito is available
on
> > the UI thread.
> 
> I see, since this state is const once initialized, we could consider using
> LazyInstance here with an object which initializes this const state in its
> constructor.
> 
> LazyInstance being thread-safe, we could kick it off the initial
initialization
> early on a background thread and it would only be waited upon by the main
thread
> if/when the main thread needs it for real?

I'm going to have to think about this some more to see if this makes sense in
the grand scheme things, especially with the plans of getting I/O out of the
init thread.

https://codereview.chromium.org/950053002/diff/40001/chrome/browser/prefs/inc...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/950053002/diff/40001/chrome/browser/prefs/inc...
chrome/browser/prefs/incognito_mode_prefs.cc:27: enum ParentalControlsState {
On 2015/02/25 20:23:52, gab wrote:
> Consider using C++11 enum class, (I typically prefer them for the extra type
> safety provided).
> 
> Also, since the enum becomes an explicit namespace, you can shorten the enum
> values (e.g., UNINITIALIZED, DISABLED, ENABLED).

Done.

https://codereview.chromium.org/950053002/diff/40001/chrome/browser/prefs/inc...
chrome/browser/prefs/incognito_mode_prefs.cc:35: ParentalControlsState
g_parental_controls_state =
On 2015/02/25 20:23:51, gab wrote:
> Merge with end of enum instead of re-declaring type, e.g.:
> 
> enum {
>  ...
> } g_parental_controls_state = PARENTAL_CONTROLS_STATE_UNKNOWN;

There aren't many enum declarations that use this style, so we should probably
keep it as is:
https://code.google.com/p/chromium/codesearch#search/&q=%5E%7D%5C%20.%2B%5C%2...

https://codereview.chromium.org/950053002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_context_menu_model_unittest.cc (right):

https://codereview.chromium.org/950053002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_context_menu_model_unittest.cc:57:
IncognitoModePrefs::InitializePlatformParentalControls();
On 2015/02/25 20:23:52, gab wrote:
> IMO this belongs in SetUpTestCase() in the test fixture (ref:
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...).

Done for test classes. Global calls like
chrome::CreateBrowserWithTestWindowForParams will stay.

https://codereview.chromium.org/950053002/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/950053002/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_unittest.cc:122:
IncognitoModePrefs::InitializePlatformParentalControls();
On 2015/02/25 20:23:52, gab wrote:
> Consider putting this in SetUpTestCase() as it's once per process.
> 
> (same in all tests below)

Done.

[robliao, 2015-02-26 21:14:46]

robliao@chromium.org changed reviewers:
+ avi@chromium.org, kalman@chromium.org, msw@chromium.org, noms@chromium.org,
phajdan.jr@chromium.org

[robliao, 2015-02-26 21:14:47]

Test owners: Please provide owner approval for the mechanical changes in the
following files due to an assumptions update. Thanks!

kalman:
chrome/browser/extensions/extension_service_test_base.cc                 

noms:
chrome/browser/profiles/profile_manager_unittest.cc

avi:
chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc

msw:
chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc
chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc 

phajdan.jr:
chrome/test/base/browser_with_test_window_test.h
chrome/test/base/browser_with_test_window_test.cc
chrome/test/base/test_browser_window.cc

[robliao, 2015-02-26 22:49:13]

On 2015/02/26 21:14:47, robliao wrote:
> Test owners: Please provide owner approval for the mechanical changes in the
> following files due to an assumptions update. Thanks!
> 
> kalman:
> chrome/browser/extensions/extension_service_test_base.cc                 
> 
> noms:
> chrome/browser/profiles/profile_manager_unittest.cc
> 
> avi:
> chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc
> 
> msw:
> chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc
> chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc 
> 
> phajdan.jr:
> chrome/test/base/browser_with_test_window_test.h
> chrome/test/base/browser_with_test_window_test.cc
> chrome/test/base/test_browser_window.cc

FYI. I'm adding an state isolation mechanism to the tests, so hold off until the
all clear.

[not at google - send to devlin, 2015-02-26 23:36:12]

https://codereview.chromium.org/950053002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_base.cc (right):

https://codereview.chromium.org/950053002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_base.cc:183:
IncognitoModePrefs::InitializePlatformParentalControls();
Kinda weird for this arbitrary thing in Extensions - and for it to be Windows
only - can you add comment?

[msw, 2015-02-27 02:08:28]

https://codereview.chromium.org/950053002/diff/140001/chrome/browser/prefs/in...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/950053002/diff/140001/chrome/browser/prefs/in...
chrome/browser/prefs/incognito_mode_prefs.cc:115: void
IncognitoModePrefs::InitializePlatformParentalControls() {
Define this on all platforms and just do platform-specific work internally, like
ArePlatformParentalControlsEnabledCached. Then remove ifdefs from clients.

[robliao, 2015-02-27 02:14:02]

https://codereview.chromium.org/950053002/diff/140001/chrome/browser/prefs/in...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/950053002/diff/140001/chrome/browser/prefs/in...
chrome/browser/prefs/incognito_mode_prefs.cc:115: void
IncognitoModePrefs::InitializePlatformParentalControls() {
On 2015/02/27 02:08:28, msw wrote:
> Define this on all platforms and just do platform-specific work internally,
like
> ArePlatformParentalControlsEnabledCached. Then remove ifdefs from clients.

Agreed with this at this point. The change started out isolated and was platform
specific for the init. This will be in the next change.

[robliao, 2015-02-27 17:59:34]

Patchset #5 (id:160001) has been deleted

[robliao, 2015-02-27 21:54:52]

robliao@chromium.org changed reviewers:
+ oshima@chromium.org, skuhne@chromium.org

[robliao, 2015-02-27 21:54:52]

Okay. The tests are happy now.

https://codereview.chromium.org/950053002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_test_base.cc (right):

https://codereview.chromium.org/950053002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_test_base.cc:183:
IncognitoModePrefs::InitializePlatformParentalControls();
On 2015/02/26 23:36:12, kalman wrote:
> Kinda weird for this arbitrary thing in Extensions - and for it to be Windows
> only - can you add comment?

Added a comment.

[Avi (use Gerrit), 2015-02-27 21:56:27]

chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc LGTM

[robliao, 2015-02-27 21:57:01]

On 2015/02/27 21:56:27, Avi wrote:
> chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc LGTM

Misclicked on the mouse before sending the main blurb.

The story here is that Windows needs to set a bit before anyone checks for the
availability of incognito. This occurs before browser creation in production
code, otherwise it would have been nice to put this initialization with the
creation of the browser. Other tests here need it as part of a context menu
creation (since that sometimes checks for the availability of incognito).

The unit test mechanical updates simply initialize the system and uninitialize
it.

kalman:
chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
chrome/browser/extensions/api/management/management_api_unittest.cc
chrome/browser/extensions/extension_context_menu_model_unittest.cc

noms:
chrome/browser/profiles/profile_manager_unittest.cc

avi:
chrome/browser/renderer_context_menu/render_view_context_menu_unittest.cc

skuhne:
chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc

oshima:
chrome/browser/ui/ash/window_positioner_unittest.cc

msw:
chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc
chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc
chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc

phajdan.jr
chrome/test/base/browser_with_test_window_test.h
chrome/test/base/browser_with_test_window_test.cc

[oshima, 2015-02-27 22:27:54]

c/b/ui/ash lgtm

[Mr4D (OOO till 08-26), 2015-02-27 22:52:06]

chrome/browser/profiles lgtm

[msw, 2015-02-28 01:05:33]

I'm a little baffled that we need so many tests to explicitly trigger caching a
system value for icognito's internal use, and I wonder how future test writers
will ever know if they need to do this or not. There must be a more centralized
way of mocking out this value for tests, (perhaps with a flag for actually
checking the system value in actual tests of incognito), just caching the value
on first access, or something better than sprinkling this init everywhere guilty
of "Creating a browser" or "Checking for incognito availability".

https://codereview.chromium.org/950053002/diff/180001/chrome/browser/prefs/in...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/950053002/diff/180001/chrome/browser/prefs/in...
chrome/browser/prefs/incognito_mode_prefs.cc:126: g_parental_controls_state =
ParentalControlsState::UNKNOWN;
Why do tests have to do this? Will the system value actually change?

[robliao, 2015-02-28 01:16:31]

As IncognitoModePrefs is structured today, the only way I can think of to
autoinit this is to
bifurcate the initialization in the header file similar to how #if
defined(UNIT_TESTS) is conditionally
declared for the uninitialize. It further complicates the header file, but gets
the job done. If that sounds good, we could do that.

https://codereview.chromium.org/950053002/diff/180001/chrome/browser/prefs/in...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/950053002/diff/180001/chrome/browser/prefs/in...
chrome/browser/prefs/incognito_mode_prefs.cc:126: g_parental_controls_state =
ParentalControlsState::UNKNOWN;
On 2015/02/28 01:05:33, msw wrote:
> Why do tests have to do this? Will the system value actually change?

Turns out this state is shared between groups of test cases in a single process
(since this is a global).

This means if one test case initializes this, all other ones get the
initialization for free. Of course this means if you reorder the test cases or
run them in complete isolation, things will break. 

The cleanup here ensures the initialization only occurs once and tests that need
to do this will break if they don't perform the initialization.

[msw, 2015-02-28 02:39:14]

On 2015/02/28 01:16:31, robliao wrote:
> As IncognitoModePrefs is structured today, the only way I can think of to
> autoinit this is to
> bifurcate the initialization in the header file similar to how #if
> defined(UNIT_TESTS) is conditionally
> declared for the uninitialize. It further complicates the header file, but
gets
> the job done. If that sounds good, we could do that.

That might work, or you could just short-circuit the call to
base::win::IsParentalControlActivityLoggingOn in
IncognitoModePrefs::InitializePlatformParentalControls, using the same macro. Or
perhaps just set a reasonable default value, then have the real browser process
explicitly call something like IncognitoModePrefs::InitParentalControlsState()
to cache the actual system value (perhaps, I hate to say it, but "on startup").
I'm sure there are other (potentially better) patterns for avoiding Windows
system calls in unit tests; you might ask around (maybe cpu@?).

[gab, 2015-03-02 13:22:04]

On 2015/02/28 02:39:14, msw wrote:
> On 2015/02/28 01:16:31, robliao wrote:
> > As IncognitoModePrefs is structured today, the only way I can think of to
> > autoinit this is to
> > bifurcate the initialization in the header file similar to how #if
> > defined(UNIT_TESTS) is conditionally
> > declared for the uninitialize. It further complicates the header file, but
> gets
> > the job done. If that sounds good, we could do that.
> 
> That might work, or you could just short-circuit the call to
> base::win::IsParentalControlActivityLoggingOn in
> IncognitoModePrefs::InitializePlatformParentalControls, using the same macro.
Or
> perhaps just set a reasonable default value, then have the real browser
process
> explicitly call something like IncognitoModePrefs::InitParentalControlsState()
> to cache the actual system value (perhaps, I hate to say it, but "on
startup").
> I'm sure there are other (potentially better) patterns for avoiding Windows
> system calls in unit tests; you might ask around (maybe cpu@?).

I had the same gut feeling when doing the initially review thinking that really
there shouldn't be an init, rather the first get call should simply cache the
value for future ones (perhaps using a LazyInstance for thread-safety).

I originally held that thought back as I liked having to make an explicit call
for the expansive part of this API, but I now agree with Mike that this is ugly
in current tests and even worse in future tests if people start tripping this
while adding only mildly related tests.

One thing we could do is to auto-init on the first get call (with an
AssertIOAllowed() if the value is uninitialized to make sure it's initialized
early enough in practice in the browser -- i.e. we could also have an explicit
Init() call for the browser or the browser could simply call the getter without
using the return value to early init without requiring an explicit call).

WDYT?

[gab, 2015-03-02 17:09:24]

Oops and one unsent draft.

https://codereview.chromium.org/950053002/diff/40001/chrome/browser/prefs/inc...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/950053002/diff/40001/chrome/browser/prefs/inc...
chrome/browser/prefs/incognito_mode_prefs.cc:35: ParentalControlsState
g_parental_controls_state =
On 2015/02/26 21:03:51, robliao wrote:
> On 2015/02/25 20:23:51, gab wrote:
> > Merge with end of enum instead of re-declaring type, e.g.:
> > 
> > enum {
> >  ...
> > } g_parental_controls_state = PARENTAL_CONTROLS_STATE_UNKNOWN;
> 
> There aren't many enum declarations that use this style, so we should probably
> keep it as is:
>
https://code.google.com/p/chromium/codesearch#search/&q=%5E%7D%5C%20.%2B%5C%2...

Ah interesting, expanded regex a bit, found 15 instances in src/. I don't think
it applies in many situations, but I think this is a good example of where it
could (i.e. this variable is declared right after and is the only one of that
type in this file -- as highlighted by the lack of an empty line between the
two, these two really go together).

https://code.google.com/p/chromium/codesearch#search/&q=%5B%5Cs%5D*%7D%5C%20(...

[Paweł Hajdan Jr., 2015-03-02 18:50:54]

chrome/test LGTM

[robliao, 2015-03-02 21:55:50]

On 2015/03/02 13:22:04, gab wrote:
> On 2015/02/28 02:39:14, msw wrote:
> > On 2015/02/28 01:16:31, robliao wrote:
> > > As IncognitoModePrefs is structured today, the only way I can think of to
> > > autoinit this is to
> > > bifurcate the initialization in the header file similar to how #if
> > > defined(UNIT_TESTS) is conditionally
> > > declared for the uninitialize. It further complicates the header file, but
> > gets
> > > the job done. If that sounds good, we could do that.
> > 
> > That might work, or you could just short-circuit the call to
> > base::win::IsParentalControlActivityLoggingOn in
> > IncognitoModePrefs::InitializePlatformParentalControls, using the same
macro.
> Or
> > perhaps just set a reasonable default value, then have the real browser
> process
> > explicitly call something like
IncognitoModePrefs::InitParentalControlsState()
> > to cache the actual system value (perhaps, I hate to say it, but "on
> startup").
> > I'm sure there are other (potentially better) patterns for avoiding Windows
> > system calls in unit tests; you might ask around (maybe cpu@?).
> 
> I had the same gut feeling when doing the initially review thinking that
really
> there shouldn't be an init, rather the first get call should simply cache the
> value for future ones (perhaps using a LazyInstance for thread-safety).
> 
> I originally held that thought back as I liked having to make an explicit call
> for the expansive part of this API, but I now agree with Mike that this is
ugly
> in current tests and even worse in future tests if people start tripping this
> while adding only mildly related tests.
> 
> One thing we could do is to auto-init on the first get call (with an
> AssertIOAllowed() if the value is uninitialized to make sure it's initialized
> early enough in practice in the browser -- i.e. we could also have an explicit
> Init() call for the browser or the browser could simply call the getter
without
> using the return value to early init without requiring an explicit call).
> 
> WDYT?

To make sure I understand this correctly, is the proposal here to merge
InitializePlatformParentalControls and ArePlatformParentalControlsEnabledCached?

At pre browser startup, we simply substitute the call to seed the value and
carry on from there? That might be a workable solution for the tests since it
keeps all the preconditions the same. (One thread running around doing things).

[robliao, 2015-03-02 23:28:29]

On 2015/03/02 21:55:50, robliao wrote:
> On 2015/03/02 13:22:04, gab wrote:
> > On 2015/02/28 02:39:14, msw wrote:
> > > On 2015/02/28 01:16:31, robliao wrote:
> > > > As IncognitoModePrefs is structured today, the only way I can think of
to
> > > > autoinit this is to
> > > > bifurcate the initialization in the header file similar to how #if
> > > > defined(UNIT_TESTS) is conditionally
> > > > declared for the uninitialize. It further complicates the header file,
but
> > > gets
> > > > the job done. If that sounds good, we could do that.
> > > 
> > > That might work, or you could just short-circuit the call to
> > > base::win::IsParentalControlActivityLoggingOn in
> > > IncognitoModePrefs::InitializePlatformParentalControls, using the same
> macro.
> > Or
> > > perhaps just set a reasonable default value, then have the real browser
> > process
> > > explicitly call something like
> IncognitoModePrefs::InitParentalControlsState()
> > > to cache the actual system value (perhaps, I hate to say it, but "on
> > startup").
> > > I'm sure there are other (potentially better) patterns for avoiding
Windows
> > > system calls in unit tests; you might ask around (maybe cpu@?).
> > 
> > I had the same gut feeling when doing the initially review thinking that
> really
> > there shouldn't be an init, rather the first get call should simply cache
the
> > value for future ones (perhaps using a LazyInstance for thread-safety).
> > 
> > I originally held that thought back as I liked having to make an explicit
call
> > for the expansive part of this API, but I now agree with Mike that this is
> ugly
> > in current tests and even worse in future tests if people start tripping
this
> > while adding only mildly related tests.
> > 
> > One thing we could do is to auto-init on the first get call (with an
> > AssertIOAllowed() if the value is uninitialized to make sure it's
initialized
> > early enough in practice in the browser -- i.e. we could also have an
explicit
> > Init() call for the browser or the browser could simply call the getter
> without
> > using the return value to early init without requiring an explicit call).
> > 
> > WDYT?
> 
> To make sure I understand this correctly, is the proposal here to merge
> InitializePlatformParentalControls and
ArePlatformParentalControlsEnabledCached?
> 
> At pre browser startup, we simply substitute the call to seed the value and
> carry on from there? That might be a workable solution for the tests since it
> keeps all the preconditions the same. (One thread running around doing
things).

The discussed alternate is here: https://codereview.chromium.org/969813005/
If it's not too crazy, we may go with that instead.

[robliao, 2015-03-03 19:07:26]

I'm going to let https://codereview.chromium.org/969813005/ supersede this one.
Thanks!