Added browser test for TemplateUrlService protected prefs

Added browser test for TemplateUrlService protected prefs

Create browser test that checks protection of preferences that are used
to define default search provider. Currently where are tests that check
protected prefs mechanism but no tests for preferences that are used in
TemplateUrlService to store or override default search provider
settings.

BUG=668139
R=pkasting@chromium.org, gab@chromium.org

Committed: https://crrev.com/38416868dc6ab9b4ae6b56ec4006d601c47704f0
Cr-Commit-Position: refs/heads/master@{#437211}

[Peter Kasting, 2016-11-23 18:49:14]

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
File
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
(right):

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:29:
// Seed is empty string in tests.
Nit: I would just pass std::string() directly in the caller.

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:36:
rlz_lib::GetMachineId(&device_id);
Why do we need to compute this?  Can't we just pass an empty string for both the
seed and the device ID?

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:55:
};
Nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:86:
DCHECK(dir_exists);
Nit: Avoid [D]CHECK in test code (failed checks cause tests to terminate
ungracefully and leave bots in a bad state).  Some ways to do this:

* Just omit the DCHECK
* Return nullptr if !dir_exists (caller will assert)
* EXPECT that this is true, then return nullptr
* Change function to return profile via outparam, then ASSERT that dir_exists
here (ASSERT can only be used in a function that returns void)

(3 places)

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:90:
base::ReplaceSubstringsAfterOffset(&replaced_prefs, 0, "'", "\"");
Nit: Seems like you wouldn't need this if you inited the string with " inside to
begin with.  See comment below about using raw string literals.

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:117:
return profile;
Nit: Simpler:

  if (profile)
    profile_manager->RegisterTestingProfile(profile, false, false);
  return profile;

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:131:
constexpr const char* default_search_provider_data =
Nit: I suspect raw string literals would make these declarations easier to read.

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:172:
// protection check fail.
Nit: It might be nice to avoid this hardcoding by getting the "default" default
search provider keyword at runtime.

If you don't do this, this comment has a typo and is slightly nongrammatical.

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:173:
const base::string16 google_keyword(base::ASCIIToUTF16("google.com"));
Nit: Prefer = to () to init "simple" objects like strings (see
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...
).

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:214:
DCHECK(!test_case.expected_keywords.empty());
Nit: Avoid [D]CHECK in test code, use ASSERT instead if necessary

[Alexander Yashkin, 2016-11-24 14:37:27]

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
File
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
(right):

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:29:
// Seed is empty string in tests.
On 2016/11/23 at 18:49:13, Peter Kasting wrote:
> Nit: I would just pass std::string() directly in the caller.

Done

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:36:
rlz_lib::GetMachineId(&device_id);
On 2016/11/23 at 18:49:13, Peter Kasting wrote:
> Why do we need to compute this?  Can't we just pass an empty string for both
the seed and the device ID?

Done

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:55:
};
On 2016/11/23 at 18:49:13, Peter Kasting wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN

Done

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:86:
DCHECK(dir_exists);
On 2016/11/23 at 18:49:13, Peter Kasting wrote:
> Nit: Avoid [D]CHECK in test code (failed checks cause tests to terminate
ungracefully and leave bots in a bad state).  Some ways to do this:
> 
> * Just omit the DCHECK
> * Return nullptr if !dir_exists (caller will assert)
> * EXPECT that this is true, then return nullptr
> * Change function to return profile via outparam, then ASSERT that dir_exists
here (ASSERT can only be used in a function that returns void)
> 
> (3 places)

Done

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:117:
return profile;
On 2016/11/23 at 18:49:13, Peter Kasting wrote:
> Nit: Simpler:
> 
>   if (profile)
>     profile_manager->RegisterTestingProfile(profile, false, false);
>   return profile;

Done

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:131:
constexpr const char* default_search_provider_data =
On 2016/11/23 at 18:49:13, Peter Kasting wrote:
> Nit: I suspect raw string literals would make these declarations easier to
read.

Could not find raw literals example in sources.
Tried to indent to be more readable.

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:172:
// protection check fail.
On 2016/11/23 at 18:49:13, Peter Kasting wrote:
> Nit: It might be nice to avoid this hardcoding by getting the "default"
default search provider keyword at runtime.
> 
> If you don't do this, this comment has a typo and is slightly nongrammatical.

