Optimize ShouldLaunchIncognito() since its on the startup path.

chrome/browser/prefs/incognito_mode_prefs.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 
 #include <stdint.h>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 124 matching lines @@
     *out_value = ENABLED;
     return false;
   }
   *out_value = static_cast<Availability>(in_value);
   return true;
 }
 
 // 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
 void IncognitoModePrefs::SetAvailability(PrefService* prefs,
                                          const Availability availability) {
   prefs->SetInteger(prefs::kIncognitoModeAvailability, availability);
 }
 
 // static
 void IncognitoModePrefs::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterIntegerPref(prefs::kIncognitoModeAvailability,
                                 IncognitoModePrefs::ENABLED);
 }
 
 // static
 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
 bool IncognitoModePrefs::CanOpenBrowser(Profile* profile) {
   if (profile->IsGuestSession())
     return true;
 
   switch (GetAvailability(profile->GetPrefs())) {
     case IncognitoModePrefs::ENABLED:
       return true;
@@ skipping 24 matching lines @@
 bool IncognitoModePrefs::ArePlatformParentalControlsEnabled() {
 #if defined(OS_WIN)
   return PlatformParentalControlsValue::GetInstance()->is_enabled();
 #elif BUILDFLAG(ANDROID_JAVA_UI)
   return chrome::android::ChromeApplication::AreParentalControlsEnabled();
 #else
   return false;
 #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;
+}