[Re-landing] Migrate external protocol prefs from local state to profiles

chrome/browser/external_protocol/external_protocol_handler.cc

Index: chrome/browser/external_protocol/external_protocol_handler.cc
diff --git a/chrome/browser/external_protocol/external_protocol_handler.cc b/chrome/browser/external_protocol/external_protocol_handler.cc
index d629ddd98ff24f7f1c2f9bf67f78d26ff9dc560f..d4ce3d3fcb8a37730bc9d59f20492a3e491943b5 100644
--- a/chrome/browser/external_protocol/external_protocol_handler.cc
+++ b/chrome/browser/external_protocol/external_protocol_handler.cc
@@ -52,11 +52,12 @@ scoped_refptr<shell_integration::DefaultProtocolClientWorker> CreateShellWorker(
 
 ExternalProtocolHandler::BlockState GetBlockStateWithDelegate(
     const std::string& scheme,
-    ExternalProtocolHandler::Delegate* delegate) {
+    ExternalProtocolHandler::Delegate* delegate,
+    Profile* profile) {
   if (!delegate)
-    return ExternalProtocolHandler::GetBlockState(scheme);
+    return ExternalProtocolHandler::GetBlockState(scheme, profile);
 
-  return delegate->GetBlockState(scheme);
+  return delegate->GetBlockState(scheme, profile);
 }
 
 void RunExternalProtocolDialogWithDelegate(
@@ -135,7 +136,7 @@ void OnDefaultProtocolClientWorkerFinished(
 
 // static
 ExternalProtocolHandler::BlockState ExternalProtocolHandler::GetBlockState(
-    const std::string& scheme) {
+    const std::string& scheme, Profile* profile) {
   // If we are being carpet bombed, block the request.
   if (!g_accept_requests)
     return BLOCK;
@@ -150,16 +151,35 @@ ExternalProtocolHandler::BlockState ExternalProtocolHandler::GetBlockState(
   // Check the stored prefs.
   // TODO(pkasting): This kind of thing should go in the preferences on the

[dominickn, 2017/01/19 07:01:22]

Nit: you can remove this TODO, because you're doing it. :) In its place, you can
say:

// Check if there are any prefs in the local state. If there are, wipe them, and
migrate the prefs to the profile.
// TODO(ramyasharma) remove the migration in M61.

[ramyasharma, 2017/01/20 04:04:45]

Done.

   // profile, not in the local state. http://crbug.com/457254
-  PrefService* pref = g_browser_process->local_state();
-  if (pref) {  // May be NULL during testing.
-    DictionaryPrefUpdate update_excluded_schemas(pref, prefs::kExcludedSchemes);
-
-    // Warm up the dictionary if needed.
-    PrepopulateDictionary(update_excluded_schemas.Get());
+  PrefService* localPref = g_browser_process->local_state();

[dominickn, 2017/01/19 07:01:22]

Nit: variables use underscore_naming, so call this local_state_prefs

[ramyasharma, 2017/01/20 04:04:45]

Done.

+  PrefService* profilePref = profile->GetPrefs();

[dominickn, 2017/01/19 07:01:22]

Call this profile_prefs

[ramyasharma, 2017/01/20 04:04:45]

Done.

+  if (localPref && profilePref) {  // May be NULL during testing.
+    DictionaryPrefUpdate update_excluded_schemas(

[dominickn, 2017/01/19 07:01:22]

Call this local_state_schemas, since we're not actually going to update it any
more?

[ramyasharma, 2017/01/20 04:04:45]

Done.

+        localPref, prefs::kExcludedSchemes);
+    DictionaryPrefUpdate update_excluded_schemas_profile(
+        profilePref, prefs::kExcludedSchemes);
+    if (update_excluded_schemas_profile->empty()) {
+      // Copy local state to profile state.
+      for (base::DictionaryValue::Iterator it(*update_excluded_schemas);
+           !it.IsAtEnd(); it.Advance()) {
+        bool is_blocked;
+        // Discard local state if set to blocked, to reset all users
+        // stuck in 'Do Nothing' + 'Do Not Open' state back to the default
+        // prompt state.
+        if (it.value().GetAsBoolean(&is_blocked) && !is_blocked) {

[dominickn, 2017/01/19 07:01:22]

Nit: one-line if statements don't need braces

[ramyasharma, 2017/01/20 04:04:45]

Done. I was wondering about that, why is it not? Until now I have been following
the rule that always add braces even if its one line, that ensures no bugs are
introduced later because of missing braces.

[dominickn, 2017/01/20 05:07:46]

I used to follow that rule too, but this style is much more prevalent in
Chromium, so for consistency I usually suggest doing it this way. I think it's
just "one of the opinions" :)

+          update_excluded_schemas_profile->SetBoolean(it.key(), is_blocked);
+        }
+      }
+     // TODO(ramyasharma): Clear only if required.
+     localPref->ClearPref(prefs::kExcludedSchemes);
+    }
+    // Prepolutate the default states each time.

[dominickn, 2017/01/19 07:01:22]

sp. "Prepopulate"

[ramyasharma, 2017/01/20 04:04:45]

Oops, done.

+    PrepopulateDictionary(update_excluded_schemas_profile.Get());
 
     bool should_block;
-    if (update_excluded_schemas->GetBoolean(scheme, &should_block))
+    if (update_excluded_schemas_profile->GetBoolean(scheme, &should_block)) {

[dominickn, 2017/01/19 07:01:22]

Nit: don't add the braces here, one line if statements don't need them

[ramyasharma, 2017/01/20 04:04:45]

Done.

       return should_block ? BLOCK : DONT_BLOCK;
+    }
   }
 
   return UNKNOWN;
@@ -167,18 +187,28 @@ ExternalProtocolHandler::BlockState ExternalProtocolHandler::GetBlockState(
 
 // static
 void ExternalProtocolHandler::SetBlockState(const std::string& scheme,
-                                            BlockState state) {
+                                            BlockState state,
+                                            Profile* profile) {
   // Set in the stored prefs.
   // TODO(pkasting): This kind of thing should go in the preferences on the

[dominickn, 2017/01/19 07:01:22]

Nit: remove this TODO (you're doing it)

[ramyasharma, 2017/01/20 04:04:45]

Done.

   // profile, not in the local state. http://crbug.com/457254
-  PrefService* pref = g_browser_process->local_state();
-  if (pref) {  // May be NULL during testing.
-    DictionaryPrefUpdate update_excluded_schemas(pref, prefs::kExcludedSchemes);
-
+  PrefService* localPref = g_browser_process->local_state();
+  PrefService* profilePref = profile->GetPrefs();

[dominickn, 2017/01/19 07:01:22]

local_state_prefs and profile_prefs

[ramyasharma, 2017/01/20 04:04:45]

Done.

+  if (localPref && profilePref) {  // May be NULL during testing.
+    DictionaryPrefUpdate update_excluded_schemas(
+        localPref, prefs::kExcludedSchemes);
+    DictionaryPrefUpdate update_excluded_schemas_profile(
+        profilePref, prefs::kExcludedSchemes);
+    bool is_profile_state = !update_excluded_schemas_profile->empty();
     if (state == UNKNOWN) {
-      update_excluded_schemas->Remove(scheme, NULL);
-    } else {
-      update_excluded_schemas->SetBoolean(scheme, (state == BLOCK));
+      is_profile_state

[dominickn, 2017/01/19 07:01:22]

What do you think about migrating here as well? My feeling is that every time
SetBlockState would be called, we would have initially called GetBlockState and
it would have been migrated already right?

[ramyasharma, 2017/01/20 04:04:45]

Yes, ideally that's how it is. Removed code that updates local state, and using
only profile state to update these prefs.

+          ? update_excluded_schemas_profile->Remove(scheme, NULL)
+          : update_excluded_schemas->Remove(scheme, NULL);
+      } else {
+      is_profile_state

[dominickn, 2017/01/19 07:01:22]

Nit: indentation.

[ramyasharma, 2017/01/20 04:04:45]

Done.

+          ? update_excluded_schemas_profile->SetBoolean(
+              scheme, (state == BLOCK))
+          :update_excluded_schemas->SetBoolean(scheme, (state == BLOCK));

[dominickn, 2017/01/19 07:01:22]

Nit: space after :

[ramyasharma, 2017/01/20 04:04:45]

Done.

     }
   }
 }
@@ -197,8 +227,15 @@ void ExternalProtocolHandler::LaunchUrlWithDelegate(
   // have parameters unexpected by the external program.
   std::string escaped_url_string = net::EscapeExternalHandlerValue(url.spec());
   GURL escaped_url(escaped_url_string);
+
+  content::WebContents* web_contents = tab_util::GetWebContentsByID(
+      render_process_host_id, render_view_routing_id);
+  Profile* profile = NULL;

[dominickn, 2017/01/19 07:01:22]

Use nullptr instead of NULL (this is a very old file so it references NULL
everywhere)

[ramyasharma, 2017/01/20 04:04:45]

Done.

+  if (web_contents) { // Maybe NULL during testing.

[dominickn, 2017/01/19 07:01:22]

Nit: no braces around a one-line else.

[ramyasharma, 2017/01/20 04:04:45]

Done.

+    profile = Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  }
   BlockState block_state =
-      GetBlockStateWithDelegate(escaped_url.scheme(), delegate);
+      GetBlockStateWithDelegate(escaped_url.scheme(), delegate, profile);
   if (block_state == BLOCK) {
     if (delegate)
       delegate->BlockRequest();
@@ -243,11 +280,6 @@ void ExternalProtocolHandler::PermitLaunchUrl() {
 // static
 void ExternalProtocolHandler::PrepopulateDictionary(
     base::DictionaryValue* win_pref) {
-  static bool is_warm = false;
-  if (is_warm)
-    return;
-  is_warm = true;
-
   static const char* const denied_schemes[] = {
     "afp",
     "data",
@@ -299,5 +331,5 @@ void ExternalProtocolHandler::RecordMetrics(bool selected) {
 
 // static
 void ExternalProtocolHandler::RegisterPrefs(PrefRegistrySimple* registry) {
-  registry->RegisterDictionaryPref(prefs::kExcludedSchemes);
+ registry->RegisterDictionaryPref(prefs::kExcludedSchemes);
 }