Changed to get default provider from current browser test browser profile.

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:173:
const base::string16 google_keyword(base::ASCIIToUTF16("google.com"));
On 2016/11/23 at 18:49:13, Peter Kasting wrote:
> Nit: Prefer = to () to init "simple" objects like strings (see
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...
).

Done

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:214:
DCHECK(!test_case.expected_keywords.empty());
On 2016/11/23 at 18:49:13, Peter Kasting wrote:
> Nit: Avoid [D]CHECK in test code, use ASSERT instead if necessary

Done.

[Peter Kasting, 2016-11-24 19:39:48]

LGTM, would still make at least the raw literal change.

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
File
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
(right):

https://codereview.chromium.org/2521823007/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:131:
constexpr const char* default_search_provider_data =
On 2016/11/24 14:37:27, Alexander Yashkin wrote:
> On 2016/11/23 at 18:49:13, Peter Kasting wrote:
> > Nit: I suspect raw string literals would make these declarations easier to
> read.
> 
> Could not find raw literals example in sources.

See https://codereview.chromium.org/2470643002 for a sample conversion.

Also not sure why you converted constexpr const -> const, as the two don't
actually mean the same thing in this case (and arguably the const is the more
important of the two).

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
File
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
(right):

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:35:
DISALLOW_COPY_AND_ASSIGN(TURLServicePrefsProtectBrowsertest);
Nit: Frequently we put a blank line above this

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:39:
// in tests.
Nit: This comment kinda restates the code.  There's no reason we couldn't use
non-empty strings too, it's just that we don't need anything special.  I might
say something like "Passing empty strings to the |hash_calculator_| would be
insecure for real usage, but is sufficient in tests to verify the protection
functionality."

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:164:
// protection check fail.
Nit: This comment probably belongs above this whole block instead of in the
middle, since the code all seems related.  If not, then probably a blank line
above the comment would be good.

The comment has some grammar issues.  What about: "In cases below where the
protection check fails, the default search provider's keyword will be unchanged.
 Get that keyword from an unmodified profile."

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:166:
GetDefaultSearchProvider()->keyword();
Nit: Prefer to wrap after '=' where there's already whitespace instead of
breaking after "->".  (git cl format would probably have changed this)

[Alexander Yashkin, 2016-11-25 09:33:34]

Changed to raw literals, cool new feature, thanks.

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
File
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
(right):

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:35:
DISALLOW_COPY_AND_ASSIGN(TURLServicePrefsProtectBrowsertest);
On 2016/11/24 at 19:39:48, Peter Kasting wrote:
> Nit: Frequently we put a blank line above this

Done

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:39:
// in tests.
On 2016/11/24 at 19:39:48, Peter Kasting wrote:
> Nit: This comment kinda restates the code.  There's no reason we couldn't use
non-empty strings too, it's just that we don't need anything special.  I might
say something like "Passing empty strings to the |hash_calculator_| would be
insecure for real usage, but is sufficient in tests to verify the protection
functionality."

What I really meant is that protected prefs subsystem uses empty strings in
tests, 
so that we have to use same values if we want calculate correct hash.
Changed to your variant.

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:164:
// protection check fail.
On 2016/11/24 at 19:39:48, Peter Kasting wrote:
> Nit: This comment probably belongs above this whole block instead of in the
middle, since the code all seems related.  If not, then probably a blank line
above the comment would be good.
> 
> The comment has some grammar issues.  What about: "In cases below where the
protection check fails, the default search provider's keyword will be unchanged.
 Get that keyword from an unmodified profile."

Thanks, changed to your variant.

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:166:
GetDefaultSearchProvider()->keyword();
On 2016/11/24 at 19:39:48, Peter Kasting wrote:
> Nit: Prefer to wrap after '=' where there's already whitespace instead of
breaking after "->".  (git cl format would probably have changed this)

Fixed, thanks.

[Alexander Yashkin, 2016-11-25 09:34:41]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-11-25 09:34:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2521823007/#ps40001
(title: "Fixed after review, round 2")

[commit-bot: I haz the power, 2016-11-25 09:34:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-25 10:02:27]

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

[commit-bot: I haz the power, 2016-11-25 10:02:28]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Yashkin, 2016-11-25 10:55:10]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-11-25 10:55:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2521823007/#ps60001
(title: "Fixed compilation")

[commit-bot: I haz the power, 2016-11-25 10:55:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-25 12:05:22]

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

