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

chrome/browser/external_protocol/external_protocol_handler.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/external_protocol/external_protocol_handler.h"
 
 #include <stddef.h>
 
 #include <set>
 
@@ skipping 34 matching lines @@
     const std::string& protocol,
     ExternalProtocolHandler::Delegate* delegate) {
   if (delegate)
     return delegate->CreateShellWorker(callback, protocol);
 
   return new shell_integration::DefaultProtocolClientWorker(callback, protocol);
 }
 
 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(
     const GURL& url,
     int render_process_host_id,
     int routing_id,
     ui::PageTransition page_transition,
     bool has_user_gesture,
     ExternalProtocolHandler::Delegate* delegate) {
   if (!delegate) {
@@ skipping 58 matching lines @@
   }
 
   LaunchUrlWithoutSecurityCheckWithDelegate(escaped_url, render_process_host_id,
                                             render_view_routing_id, delegate);
 }
 
 }  // namespace
 
 // 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;
 
   if (scheme.length() == 1) {
     // We have a URL that looks something like:
     //   C:/WINDOWS/system32/notepad.exe
     // ShellExecuting this URL will cause the specified program to be executed.
     return BLOCK;
   }
 
   // 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;
 }
 
 // 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.

     }
   }
 }
 
 // static
 void ExternalProtocolHandler::LaunchUrlWithDelegate(
     const GURL& url,
     int render_process_host_id,
     int render_view_routing_id,
     ui::PageTransition page_transition,
     bool has_user_gesture,
     Delegate* delegate) {
   DCHECK(base::MessageLoopForUI::IsCurrent());
 
   // Escape the input scheme to be sure that the command does not
   // 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();
     return;
   }
 
   g_accept_requests = false;
 
   // The worker creates tasks with references to itself and puts them into
   // message loops.
@@ skipping 24 matching lines @@
 
 // static
 void ExternalProtocolHandler::PermitLaunchUrl() {
   DCHECK(base::MessageLoopForUI::IsCurrent());
   g_accept_requests = true;
 }
 
 // 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",
     "disk",
     "disks",
     // ShellExecuting file:///C:/WINDOWS/system32/notepad.exe will simply
     // execute the file specified!  Hopefully we won't see any "file" schemes
     // because we think of file:// URLs as handled URLs, but better to be safe
     // than to let an attacker format the user's hard drive.
     "file",
@@ skipping 31 matching lines @@
 }
 
 // static
 void ExternalProtocolHandler::RecordMetrics(bool selected) {
   UMA_HISTOGRAM_BOOLEAN("BrowserDialogs.ExternalProtocol.RememberCheckbox",
                         selected);
 }
 
 // static
 void ExternalProtocolHandler::RegisterPrefs(PrefRegistrySimple* registry) {
-  registry->RegisterDictionaryPref(prefs::kExcludedSchemes);
+ registry->RegisterDictionaryPref(prefs::kExcludedSchemes);
 }