Optimize ShouldLaunchIncognito() since its on the startup path.

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 2e20d04d8d1230f2863471a010289aefa901f704..5f47eb967ff8fce2b5fc243e09e63c9f9b332714 100644
--- a/chrome/browser/prefs/incognito_mode_prefs.cc
+++ b/chrome/browser/prefs/incognito_mode_prefs.cc
@@ -142,17 +142,7 @@ bool IncognitoModePrefs::IntToAvailability(int in_value,
 // static
 IncognitoModePrefs::Availability IncognitoModePrefs::GetAvailability(
     const PrefService* pref_service) {
-  DCHECK(pref_service);
-  int pref_value = pref_service->GetInteger(prefs::kIncognitoModeAvailability);
-  Availability result = IncognitoModePrefs::ENABLED;
-  bool valid = IntToAvailability(pref_value, &result);
-  DCHECK(valid);
-  if (ArePlatformParentalControlsEnabled()) {
-    if (result == IncognitoModePrefs::FORCED)
-      LOG(ERROR) << "Ignoring FORCED incognito. Parental control logging on";
-    return IncognitoModePrefs::DISABLED;
-  }
-  return result;
+  return GetAvailabilityInternal(pref_service, true);

[robliao, 2016/09/30 17:43:46]

Nit: true -> enum. Something like CHECK_PARENTAL_CONTROLS and
DONT_CHECK_PARENTAL_CONTROLS.

[Alexei Svitkine (slow), 2016/09/30 20:16:16]

Done.

 }
 
 // static
@@ -172,10 +162,14 @@ void IncognitoModePrefs::RegisterProfilePrefs(
 bool IncognitoModePrefs::ShouldLaunchIncognito(
     const base::CommandLine& command_line,
     const PrefService* prefs) {
-  Availability incognito_avail = GetAvailability(prefs);
-  return incognito_avail != IncognitoModePrefs::DISABLED &&
-         (command_line.HasSwitch(switches::kIncognito) ||
-          incognito_avail == IncognitoModePrefs::FORCED);
+  // Note: The following code structure ensures we don't query

[robliao, 2016/09/30 18:26:13]

I would make this more explicit:
We only absolutely need to check for parental controls in the startup path if
the user requested to launch in incognito mode since computing parental controls
can be slow. Otherwise, we can check later.

[Alexei Svitkine (slow), 2016/09/30 20:16:16]

Done.

+  // parental controls on the normal code path - since querying
+  // them is slow.
+  if (command_line.HasSwitch(switches::kIncognito) &&
+      GetAvailability(prefs) != IncognitoModePrefs::DISABLED) {

[robliao, 2016/09/30 18:26:13]

This is also used here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_commands.cc?q=...

It looks like this uses the CrOS path, so this shouldn't impact it too much.
Might be worth commenting in browser_commands for future folks who potentially
change the code.

Taking a step back, that area looks suspect as it allows Incognito if Chrome is
simply in a guest session, even if parental controls are enabled.

[gab, 2016/09/30 18:47:20]

I think this would be more readable as GetAvailabilityInternal(prefs, true), it
others took me some time to match the comment to the code (have to realize that
GetAvailability() is implemented to always consider parental controls).

[Alexei Svitkine (slow), 2016/09/30 20:16:16]

Done.

+    return true;
+  }
+  return GetAvailabilityInternal(prefs, false) == IncognitoModePrefs::FORCED;

[gab, 2016/09/30 18:47:20]

Won't this launch incognito if the pref is set to FORCED but parental controls
disallow it (and there isn't the incognito switch)?

I don't this it's possible to launch incognito without checking parental
controls.

i.e. the way I see to make this work is: decide whether to launch incognito w/o
parental controls, if incognito launch is desired (switch or FORCED) then check
parental controls.

[Alexei Svitkine (slow), 2016/09/30 18:55:08]

You're right - good catch. Let me re-work the logic as you suggest.

[Alexei Svitkine (slow), 2016/09/30 20:16:16]

Done.

 }
 
 // static
@@ -220,3 +214,19 @@ bool IncognitoModePrefs::ArePlatformParentalControlsEnabled() {
 #endif
 }
 
+// static
+IncognitoModePrefs::Availability IncognitoModePrefs::GetAvailabilityInternal(

[robliao, 2016/09/30 18:26:13]

This can be part of the anonymous namespace, right?

[Alexei Svitkine (slow), 2016/09/30 20:16:16]

I tried it this way, but it was referencing too many things from the class so I
had to prefix a bunch pf things like Availability, IntToAvailability,
ArePlatformParentalControlsEnabled with the class name - which seemed like it
would be better to keep static so it's more concise. Let me know if you prefer I
still make it anon.

[gab, 2016/10/03 17:08:11]

Since it's already a class with only static methods that sounds fine to me (i.e.
if it was instead a namespace you would put this private method in the .cc in an
anon namespace inside that namespace but classes with static methods don't allow
this).

+    const PrefService* pref_service,
+    bool check_parental_controls) {
+  DCHECK(pref_service);
+  int pref_value = pref_service->GetInteger(prefs::kIncognitoModeAvailability);
+  Availability result = IncognitoModePrefs::ENABLED;
+  bool valid = IntToAvailability(pref_value, &result);
+  DCHECK(valid);
+  if (check_parental_controls && ArePlatformParentalControlsEnabled()) {

[gab, 2016/09/30 18:47:20]

Also only need to check if |result != IncognitoModePrefs::DISABLED|

[Alexei Svitkine (slow), 2016/09/30 20:16:16]

Done.

+    if (result == IncognitoModePrefs::FORCED)
+      LOG(ERROR) << "Ignoring FORCED incognito. Parental control logging on";
+    return IncognitoModePrefs::DISABLED;
+  }
+  return result;
+}