[commit-bot: I haz the power, 2016-11-25 12:05:23]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Yashkin, 2016-11-25 14:10:46]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-11-25 14:10:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2521823007/#ps80001
(title: "Fixed test under chromeOS")

[commit-bot: I haz the power, 2016-11-25 14:10:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-11-25 14:24:41]

Feels like this should be in pref_hash_browsertest.cc? Seems to test tracked
pref much more than anything under search_engines/. Plus the PrefHashBrowserTest
fixture already does a lot of the legwork done here I think. Or perhaps I'm not
seeing a major difference?

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
File
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
(right):

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:117:
const char* default_search_provider_data =
nit: char[] instead of char*
nit: prefer "static" for local const (on phone and don't have chromium-dev
reference but it avoids a copy on MSVC)
nit: I think constexpr char[] is even better than static const char[] these days

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:169:
const base::string16 my_search = base::ASCIIToUTF16("my_search");
L"my_search" instead of ASCIIToUTF16

and below

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:173:
{default_search_provider_data,
Add a comment above each test case mentioning what it's testing

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:208:
ASSERT_TRUE(dse) << "Test case i=" << i;
Can replace all of these logs by SCOPED_TRACE(i) at to of loop

[Alexander Yashkin, 2016-11-25 14:46:06]

On 2016/11/25 at 14:24:41, gab wrote:
> Feels like this should be in pref_hash_browsertest.cc? Seems to test tracked
pref much more than anything under search_engines/. Plus the PrefHashBrowserTest
fixture already does a lot of the legwork done here I think. Or perhaps I'm not
seeing a major difference?
I wanted to test that preferences that influence default search are protected.
Actually we added such test because we accidentally disabled protection of one
of these prefs and it went unnoticed for some time.
I thought that PrefHashBrowserTest tests more mechanics of protected prefs, than
their correct usage by components.

[Alexander Yashkin, 2016-11-25 14:46:30]

The CQ bit was unchecked by a-v-y@yandex-team.ru

[chromium-reviews, 2016-11-25 15:31:14]

Well this is integration testing chrome's protection policies (I.e. it's
testing chrome_pref_service_factory.cc). Nothing in search_engines/ should
really be involved here.

Le ven. 25 nov. 2016 09:46, <a-v-y@yandex-team.ru> a écrit :

> On 2016/11/25 at 14:24:41, gab wrote:
> > Feels like this should be in pref_hash_browsertest.cc? Seems to test
> tracked
> pref much more than anything under search_engines/. Plus the
> PrefHashBrowserTest
> fixture already does a lot of the legwork done here I think. Or perhaps
> I'm not
> seeing a major difference?
> I wanted to test that preferences that influence default search are
> protected.
> Actually we added such test because we accidentally disabled protection of
> one
> of these prefs and it went unnoticed for some time.
> I thought that PrefHashBrowserTest tests more mechanics of protected
> prefs, than
> their correct usage by components.
>
>
>
>
> https://codereview.chromium.org/2521823007/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexander Yashkin, 2016-11-25 15:55:19]

On 2016/11/25 at 15:31:14, chromium-reviews wrote:
> Well this is integration testing chrome's protection policies (I.e. it's
> testing chrome_pref_service_factory.cc). Nothing in search_engines/ should
> really be involved here.
> 
> Le ven. 25 nov. 2016 09:46, <a-v-y@yandex-team.ru> a écrit :
> 
> > On 2016/11/25 at 14:24:41, gab wrote:
> > > Feels like this should be in pref_hash_browsertest.cc? Seems to test
> > tracked
> > pref much more than anything under search_engines/. Plus the
> > PrefHashBrowserTest
> > fixture already does a lot of the legwork done here I think. Or perhaps
> > I'm not
> > seeing a major difference?
> > I wanted to test that preferences that influence default search are
> > protected.
> > Actually we added such test because we accidentally disabled protection of
> > one
> > of these prefs and it went unnoticed for some time.
> > I thought that PrefHashBrowserTest tests more mechanics of protected
> > prefs, than
> > their correct usage by components.
> >
> >
> >
> >
> > https://codereview.chromium.org/2521823007/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

Do you propose moving this test to pref_hash_browsertest.cc and converting it to
use PrefHashBrowserTest infrastructure?

[gab, 2016-11-25 16:26:54]

