Import policy data into default search dictionary pref.

Import policy data into default search dictionary pref. 


BUG=365762
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267002

[gab, 2014-04-15 20:52:33]

Very nice, I think we should also add TODOs in every CL we write above the code
that will be removed/simplified once this is fully live (i.e. in this case most
of the pre-existing policy code).

This will help show that we aren't introducing additional logic, but rather
replacing cruft.

Cheers!
Gab

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:52: const char
kCreatedByPolicy[] = "created_by_policy";
We should avoid duplicating these again here (IIRC you have all of these strings
somewhere else; I assume they're identical?).

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:157: 
nit: rm extra empty line

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:243: void
SetListInPref(const PolicyMap& policies, base::DictionaryValue* dict,
out-param last

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:248:
value->GetAsList(&list);
I would prefer to check that this is indeed a list (and be prepared if it isn't
one -- as this method is called based on the expected type, not the type that
was actually read).

Also, this will leak the |list| you created above if GetAsList() succeeds.

I instead suggest something like:

const base::Value* policy_value = policies.GetValue(policy_name);
const base::ListValue* policy_list = NULL;
if (policy_value) {
  bool is_list = value->GetAsList(&policy_list);
  DCHECK(is_list);
}

dict->Set(key, policy_list ? policy_list->DeepCopy() : new base::ListValue());

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:255: std::string
str = std::string();
No need to explicitly initialize:

std::string str;

is equivalent.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:257:
value->GetAsString(&str);
DCHECK return value.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:265: // Set data
to default values.
TODO?

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:287: }
Clang will complain about not all cases being handled.

Add a 
default: 
  NOTREACHED(); 
  break;
block.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:292:
dict->SetString(kOriginatingURL, std::string());
Add comments where necessary above/below for the reasoning behind these
hardcoded values.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/default_search_policy_handler.h (right):

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.h:42: void
HandleDictionaryPref(const PolicyMap& policies, PrefValueMap* prefs);
nit: add empty line below.

[gab, 2014-04-15 21:01:38]

+erik FYI

[Cait (Slow), 2014-04-16 21:14:11]

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:52: const char
kCreatedByPolicy[] = "created_by_policy";
On 2014/04/15 20:52:33, gab wrote:
> We should avoid duplicating these again here (IIRC you have all of these
strings
> somewhere else; I assume they're identical?).

Done.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:157: 
On 2014/04/15 20:52:33, gab wrote:
> nit: rm extra empty line

Done.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:243: void
SetListInPref(const PolicyMap& policies, base::DictionaryValue* dict,
On 2014/04/15 20:52:33, gab wrote:
> out-param last

Done.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:248:
value->GetAsList(&list);
On 2014/04/15 20:52:33, gab wrote:
> I would prefer to check that this is indeed a list (and be prepared if it
isn't
> one -- as this method is called based on the expected type, not the type that
> was actually read).
> 
> Also, this will leak the |list| you created above if GetAsList() succeeds.
> 
> I instead suggest something like:
> 
> const base::Value* policy_value = policies.GetValue(policy_name);
> const base::ListValue* policy_list = NULL;
> if (policy_value) {
>   bool is_list = value->GetAsList(&policy_list);
>   DCHECK(is_list);
> }
> 
> dict->Set(key, policy_list ? policy_list->DeepCopy() : new base::ListValue());

Done.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:255: std::string
str = std::string();
On 2014/04/15 20:52:33, gab wrote:
> No need to explicitly initialize:
> 
> std::string str;
> 
> is equivalent.

Done.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:257:
value->GetAsString(&str);
On 2014/04/15 20:52:33, gab wrote:
> DCHECK return value.

Done.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:265: // Set data
to default values.
On 2014/04/15 20:52:33, gab wrote:
> TODO?

Done.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:287: }
On 2014/04/15 20:52:33, gab wrote:
> Clang will complain about not all cases being handled.
> 
> Add a 
> default: 
>   NOTREACHED(); 
>   break;
> block.

Done.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.cc:292:
dict->SetString(kOriginatingURL, std::string());
On 2014/04/15 20:52:33, gab wrote:
> Add comments where necessary above/below for the reasoning behind these
> hardcoded values.

These may go away if we decide there's no point in setting them here. I've added
a todo to add comments or remove hardcoding.

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
File chrome/browser/search_engines/default_search_policy_handler.h (right):

https://codereview.chromium.org/237653002/diff/1/chrome/browser/search_engine...
chrome/browser/search_engines/default_search_policy_handler.h:42: void
HandleDictionaryPref(const PolicyMap& policies, PrefValueMap* prefs);
On 2014/04/15 20:52:33, gab wrote:
> nit: add empty line below.

Done.

[Cait (Slow), 2014-04-28 20:04:18]

Hi Peter, PTAL:
This is the next step in the default search migration and handles importing
policy data into the new pref. 

Thanks!

[Peter Kasting, 2014-04-29 01:47:32]

LGTM

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
File chrome/browser/search_engines/default_search_manager.h (right):

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
chrome/browser/search_engines/default_search_manager.h:66:
base::DictionaryValue* value);
Nit: Reversing the two args would read more like the function name.

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
File chrome/browser/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
chrome/browser/search_engines/default_search_policy_handler.cc:25: // Extract a
list from a policy value and add it to a pref dictionary.
Nit: Extract -> Extracts (and similar with "add") (2 places)

Frankly, these are short enough that I'd probably inline them into the lone
caller, but up to you.

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
chrome/browser/search_engines/default_search_policy_handler.cc:236:
DefaultSearchManager::AddPrefValueToMap(prefs, dict.release());
Nit: Early return and remove "else" below, to avoid indenting almost the whole
function

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
chrome/browser/search_engines/default_search_policy_handler.cc:283: // there is
no host (file: URLs?  Not sure), use "_" to guarantee that the
Right, I believe file: URLs won't have a host.  You should check this (and you
should have a unittest covering it!) and then firm up the comment.

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
File chrome/browser/search_engines/default_search_policy_handler_unittest.cc
(right):

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
chrome/browser/search_engines/default_search_policy_handler_unittest.cc:376:
POLICY_SCOPE_USER, base::Value::CreateBooleanValue(false), NULL);
Nit: Indenting

