Unify the Windows Parental Controls Platform Caching

chrome/browser/prefs/incognito_mode_prefs.cc

Index: chrome/browser/prefs/incognito_mode_prefs.cc
diff --git a/chrome/browser/prefs/incognito_mode_prefs.cc b/chrome/browser/prefs/incognito_mode_prefs.cc
index e0255d4947d11a90c3d2e5fbbd56f707459c811b..6e021bac3e7359101053723c8f9ac79a7d2bc573 100644
--- a/chrome/browser/prefs/incognito_mode_prefs.cc
+++ b/chrome/browser/prefs/incognito_mode_prefs.cc
@@ -7,23 +7,59 @@
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/prefs/pref_service.h"
+#include "base/threading/thread_restrictions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
+#include "content/public/browser/browser_thread.h"
 
 #if defined(OS_WIN)
-#include "base/win/metro.h"
+#include <windows.h>
+#include <wpcapi.h>
+#include "base/win/scoped_comptr.h"
+#include "base/win/windows_version.h"
 #endif  // OS_WIN
 
 #if defined(OS_ANDROID)
 #include "chrome/browser/android/chromium_application.h"
 #endif  // OS_ANDROID
 
+using content::BrowserThread;
+
 #if defined(OS_WIN)
 namespace {
 
-bool g_parental_control_on = false;
+// Returns true if Windows Parental control activity logging is enabled. This
+// feature is available on Windows 7 and beyond. This function should be called
+// on a COM Initialized thread and is potentially blocking.
+bool IsParentalControlActivityLoggingOn() {
+  using namespace base::win;

[grt (UTC plus 2), 2015/03/04 03:59:39]

this doesn't seem to reduce the need for wrapping on lines 47 and 53. please
remove and use:
  base::win::ScopedComPtr<IWindowsParentalControlsCore> parent_controls;
and
  base::win::ScopedComPtr<IWPCSettings> settings;
down there (unless i'm missing another place where it really does lead to
savings).

[robliao, 2015/03/04 18:47:44]

More of a style thing than anything, since that's what this is. I'm used to
seeing non-namespaced COM smart pointers.

+
+  // Since we can potentially block, make sure the thread is okay with this.
+  base::ThreadRestrictions::AssertIOAllowed();
+  base::ThreadRestrictions::AssertWaitAllowed();
+
+  // Query this info on Windows 7 and above.
+  if (base::win::GetVersion() < base::win::VERSION_WIN7)
+    return false;
+
+  ScopedComPtr<IWindowsParentalControlsCore> parent_controls;
+  HRESULT hr = parent_controls.CreateInstance(
+      __uuidof(WindowsParentalControls));
+  if (FAILED(hr))
+    return false;
+
+  ScopedComPtr<IWPCSettings> settings;
+  hr = parent_controls->GetUserSettings(nullptr, settings.Receive());
+  if (FAILED(hr))
+    return false;
+
+  unsigned long restrictions = 0;
+  settings->GetRestrictions(&restrictions);
+
+  return (restrictions & WPCFLAG_LOGGING_REQUIRED) == WPCFLAG_LOGGING_REQUIRED;
+}
 
 } // empty namespace

[grt (UTC plus 2), 2015/03/04 03:59:39]

while you're here, please change to:
}  // namespace

[robliao, 2015/03/04 18:47:44]

Done.

 #endif  // OS_WIN