On 2016/11/25 15:55:19, Alexander Yashkin wrote:
> On 2016/11/25 at 15:31:14, chromium-reviews wrote:
> > Well this is integration testing chrome's protection policies (I.e. it's
> > testing chrome_pref_service_factory.cc). Nothing in search_engines/ should
> > really be involved here.
> > 
> > Le ven. 25 nov. 2016 09:46,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=a-v-y@yandex-team.ru> a
écrit :
> > 
> > > On 2016/11/25 at 14:24:41, gab wrote:
> > > > Feels like this should be in pref_hash_browsertest.cc? Seems to test
> > > tracked
> > > pref much more than anything under search_engines/. Plus the
> > > PrefHashBrowserTest
> > > fixture already does a lot of the legwork done here I think. Or perhaps
> > > I'm not
> > > seeing a major difference?
> > > I wanted to test that preferences that influence default search are
> > > protected.
> > > Actually we added such test because we accidentally disabled protection of
> > > one
> > > of these prefs and it went unnoticed for some time.
> > > I thought that PrefHashBrowserTest tests more mechanics of protected
> > > prefs, than
> > > their correct usage by components.
> > >
> > >
> > >
> > >
> > > https://codereview.chromium.org/2521823007/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email to
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
> > 
> 
> Do you propose moving this test to pref_hash_browsertest.cc and converting it
to
> use PrefHashBrowserTest infrastructure?

I think that'd make sense yes, this is how the other tracked prefs are tested.

I'd like to understand how you accidently disabled protection of those prefs
though? i.e. in which component was the bug introduced? I would have expected
[1] to already cover this.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/prefs/tracked/pref_hash_b...

[Peter Kasting, 2016-11-26 06:12:40]

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
File
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
(right):

https://codereview.chromium.org/2521823007/diff/20001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:39:
// in tests.
On 2016/11/25 09:33:34, Alexander Yashkin wrote:
> On 2016/11/24 at 19:39:48, Peter Kasting wrote:
> > Nit: This comment kinda restates the code.  There's no reason we couldn't
use
> non-empty strings too, it's just that we don't need anything special.  I might
> say something like "Passing empty strings to the |hash_calculator_| would be
> insecure for real usage, but is sufficient in tests to verify the protection
> functionality."
> 
> What I really meant is that protected prefs subsystem uses empty strings in
> tests, 
> so that we have to use same values if we want calculate correct hash.
> Changed to your variant.

Wait, where is that code that you're referring to?  If we have to match
something else's behavior, we need to say that more clearly here.  My comment
doesn't say that because I didn't know that was the case.

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
File
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
(right):

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:117:
const char* default_search_provider_data =
On 2016/11/25 14:24:41, gab wrote:
> nit: char[] instead of char*
> nit: prefer "static" for local const (on phone and don't have chromium-dev
> reference but it avoids a copy on MSVC)

Please find the reference; I've been explicitly asking people not to use static
for local consts in general because it can make generated code worse.

> nit: I think constexpr char[] is even better than static const char[] these
days

constexpr was the original method here, and I asked why it was removed, and
thought it would be put back.  Disappointed to see it wasn't, with no response
to my question.

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:169:
const base::string16 my_search = base::ASCIIToUTF16("my_search");
On 2016/11/25 14:24:41, gab wrote:
> L"my_search" instead of ASCIIToUTF16
> 
> and below

Doesn't work.  Can't assign a wide string to a string16 on non-Windows.

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:208:
ASSERT_TRUE(dse) << "Test case i=" << i;
On 2016/11/25 14:24:41, gab wrote:
> Can replace all of these logs by SCOPED_TRACE(i) at to of loop

Good call, I forgot about that.

[gab, 2016-11-28 16:12:20]

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
File
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
(right):

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:117:
const char* default_search_provider_data =
On 2016/11/26 06:12:40, Peter Kasting wrote:
> On 2016/11/25 14:24:41, gab wrote:
> > nit: char[] instead of char*
> > nit: prefer "static" for local const (on phone and don't have chromium-dev
> > reference but it avoids a copy on MSVC)
> 
> Please find the reference; I've been explicitly asking people not to use
static
> for local consts in general because it can make generated code worse.

According to our tests from 3 years ago, static was required for MSVC to
generate copy-less code:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/wrNhX...


