Add a preference to control Windows desktop search redirection.

Add a preference to control Windows desktop search redirection.

This CL adds a new preference called
"windows_desktop_search_redirection", that controls whether Windows
desktop searches should be redirected to the default search engine.

The preference can be enabled, disabled or unset. Enabled and disabled
mean that redirection of desktop searches has been explicitly enabled
or disabled. Unset means no value has been explicitly set and that the
browser should ask the user whether they want to use the feature
(prompt + checkbox to edit the preference from chrome://settings will
be implemented in an upcoming CL).

BUG=539963
Committed: https://crrev.com/ee511066919908c8b8b5d6f17bb18a2a72971372
Cr-Commit-Position: refs/heads/master@{#363064}

[fdoray, 2015-11-27 01:50:49]

fdoray@chromium.org changed reviewers:
+ pkasting@chromium.org

[fdoray, 2015-11-27 01:50:49]

Could you review this CL? Thanks.

With this upcoming CL: https://codereview.chromium.org/1477273002/, it will
become possible to control that preference from chrome://settings. I'm still
discussing how it will look with ainslie@.

[Peter Kasting, 2015-12-01 02:23:05]

LGTM

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
File components/search_engines/desktop_search_win.cc (right):

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
components/search_engines/desktop_search_win.cc:24: const base::Feature
kWindowsDesktopSearchRedirectionFeature = {
Nit: If you remove the .h declaration, you can declare this in
WindowsDesktopSearchRedirectionFeatureIsEnabled().

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
components/search_engines/desktop_search_win.cc:48: return
WindowsDesktopSearchRedirectionPref::DISABLED;
Nit: Simpler:

  return pref_value ? WindowsDesktopSearchRedirectionPref::ENABLED
                    : WindowsDesktopSearchRedirectionPref::DISABLED;

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
File components/search_engines/desktop_search_win.h (right):

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
components/search_engines/desktop_search_win.h:32: extern const base::Feature
kWindowsDesktopSearchRedirectionFeature;
There's no need to declare this in the header as only the .cc file uses it.

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
File components/search_engines/desktop_search_win_unittest.cc (right):

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
components/search_engines/desktop_search_win_unittest.cc:21:
TEST(WindowsDesktopSearch, Prefs) {
This test seems pretty useless.  You're basically testing SetBoolean(), which is
plenty well tested elsewhere.  I'd remove this.

If you're going to test this, you'd want to test the code in
startup_browser_creator.cc that actually parses the input URLs and ensure it
respects these prefs appropriately.  (Testing that code is good anyway if it's
not already happening.)

This raises a question: should that code be responsible for checking the feature
and prefs here?  Or should those checks be internalized to
DetectWindowsDesktopSearch() itself?

[fdoray, 2015-12-02 17:22:42]

Description was changed from

==========
Add a preference to control Windows desktop search redirection.

This CL adds a new preference called
"windows_desktop_search_redirection", that controls whether Windows
desktop searches should be redirected to the default search engine.

The preference can be enabled, disabled or unset. Enabled and disabled
mean that redirection of desktop searches has been explicitly enabled
or disabled. Unset means no value has been explicitly set and that the
browser should ask the user whether they want to use the feature.

An upcoming CL will allow this preference to be edited in
chrome://settings.

BUG=539963
==========

to

==========
Add a preference to control Windows desktop search redirection.

This CL adds a new preference called
"windows_desktop_search_redirection", that controls whether Windows
desktop searches should be redirected to the default search engine.

The preference can be enabled, disabled or unset. Enabled and disabled
mean that redirection of desktop searches has been explicitly enabled
or disabled. Unset means no value has been explicitly set and that the
browser should ask the user whether they want to use the feature
(prompt will be implemented in an upcoming CL).

An upcoming CL will allow this preference to be edited in
chrome://settings.

BUG=539963
==========

[fdoray, 2015-12-02 17:23:17]

Description was changed from

==========
Add a preference to control Windows desktop search redirection.

This CL adds a new preference called
"windows_desktop_search_redirection", that controls whether Windows
desktop searches should be redirected to the default search engine.

The preference can be enabled, disabled or unset. Enabled and disabled
mean that redirection of desktop searches has been explicitly enabled
or disabled. Unset means no value has been explicitly set and that the
browser should ask the user whether they want to use the feature
(prompt will be implemented in an upcoming CL).

An upcoming CL will allow this preference to be edited in
chrome://settings.

BUG=539963
==========

to

==========
Add a preference to control Windows desktop search redirection.

This CL adds a new preference called
"windows_desktop_search_redirection", that controls whether Windows
desktop searches should be redirected to the default search engine.

The preference can be enabled, disabled or unset. Enabled and disabled
mean that redirection of desktop searches has been explicitly enabled
or disabled. Unset means no value has been explicitly set and that the
browser should ask the user whether they want to use the feature
(prompt + checkbox to edit the preference from chrome://settings will
be implemented in an upcoming CL).

BUG=539963
==========

[fdoray, 2015-12-02 17:28:26]

fdoray@chromium.org changed reviewers:
+ asvitkine@chromium.org

[fdoray, 2015-12-02 17:28:27]

pkasting@: I added a test for StartupBrowserCreator::GetURLsFromCommandLine, as
you recommended. Can you review it in startup_browser_creator_win_unittest.cc?

asvitkine@: Can you review about_flags.cc, generated_resources.grd and usage of
base::Feature throughout the CL?

Thanks!

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
File components/search_engines/desktop_search_win.cc (right):

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
components/search_engines/desktop_search_win.cc:24: const base::Feature
kWindowsDesktopSearchRedirectionFeature = {
On 2015/12/01 02:23:05, Peter Kasting wrote:
> Nit: If you remove the .h declaration, you can declare this in
> WindowsDesktopSearchRedirectionFeatureIsEnabled().

See other comment. It needs to be declared in the header file so that it can be
referenced from about_flags.cc

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
components/search_engines/desktop_search_win.cc:48: return
WindowsDesktopSearchRedirectionPref::DISABLED;
On 2015/12/01 02:23:05, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return pref_value ? WindowsDesktopSearchRedirectionPref::ENABLED
>                     : WindowsDesktopSearchRedirectionPref::DISABLED;

This code is gone now.

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
File components/search_engines/desktop_search_win.h (right):

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
components/search_engines/desktop_search_win.h:32: extern const base::Feature
kWindowsDesktopSearchRedirectionFeature;
On 2015/12/01 02:23:05, Peter Kasting wrote:
> There's no need to declare this in the header as only the .cc file uses it.

It's required if I want the feature to be controlled by about:flags. I've
included in this CL the about_flags.cc code that uses this.

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
File components/search_engines/desktop_search_win_unittest.cc (right):

https://codereview.chromium.org/1477783004/diff/40001/components/search_engin...
components/search_engines/desktop_search_win_unittest.cc:21:
TEST(WindowsDesktopSearch, Prefs) {
On 2015/12/01 02:23:05, Peter Kasting wrote:
> This test seems pretty useless.  You're basically testing SetBoolean(), which
is
> plenty well tested elsewhere.  I'd remove this.
> 
> If you're going to test this, you'd want to test the code in
> startup_browser_creator.cc that actually parses the input URLs and ensure it
> respects these prefs appropriately.  (Testing that code is good anyway if it's
> not already happening.)
> 
> This raises a question: should that code be responsible for checking the
feature
> and prefs here?  Or should those checks be internalized to
> DetectWindowsDesktopSearch() itself?

I've replaced GetWindowsDesktopSearchRedirectionPref with a function that checks
if both the feature and the pref are enabled. That way, there is less logic in
startup_browser_creator.cc.

Internalizing checks to DetectWindowsSearch() doesn't work, because in an
upcoming CL, I'll need to be able to detect Windows desktop searches even when
the feature is disabled, in order to log a UMA metric.

[Peter Kasting, 2015-12-03 05:41:39]

LGTM

https://codereview.chromium.org/1477783004/diff/100001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1477783004/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags.cc:97: #endif  // defined(OS_WIN)
Nit: I suggest grouping together OS_, USE_, and ENABLE_ groups of ifdefs; right
now the order is arbitrary.

https://codereview.chromium.org/1477783004/diff/100001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator_win_unittest.cc (right):

https://codereview.chromium.org/1477783004/diff/100001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_win_unittest.cc:22: class
StartupBrowserCreatorWinUnitTest : public testing::Test {
Nit: I would probably remove "Unit" from this name; I think it's less common to
use that within the names of test classes.

https://codereview.chromium.org/1477783004/diff/100001/components/search_engi...
File components/search_engines/desktop_search_win.cc (right):

https://codereview.chromium.org/1477783004/diff/100001/components/search_engi...
components/search_engines/desktop_search_win.cc:21:
"windows_desktop_search_redirection";
Multiple test files also want to use this pref name.  I suggest declaring it
publicly so the tests can just use this string constant directly and not risk
getting out of sync.

https://codereview.chromium.org/1477783004/diff/100001/components/search_engi...
components/search_engines/desktop_search_win.cc:25:
"WindowsDesktopSearchRedirection", base::FEATURE_DISABLED_BY_DEFAULT};
Nit: I'd wrap the final "};" to the next line.

https://codereview.chromium.org/1477783004/diff/100001/components/search_engi...
components/search_engines/desktop_search_win.cc:38: return
pref_service->GetBoolean(kWindowsDesktopSearchRedirectionPref);
Nit: Simpler:

  return base::FeatureList::IsEnabled(
             kWindowsDesktopSearchRedirectionFeature) &&
         pref_service->GetBoolean(kWindowsDesktopSearchRedirectionPref);

[fdoray, 2015-12-03 17:16:18]

pkasting@: all done.

asvitkine@: Can you review about_flags.cc, generated_resources.grd and usage of
base::Feature throughout the CL?

https://codereview.chromium.org/1477783004/diff/100001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1477783004/diff/100001/chrome/browser/about_f...
chrome/browser/about_flags.cc:97: #endif  // defined(OS_WIN)
On 2015/12/03 05:41:39, Peter Kasting wrote:
> Nit: I suggest grouping together OS_, USE_, and ENABLE_ groups of ifdefs;
right
> now the order is arbitrary.

Done.

https://codereview.chromium.org/1477783004/diff/100001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator_win_unittest.cc (right):

https://codereview.chromium.org/1477783004/diff/100001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_win_unittest.cc:22: class
StartupBrowserCreatorWinUnitTest : public testing::Test {
On 2015/12/03 05:41:39, Peter Kasting wrote:
> Nit: I would probably remove "Unit" from this name; I think it's less common
to
> use that within the names of test classes.

Done.

https://codereview.chromium.org/1477783004/diff/100001/components/search_engi...
File components/search_engines/desktop_search_win.cc (right):

https://codereview.chromium.org/1477783004/diff/100001/components/search_engi...
components/search_engines/desktop_search_win.cc:21:
"windows_desktop_search_redirection";
On 2015/12/03 05:41:39, Peter Kasting wrote:
> Multiple test files also want to use this pref name.  I suggest declaring it
> publicly so the tests can just use this string constant directly and not risk
> getting out of sync.

Done.

https://codereview.chromium.org/1477783004/diff/100001/components/search_engi...
components/search_engines/desktop_search_win.cc:25:
"WindowsDesktopSearchRedirection", base::FEATURE_DISABLED_BY_DEFAULT};
On 2015/12/03 05:41:39, Peter Kasting wrote:
> Nit: I'd wrap the final "};" to the next line.

Done.

https://codereview.chromium.org/1477783004/diff/100001/components/search_engi...
components/search_engines/desktop_search_win.cc:38: return
pref_service->GetBoolean(kWindowsDesktopSearchRedirectionPref);
On 2015/12/03 05:41:39, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return base::FeatureList::IsEnabled(
>              kWindowsDesktopSearchRedirectionFeature) &&
>          pref_service->GetBoolean(kWindowsDesktopSearchRedirectionPref);

Done.

[Alexei Svitkine (slow), 2015-12-03 18:25:24]

lgtm

[fdoray, 2015-12-03 18:26:49]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2015-12-03 18:26:50]

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/1477783004/#ps120001
(title: "nits from pkasting")

[commit-bot: I haz the power, 2015-12-03 18:28:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477783004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477783004/120001

[commit-bot: I haz the power, 2015-12-03 18:35:20]

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

[commit-bot: I haz the power, 2015-12-03 18:35:21]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[fdoray, 2015-12-03 18:57:31]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2015-12-03 18:57:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1477783004/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-03 18:59:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477783004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477783004/140001

[commit-bot: I haz the power, 2015-12-03 19:21:58]

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

[commit-bot: I haz the power, 2015-12-03 19:21:59]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[fdoray, 2015-12-03 20:08:57]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2015-12-03 20:08:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1477783004/#ps160001
(title: "fix compile error.")

[commit-bot: I haz the power, 2015-12-03 20:09:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477783004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477783004/160001

[commit-bot: I haz the power, 2015-12-03 21:37:19]

Description was changed from

==========
Add a preference to control Windows desktop search redirection.

This CL adds a new preference called
"windows_desktop_search_redirection", that controls whether Windows
desktop searches should be redirected to the default search engine.

The preference can be enabled, disabled or unset. Enabled and disabled
mean that redirection of desktop searches has been explicitly enabled
or disabled. Unset means no value has been explicitly set and that the
browser should ask the user whether they want to use the feature
(prompt + checkbox to edit the preference from chrome://settings will
be implemented in an upcoming CL).

BUG=539963
==========

to

==========
Add a preference to control Windows desktop search redirection.

This CL adds a new preference called
"windows_desktop_search_redirection", that controls whether Windows
desktop searches should be redirected to the default search engine.

The preference can be enabled, disabled or unset. Enabled and disabled
mean that redirection of desktop searches has been explicitly enabled
or disabled. Unset means no value has been explicitly set and that the
browser should ask the user whether they want to use the feature
(prompt + checkbox to edit the preference from chrome://settings will
be implemented in an upcoming CL).

BUG=539963
==========

[commit-bot: I haz the power, 2015-12-03 21:37:20]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-03 21:38:12]

Description was changed from

==========
Add a preference to control Windows desktop search redirection.

This CL adds a new preference called
"windows_desktop_search_redirection", that controls whether Windows
desktop searches should be redirected to the default search engine.

The preference can be enabled, disabled or unset. Enabled and disabled
mean that redirection of desktop searches has been explicitly enabled
or disabled. Unset means no value has been explicitly set and that the
browser should ask the user whether they want to use the feature
(prompt + checkbox to edit the preference from chrome://settings will
be implemented in an upcoming CL).

BUG=539963
==========

to

==========
Add a preference to control Windows desktop search redirection.

This CL adds a new preference called
"windows_desktop_search_redirection", that controls whether Windows
desktop searches should be redirected to the default search engine.

The preference can be enabled, disabled or unset. Enabled and disabled
mean that redirection of desktop searches has been explicitly enabled
or disabled. Unset means no value has been explicitly set and that the
browser should ask the user whether they want to use the feature
(prompt + checkbox to edit the preference from chrome://settings will
be implemented in an upcoming CL).

BUG=539963

Committed: https://crrev.com/ee511066919908c8b8b5d6f17bb18a2a72971372
Cr-Commit-Position: refs/heads/master@{#363064}
==========

[commit-bot: I haz the power, 2015-12-03 21:38:13]

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