[Cait (Slow), 2014-04-29 17:14:51]

Thanks!

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
File chrome/browser/search_engines/default_search_manager.h (right):

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
chrome/browser/search_engines/default_search_manager.h:66:
base::DictionaryValue* value);
On 2014/04/29 01:47:32, Peter Kasting wrote:
> Nit: Reversing the two args would read more like the function name.

Done.

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
File chrome/browser/search_engines/default_search_policy_handler.cc (right):

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
chrome/browser/search_engines/default_search_policy_handler.cc:25: // Extract a
list from a policy value and add it to a pref dictionary.
On 2014/04/29 01:47:32, Peter Kasting wrote:
> Nit: Extract -> Extracts (and similar with "add") (2 places)
> 
> Frankly, these are short enough that I'd probably inline them into the lone
> caller, but up to you.

Done.

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
chrome/browser/search_engines/default_search_policy_handler.cc:236:
DefaultSearchManager::AddPrefValueToMap(prefs, dict.release());
On 2014/04/29 01:47:32, Peter Kasting wrote:
> Nit: Early return and remove "else" below, to avoid indenting almost the whole
> function

Done.

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
chrome/browser/search_engines/default_search_policy_handler.cc:283: // there is
no host (file: URLs?  Not sure), use "_" to guarantee that the
On 2014/04/29 01:47:32, Peter Kasting wrote:
> Right, I believe file: URLs won't have a host.  You should check this (and you
> should have a unittest covering it!) and then firm up the comment.

Done.

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
File chrome/browser/search_engines/default_search_policy_handler_unittest.cc
(right):

https://codereview.chromium.org/237653002/diff/90001/chrome/browser/search_en...
chrome/browser/search_engines/default_search_policy_handler_unittest.cc:376:
POLICY_SCOPE_USER, base::Value::CreateBooleanValue(false), NULL);
On 2014/04/29 01:47:32, Peter Kasting wrote:
> Nit: Indenting

Done.

[Cait (Slow), 2014-04-29 17:16:27]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-04-29 17:17:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/237653002/110001

[commit-bot: I haz the power, 2014-04-29 18:04:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 18:04:31]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium

[Cait (Slow), 2014-04-29 18:08:32]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-04-29 18:11:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/237653002/110001

[commit-bot: I haz the power, 2014-04-30 00:19:51]

Change committed as 267002