> 
> > nit: I think constexpr char[] is even better than static const char[] these
> days
> 
> constexpr was the original method here, and I asked why it was removed, and
> thought it would be put back.  Disappointed to see it wasn't, with no response
> to my question.

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:169:
const base::string16 my_search = base::ASCIIToUTF16("my_search");
On 2016/11/26 06:12:40, Peter Kasting wrote:
> On 2016/11/25 14:24:41, gab wrote:
> > L"my_search" instead of ASCIIToUTF16
> > 
> > and below
> 
> Doesn't work.  Can't assign a wide string to a string16 on non-Windows.

Duh, right, that's unfortunate. 

Beyond this CL, but this makes me think that the string conversion methods
should perhaps be constexpr so that the compiler can do this work ahead of time
when possible?

Also, file_path.h provides the FILE_PATH_LITERAL macro to avoid exactly this,
maybe that macro needs to go beyond FilePaths?

[Peter Kasting, 2016-11-28 20:20:44]

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
File
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
(right):

https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:117:
const char* default_search_provider_data =
On 2016/11/28 16:12:20, gab wrote:
> On 2016/11/26 06:12:40, Peter Kasting wrote:
> > On 2016/11/25 14:24:41, gab wrote:
> > > nit: char[] instead of char*
> > > nit: prefer "static" for local const (on phone and don't have chromium-dev
> > > reference but it avoids a copy on MSVC)
> > 
> > Please find the reference; I've been explicitly asking people not to use
> static
> > for local consts in general because it can make generated code worse.
> 
> According to our tests from 3 years ago, static was required for MSVC to
> generate copy-less code:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/wrNhX...

Fascinating!  I forgot about this thread.  So, looks like this applies mainly to
compound types (which makes more sense), so technically, it's not really needed
if this stays as char* instead of being converted back to char[].

I was curious whether the story has changed with MSVC 2015 + constexpr.  I would
hope the two of those together eliminate the need for "static".  However, in
some local testing, it looks to me as if it has not, at least for the test code
in your email; no use of constexpr I could find made "char[]" as good as the
static version thereof.

[gab, 2016-11-28 20:24:25]

On 2016/11/28 20:20:44, Peter Kasting wrote:
>
https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
> File
>
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/2521823007/diff/60001/chrome/browser/search_e...
>
chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc:117:
> const char* default_search_provider_data =
> On 2016/11/28 16:12:20, gab wrote:
> > On 2016/11/26 06:12:40, Peter Kasting wrote:
> > > On 2016/11/25 14:24:41, gab wrote:
> > > > nit: char[] instead of char*
> > > > nit: prefer "static" for local const (on phone and don't have
chromium-dev
> > > > reference but it avoids a copy on MSVC)
> > > 
> > > Please find the reference; I've been explicitly asking people not to use
> > static
> > > for local consts in general because it can make generated code worse.
> > 
> > According to our tests from 3 years ago, static was required for MSVC to
> > generate copy-less code:
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/wrNhX...
> 
> Fascinating!  I forgot about this thread.  So, looks like this applies mainly
to
> compound types (which makes more sense), so technically, it's not really
needed
> if this stays as char* instead of being converted back to char[].

Right but I think we should prefer char[] to char* for strings as I also
described in that post.

> 
> I was curious whether the story has changed with MSVC 2015 + constexpr.  I
would
> hope the two of those together eliminate the need for "static".  However, in
> some local testing, it looks to me as if it has not, at least for the test
code
> in your email; no use of constexpr I could find made "char[]" as good as the
> static version thereof.

Oh really... wow! I had never tried but had assumed static was redundant for
constexpr..! Want to resurrect that thread to let everyone know?

[Peter Kasting, 2016-11-28 20:32:43]

On 2016/11/28 20:24:25, gab wrote:
> On 2016/11/28 20:20:44, Peter Kasting wrote:
> > I was curious whether the story has changed with MSVC 2015 + constexpr.  I
> would
> > hope the two of those together eliminate the need for "static".  However, in
> > some local testing, it looks to me as if it has not, at least for the test
> code
> > in your email; no use of constexpr I could find made "char[]" as good as the
> > static version thereof.
> 
> Oh really... wow! I had never tried but had assumed static was redundant for
> constexpr..! Want to resurrect that thread to let everyone know?

Already did :)

[Alexander Yashkin, 2016-12-07 13:56:37]