@@ -104,25 +140,29 @@ bool IncognitoModePrefs::CanOpenBrowser(Profile* profile) {
 // static
 bool IncognitoModePrefs::ArePlatformParentalControlsEnabled() {
 #if defined(OS_WIN)
-  // Disable incognito mode windows if parental controls are on. This is only
-  // for Windows Vista and above.
-  return base::win::IsParentalControlActivityLoggingOn();
-#elif defined(OS_ANDROID)
-  return chrome::android::ChromiumApplication::AreParentalControlsEnabled();
-#else
-  return false;
-#endif
-}
-
-#if defined(OS_WIN)
-void IncognitoModePrefs::InitializePlatformParentalControls() {
-  g_parental_control_on = base::win::IsParentalControlActivityLoggingOn();
-}
-#endif // OS_WIN
-
-bool IncognitoModePrefs::ArePlatformParentalControlsEnabledCached() {
-#if defined(OS_WIN)
-  return g_parental_control_on;
+  // Possible values for the parental controls state.

[gab, 2015/03/04 16:25:23]

Consider removing this comment, feels redundant (i.e. that's always what an enum
is -- a set of possible value for the thing it's named after).

[robliao, 2015/03/04 18:47:44]

Just following the style set by enums in the neighboring .h file. Some folks are
a stickler for this. I would prefer to remove it along with the annotating
comments since they don't add value.

+  static enum class ParentalControlsState {

[gab, 2015/03/04 16:25:23]

Move static keyword right before variable declaration (I know both mean the same
thing, but I prefer reading it where it's applied (on the variable) then on the
enum), e.g.:

enum class ... {
  ...
} static parental_controls_state = ...;

[grt (UTC plus 2), 2015/03/04 16:45:12]

Not sure I agree here. You would never write
  int static foo = ...;
If you don't like the static at the front (I don't blame you), then I suggesting
changing to:
  enum class Blah {
    ...
  };
  static Blah blah = ...;

[gab, 2015/03/04 16:53:19]

I see your point, I still prefer my proposal though, but don't care strongly
either way.

I find
static enum class {
  ...
} var = FOO;
hides the static keyword (despite it being in proper order as far as typical
<keywords> <type> <var_name> = <value> order).

Where as having an extra line for an explicit variable declaration feels
overkill when you only need one variable of this type.

But I guess I'm fine with anything but having the static keyword before the enum
declaration which hinders readability IMO as it's so far from the variable
declaration.

[robliao, 2015/03/04 18:47:44]

A search from the earlier proposal revealed that this inline type + variable
declaration isn't used very often.

The C++ style guide doesn't have a ruling on either declaration order or type +
variable declaration other than to be consistent. I personally find declaring a
type and the variable separately more readable and more reusable should we
decide to use this later as it disrupts less code.

The location of the static keyword stops being an issue with separation, so I
think we should go with that.

[gab, 2015/03/04 19:21:20]

Okay, sgtm.

+    // Parental controls state unknown. We have not queried for this value yet.
+    UNKNOWN = 0,
+    // Parental controls activity logging disabled.
+    ACTIVITY_LOGGING_DISABLED = 1,
+    // Parental controls activity logging enabled.
+    ACTIVITY_LOGGING_ENABLED = 2,
+  } g_parental_controls_state = ParentalControlsState::UNKNOWN;

[grt (UTC plus 2), 2015/03/04 03:59:39]

no need for g_ since this is no longer a global

[robliao, 2015/03/04 18:47:44]

Missed that. Thanks.

+  if (g_parental_controls_state == ParentalControlsState::UNKNOWN) {
+    // Production: The thread isn't initialized, so we're the only thread that
+    // should be able to update this.
+    // Test: The thread may be initialized, so check that it's the UI thread.
+    DCHECK(
+        !BrowserThread::IsThreadInitialized(BrowserThread::UI) ||

[grt (UTC plus 2), 2015/03/04 03:59:39]

formatting nit:
    DCHECK(!BrowserThread::IsThreadInitialized(BrowserThread::UI) ||
	   BrowserThread::CurrentlyOn(BrowserThread::UI));

[robliao, 2015/03/04 18:47:44]

Done.

+        BrowserThread::CurrentlyOn(BrowserThread::UI));
+    g_parental_controls_state =
+        IsParentalControlActivityLoggingOn() ?
+            ParentalControlsState::ACTIVITY_LOGGING_ENABLED :
+            ParentalControlsState::ACTIVITY_LOGGING_DISABLED;
+  }
+  return g_parental_controls_state ==
+      ParentalControlsState::ACTIVITY_LOGGING_ENABLED;
 #elif defined(OS_ANDROID)
   return chrome::android::ChromiumApplication::AreParentalControlsEnabled();
 #else