On 2016/11/25 at 16:26:54, gab wrote:
> On 2016/11/25 15:55:19, Alexander Yashkin wrote:
> > On 2016/11/25 at 15:31:14, chromium-reviews wrote:
> > > Well this is integration testing chrome's protection policies (I.e. it's
> > > testing chrome_pref_service_factory.cc). Nothing in search_engines/ should
> > > really be involved here.
> > > 
> > > Le ven. 25 nov. 2016 09:46,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=a-v-y@yandex-team.ru> a
écrit :
> > > 
> > > > On 2016/11/25 at 14:24:41, gab wrote:
> > > > > Feels like this should be in pref_hash_browsertest.cc? Seems to test
> > > > tracked
> > > > pref much more than anything under search_engines/. Plus the
> > > > PrefHashBrowserTest
> > > > fixture already does a lot of the legwork done here I think. Or perhaps
> > > > I'm not
> > > > seeing a major difference?
> > > > I wanted to test that preferences that influence default search are
> > > > protected.
> > > > Actually we added such test because we accidentally disabled protection
of
> > > > one
> > > > of these prefs and it went unnoticed for some time.
> > > > I thought that PrefHashBrowserTest tests more mechanics of protected
> > > > prefs, than
> > > > their correct usage by components.
> > > >
> > > >
> > > >
> > > >
> > > > https://codereview.chromium.org/2521823007/
> > > >
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email to
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
> > > 
> > 
> > Do you propose moving this test to pref_hash_browsertest.cc and converting
it to
> > use PrefHashBrowserTest infrastructure?
> 
> I think that'd make sense yes, this is how the other tracked prefs are tested.
> 
> I'd like to understand how you accidently disabled protection of those prefs
though? i.e. in which component was the bug introduced? I would have expected
[1] to already cover this.
> 
> [1]
https://cs.chromium.org/chromium/src/chrome/browser/prefs/tracked/pref_hash_b...

Ok, I moved test to pref_hash_browsertest.cc and rewritten it to use
PrefHashBrowserTest support.
It was much easier than in original CL.

PTAL, code changed a lot.

[Alexander Yashkin, 2016-12-07 14:20:33]

To the question of how we disabled protection - while turning off some prefs
protection in chrome_pref_service_factory.cc
we also disabled default search prefs protection and after we decided to return
to previous protected state,
not all of default search prefs were returned to protected state. 
Its more of human factor.
So we decided to write test that checks that all prefs that could change default
search.

Why test PrefHashBrowserTestUntrustedInitialized did not work - 
it uses SetUserSelectedDefaultSearchEngine function which stores user choice to 
"default_search_provider_data.template_url_data" pref only.
But while choosing default search DefaultSearchManager also can read from
"search_provider_overrides" and "default_search_provider"
and test checks only prefs written by it, but not read by it.

Thats why I placed this test to search_engines at first - it tests not how
protected prefs works, 
but what all prefs that influence default search are protected.

[gab, 2016-12-07 16:18:44]

Awesome! Glad this was a much simpler test to write. It makes sense to have a
test here that uses the DefaultSearchManager directly to ensure it reflects the
desired effect of our resets. This essentially verifies that the policy in
chrome_pref_service_factory.cc is effective (which is where the human failure
occured last time so it was definitely lacking coverage).

LGTM, thanks!

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1178: static constexpr
char default_search_provider_data[] =
No need for "static" keyword at file-scope

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1184: "keyword" :
"malwarekeyword",
Let's not use "malware" in our codebase, "bad" or "unknown" will work just as
well.

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1221: class
TestDefaultSearchProtected : public PrefHashBrowserTestBase {
All other fixtures in this file are prefixed with PrefHashBrowserTest, let's do
this for this one too (allows --gtest_filter=*PrefHashBrowserTest*)

[Peter Kasting, 2016-12-07 18:40:18]

LGTM

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1178: static constexpr
char default_search_provider_data[] =
On 2016/12/07 16:18:44, gab wrote:
> No need for "static" keyword at file-scope

Better yet, make these function-scope, since they're only used within a single
function.

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1179: R"({
Nit: I suggest formatting these like:

static constexpr char default_search_provider_data[] = R"(
{
  "default_search_provider_data" : {
    "template_url_data" : {
      "keyword" : "unknown",
      ...
    }
  }
})";
...
  "search_provider_overrides" : [
    {
      "keyword" : "unknown"
      ...
    }, {
      ...
    }
  ]
...

* Starting at the beginning of the line helps to reduce excessive indenting and
thus >80-column lines
* Placing the opening brace at the end of the previous line matches how you do
list formatting for search_provider_overrides and in general shortens things
without harming readability; this also fixes an erroneous one-space brace
indentation
* Modified indenting/wrapping of list items eliminates successive symbols at the
same indentation level (which is error-prone)

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1277: // Attack is
successfull.
Nit: Only one l

[Peter Kasting, 2016-12-07 18:41:34]

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1179: R"({
On 2016/12/07 18:40:18, Peter Kasting wrote:
> Nit: I suggest formatting these like:

(Note, read the above comment on the review tool, as email will strip the
leading whitespace)

[Alexander Yashkin, 2016-12-08 04:53:57]

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1178: static constexpr
char default_search_provider_data[] =
On 2016/12/07 at 18:40:18, Peter Kasting wrote:
> On 2016/12/07 16:18:44, gab wrote:
> > No need for "static" keyword at file-scope
> 
> Better yet, make these function-scope, since they're only used within a single
function.

Done

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1179: R"({
Tried to follow your guide, yet not sure if I succeed. :)
Maybe add it to chromium C++ code style? I saw a lot of embedded json in
chromium test code.

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1184: "keyword" :
"malwarekeyword",
On 2016/12/07 at 16:18:44, gab wrote:
> Let's not use "malware" in our codebase, "bad" or "unknown" will work just as
well.

replaced with "bad"

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1221: class
TestDefaultSearchProtected : public PrefHashBrowserTestBase {
On 2016/12/07 at 16:18:44, gab wrote:
> All other fixtures in this file are prefixed with PrefHashBrowserTest, let's
do this for this one too (allows --gtest_filter=*PrefHashBrowserTest*)

Done

https://codereview.chromium.org/2521823007/diff/100001/chrome/browser/prefs/t...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:1277: // Attack is
successfull.
On 2016/12/07 at 18:40:18, Peter Kasting wrote:
> Nit: Only one l

Successfully corrected :)

[Alexander Yashkin, 2016-12-08 04:54:16]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-12-08 04:54:16]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2521823007/#ps120001
(title: "Fixed after review, round 1")

[commit-bot: I haz the power, 2016-12-08 04:54:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 06:04:37]

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

[commit-bot: I haz the power, 2016-12-08 06:04:38]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Yashkin, 2016-12-08 06:23:48]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-12-08 06:23:49]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2521823007/#ps140001
(title: "Disabled test checks on chromeos")

[commit-bot: I haz the power, 2016-12-08 06:24:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 07:29:00]

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

[commit-bot: I haz the power, 2016-12-08 07:29:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Yashkin, 2016-12-08 07:56:22]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-12-08 07:56:23]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2521823007/#ps160001
(title: "Disabled test checks on chromeos")

[commit-bot: I haz the power, 2016-12-08 07:56:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 08:40:17]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481183782796530,
"parent_rev": "6a0bb72c50ad13d6ae620f90d1493e1067a43d8b", "commit_rev":
"81a563ce2e70620fb1c90e38bc99a55de9a2a071"}

[commit-bot: I haz the power, 2016-12-08 08:40:43]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-08 08:42:32]

Description was changed from

==========
Added browser test for TemplateUrlService protected prefs

Create browser test that checks protection of preferences that are used
to define default search provider. Currently where are tests that check
protected prefs mechanism but no tests for preferences that are used in
TemplateUrlService to store or override default search provider
settings.

BUG=668139

R=pkasting@chromium.org, gab@chromium.org
==========

to

==========
Added browser test for TemplateUrlService protected prefs

Create browser test that checks protection of preferences that are used
to define default search provider. Currently where are tests that check
protected prefs mechanism but no tests for preferences that are used in
TemplateUrlService to store or override default search provider
settings.

BUG=668139

R=pkasting@chromium.org, gab@chromium.org

Committed: https://crrev.com/38416868dc6ab9b4ae6b56ec4006d601c47704f0
Cr-Commit-Position: refs/heads/master@{#437211}
==========

[commit-bot: I haz the power, 2016-12-08 08:42:33]

Patchset 9 (id:??) landed as
https://crrev.com/38416868dc6ab9b4ae6b56ec4006d601c47704f0
Cr-Commit-Position: refs/heads/master@{#437211}