Implement the Windows desktop search redirection feature.

Implement the Windows desktop search redirection feature.

With this CL, Windows desktop searches are redirected to the default
search engine when the DesktopSearchRedirection feature is enabled.
The first time that a desktop search is redirected, an infobar is
shown to inform the user of the redirection.

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

[fdoray, 2016-01-16 18:07:34]

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

[fdoray, 2016-01-16 18:07:35]

pkasting@: Can you review this CL? Thanks!
asvitkine@: Can you review tools/metrics/*?

Note: The UI was proposed by ainslie@ and has been accepted by UI-review.

[fdoray, 2016-01-16 18:09:09]

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

[fdoray, 2016-01-16 18:09:10]

+asvitkine@

pkasting@: Can you review this CL? Thanks!
asvitkine@: Can you review tools/metrics/*?

Note: The UI was proposed by ainslie@ and has been accepted by UI-review.

[Alexei Svitkine (slow), 2016-01-18 16:08:52]

https://codereview.chromium.org/1598553003/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/1598553003/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator.cc:407: DCHECK(browser);
What guarantees this is true? I see an |in_synchronous_profile_launch_| variable
above - which implies the browser launch could be async - in that case, wouldn't
it possible the browser hasn't been created yet?

Also, it seems strange to get this via GetLastActive() if you're launching it
just above. Can lwp be modified to give us the Browser* it launched?

https://codereview.chromium.org/1598553003/diff/1/components/search_engines/d...
File components/search_engines/desktop_search_infobar_delegate_win.cc (right):

https://codereview.chromium.org/1598553003/diff/1/components/search_engines/d...
components/search_engines/desktop_search_infobar_delegate_win.cc:36: }  //
namespace
Nit: Add empty lines within it.

https://codereview.chromium.org/1598553003/diff/1/components/search_engines/d...
File components/search_engines/desktop_search_utils_win.cc (right):

https://codereview.chromium.org/1598553003/diff/1/components/search_engines/d...
components/search_engines/desktop_search_utils_win.cc:27: const base::Feature
kWindowsDesktopSearchRedirectionFeature = {
Nit: No =

https://codereview.chromium.org/1598553003/diff/1/tools/metrics/actions/actio...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1598553003/diff/1/tools/metrics/actions/actio...
tools/metrics/actions/actions.xml:13658: <action
name="WindowsDesktopSearchRedirectionInfoBar_Dismiss">
It might be good to add an action that the Infobar was shown.

[fdoray, 2016-01-18 18:51:45]

asvitkine@: Can you take another look?
pkasting@: Can you review this CL?

https://codereview.chromium.org/1598553003/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/1598553003/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator.cc:407: DCHECK(browser);
On 2016/01/18 16:08:51, asvitkine (OOO until Jan18) wrote:
> What guarantees this is true? I see an |in_synchronous_profile_launch_|
variable
> above - which implies the browser launch could be async - in that case,
wouldn't
> it possible the browser hasn't been created yet?
> 
> Also, it seems strange to get this via GetLastActive() if you're launching it
> just above. Can lwp be modified to give us the Browser* it launched?

I changed the code to handle cases where |browser| is nullptr (can happen with
--no-startup-window).

Note that if a browser has to be created, it will have been created at this
point. |in_synchronous_profile_launch_| means: are we launching a new chrome.exe
process? vs. did we run chrome.exe when a chrome.exe process was already
running?

|lwp| itself uses GetLastActive() [1] Can I rely on this as well? Or should I
change StartupBrowserCreatorImpl::Launch to return the Browser* returned by
StartupBrowserCreatorImpl::OpenTabsInBrowser?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/1598553003/diff/1/components/search_engines/d...
File components/search_engines/desktop_search_infobar_delegate_win.cc (right):

https://codereview.chromium.org/1598553003/diff/1/components/search_engines/d...
components/search_engines/desktop_search_infobar_delegate_win.cc:36: }  //
namespace
On 2016/01/18 16:08:51, asvitkine (OOO until Jan18) wrote:
> Nit: Add empty lines within it.

Done.

https://codereview.chromium.org/1598553003/diff/1/components/search_engines/d...
File components/search_engines/desktop_search_utils_win.cc (right):

https://codereview.chromium.org/1598553003/diff/1/components/search_engines/d...
components/search_engines/desktop_search_utils_win.cc:27: const base::Feature
kWindowsDesktopSearchRedirectionFeature = {
On 2016/01/18 16:08:52, asvitkine (OOO until Jan18) wrote:
> Nit: No =

Done.

https://codereview.chromium.org/1598553003/diff/1/tools/metrics/actions/actio...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1598553003/diff/1/tools/metrics/actions/actio...
tools/metrics/actions/actions.xml:13658: <action
name="WindowsDesktopSearchRedirectionInfoBar_Dismiss">
On 2016/01/18 16:08:52, asvitkine (OOO until Jan18) wrote:
> It might be good to add an action that the Infobar was shown.

Done.

[Alexei Svitkine (slow), 2016-01-19 19:55:11]

metrics lgtm - I only did a minimal review of the rest of the CL, so deferring
to Peter for that.

[Peter Kasting, 2016-01-20 03:03:02]

https://codereview.chromium.org/1598553003/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1598553003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5961: +          Enable the experimental
implementation of the "Cache-Control: stale-while-revalidate" directive. This
permits servers to specify that some resources may be revalidated in the
background to improve latency.
What changed on this line?

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:406:
BrowserList::GetInstance(host_desktop_type)->GetLastActive();
We ultimately only need this in order to get the active WebContents.  Is there a
more direct way of getting this, e.g. to have the launching code directly tell
us what WebContents it opened?

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:407: if (browser) {
When can this be null?

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:590: // Ignore cases where
there is multiple command line arguments, because
Nit: is -> are

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator.h (right):

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.h:148: bool*
should_show_desktop_search_infobar);
Nit: I might name this |show_search_redirection_infobar|, since "redirection"
seems like the critical bit of info the infobar is communicating.  (Similar
comment applies to temp names in the .cc file, the name of the infobar delegate
class, etc.)

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_infobar_delegate_win.cc (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.cc:52:
prefs::kWindowsDesktopSearchRedirectionInfobarShownPref, true);
What if the user navigates quickly and never really sees the infobar?  I wonder
if we shouldn't stop showing the infobar until the user has explicitly taken an
action with it at least once.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.cc:74:
std::vector<base::string16>(1, default_search_engine_name_), nullptr);
Why not just call

  return l10n_util::GetStringFUTF16(
      IDS_WINDOWS_DESKTOP_SEARCH_REDIRECTION_INFOBAR_MESSAGE,
      default_search_engine_name_);

?

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.cc:92:
infobar()->owner()->OpenURL(search_settings_url_, NEW_FOREGROUND_TAB);
If we already have a settings page open, will this reuse that tab, or open a
second one?  The latter would be a bug, since we always want to reuse settings
tabs.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.cc:94: // Do not
close the infobar.
Why?

Also, doesn't this mean we'll always record a "dismissed" or "ignored" tick as
well?  "ignored" in particular seems inaccurate in this case.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_infobar_delegate_win.h (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.h:20: // The
delegate for the infobar informing the user that a Windows desktop search
Nit: I'd probably just say "Informs the user that..."

Also, I probably wouldn't refer to Windows anywhere in the comments, class name,
file name, string identifier names, etc.  While callers might only use this on
Windows, there's no reason this infobar is restricted to Windows in principle. 
Basically, I would only refer to "Windows" in code that's actually
Windows-specific, i.e. has #ifdef (OS_WIN) or the like.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.h:28: // records
in |pref_service| that the infobar has been shown.
Nit: Simpler, and more parallel to other infobars' comments:

Creates a search redirection infobar and delegate and adds the infobar to
|infobar_manager|.  Records in |pref_service| that such an infobar was shown.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.h:43: //
ConfirmInfoBarDelegate:
It seems like we really ought to have a Learn More link where we explain about
desktop search redirection and how to disable it if necessary?

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.h:49: //
infobars::InfoBarDelegate:
You don't directly subclass this class, so these overrides should be with the
ConfirmInfoBarDelegate overrides, and listed first in that group.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_infobar_delegate_win_unittest.cc
(right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win_unittest.cc:33:
const base::char16 kSearchSettingsURL[] = L"chrome://settings/searchEngines";
I wouldn't use the real chrome: URL in a components/ test file.  Just use a
bogus URL for testing.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win_unittest.cc:37:
TEST(WindowsDesktopSearchInfobarDelegateTest, ManageSearchSettings) {
Does this test really matter?  It seems like it's not worth much to just check
that calling Accept() on the infobar causes it to call OpenURL().  I mean, I
guess it's not wrong, just... unlikely to actually catch a real bug ever?

It seems like testing that the prefs and histograms are updated as desired would
be a better thing to test, if we wanted to have tests at all.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win_unittest.cc:40:
new infobars::InfoBar(scoped_ptr<infobars::InfoBarDelegate>(
Nit: make_scoped_ptr()?

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_utils_win.cc (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win.cc:58: 
Nit: I'd remove this blank line and the one on line 68.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win.cc:79: search_terms->clear();
This should have been done atop the function to handle early return cases.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win.cc:142:
"WindowsDesktopSearchRedirectionInfoBar_Shown"));
It seems like it would make more sense to record this in the infobar delegate
code than here.

Also, why do we need both this and
RecordWindowsDesktopSearchURLAction(REDIRECTION_INFOBAR_SHOWN)?

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_utils_win.h (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win.h:43: bool*
should_show_infobar);
Why doesn't this return a bool instead of taking it as an outparam?  Seems like
that would be simpler, and would simplify the test code slightly too.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_utils_win_unittest.cc (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win_unittest.cc:82:
{L"https://www.google.com", nullptr},
Nit: I think this would be clearer if we just had two char*s, one for the
expected input and one for the expected output.  The caller could convert the
input to a string16 as necessary, and call .spec() to convert GURLs back to
strings for comparison.  In cases where you use nullptr for the output, we'd
just duplicate the input string, eliminating the need for the ternary in the
loop below.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win_unittest.cc:137: const GURL
expected_return_value(
This doesn't seem to be used.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines_strings.grdp (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines_strings.grdp:9: Manage search settings
Why is going to the settings useful?  Is there going to be a checkbox there to
disable search redirection?  If not it seems somewhat unrelated to what the
infobar is actually doing, and liable to just frustrate users who wanted a way
to turn off this functionality.

[fdoray, 2016-01-21 19:31:55]

Description was changed from

==========
Implement the Windows desktop search redirection feature.

With this CL, Windows desktop searches are redirected to the default
search engine when the WindowsDesktopSearchRedirection feature is
enabled. The first time that a desktop search is redirected, an
infobar is shown to inform the user of the redirection.

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

to

==========
Implement the Windows desktop search redirection feature.

With this CL, Windows desktop searches are redirected to the default
search engine when the DesktopSearchRedirection feature is enabled.
The first time that a desktop search is redirected, an infobar is
shown to inform the user of the redirection.

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

[fdoray, 2016-01-21 19:32:10]

fdoray@chromium.org changed reviewers:
+ ainslie@chromium.org, caitkp@chromium.org

[fdoray, 2016-01-21 21:02:34]

pkasting@: Can you take another look?

caitkp@: Can you review components/components_strings.grd,
components/components_tests.gyp, components/search_engines_strings.grdp?

ainslie@: Can you reply to the 2 comments in which I mentioned you?

https://codereview.chromium.org/1598553003/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1598553003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5961: +          Enable the experimental
implementation of the "Cache-Control: stale-while-revalidate" directive. This
permits servers to specify that some resources may be revalidated in the
background to improve latency.
On 2016/01/20 03:03:01, Peter Kasting wrote:
> What changed on this line?

A white space was removed by my editor at the end of the line. I put it back to
avoid polluting the diff.

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:406:
BrowserList::GetInstance(host_desktop_type)->GetLastActive();
On 2016/01/20 03:03:01, Peter Kasting wrote:
> We ultimately only need this in order to get the active WebContents.  Is there
a
> more direct way of getting this, e.g. to have the launching code directly tell
> us what WebContents it opened?

Done. I moved this code to StartupBrowserCreatorImpl::AddInfoBarsIfNecessary.
This method receives a Browser*, so I no longer need
BrowserList::GetInstance()->GetLastActive();

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:407: if (browser) {
On 2016/01/20 03:03:01, Peter Kasting wrote:
> When can this be null?

chrome.exe --no-startup-window

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:590: // Ignore cases where
there is multiple command line arguments, because
On 2016/01/20 03:03:01, Peter Kasting wrote:
> Nit: is -> are

Done.

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator.h (right):

https://codereview.chromium.org/1598553003/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.h:148: bool*
should_show_desktop_search_infobar);
On 2016/01/20 03:03:01, Peter Kasting wrote:
> Nit: I might name this |show_search_redirection_infobar|, since "redirection"
> seems like the critical bit of info the infobar is communicating.  (Similar
> comment applies to temp names in the .cc file, the name of the infobar
delegate
> class, etc.)

Done.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_infobar_delegate_win.cc (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.cc:52:
prefs::kWindowsDesktopSearchRedirectionInfobarShownPref, true);
On 2016/01/20 03:03:01, Peter Kasting wrote:
> What if the user navigates quickly and never really sees the infobar?  I
wonder
> if we shouldn't stop showing the infobar until the user has explicitly taken
an
> action with it at least once.

ainslie@: What do you think? Should we show the infobar until the user has
clicked either the "Manage search settings" or the X button?

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.cc:74:
std::vector<base::string16>(1, default_search_engine_name_), nullptr);
On 2016/01/20 03:03:01, Peter Kasting wrote:
> Why not just call
> 
>   return l10n_util::GetStringFUTF16(
>       IDS_WINDOWS_DESKTOP_SEARCH_REDIRECTION_INFOBAR_MESSAGE,
>       default_search_engine_name_);
> 
> ?

Done.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.cc:92:
infobar()->owner()->OpenURL(search_settings_url_, NEW_FOREGROUND_TAB);
On 2016/01/20 03:03:01, Peter Kasting wrote:
> If we already have a settings page open, will this reuse that tab, or open a
> second one?  The latter would be a bug, since we always want to reuse settings
> tabs.

Done.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.cc:94: // Do not
close the infobar.
On 2016/01/20 03:03:01, Peter Kasting wrote:
> Why?
> 
> Also, doesn't this mean we'll always record a "dismissed" or "ignored" tick as
> well?  "ignored" in particular seems inaccurate in this case.

Done. Once the user has interacted with the infobar, I agree that it is useless
to keep it visible.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_infobar_delegate_win.h (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.h:20: // The
delegate for the infobar informing the user that a Windows desktop search
On 2016/01/20 03:03:01, Peter Kasting wrote:
> Nit: I'd probably just say "Informs the user that..."
> 
> Also, I probably wouldn't refer to Windows anywhere in the comments, class
name,
> file name, string identifier names, etc.  While callers might only use this on
> Windows, there's no reason this infobar is restricted to Windows in principle.

> Basically, I would only refer to "Windows" in code that's actually
> Windows-specific, i.e. has #ifdef (OS_WIN) or the like.

Done.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.h:28: // records
in |pref_service| that the infobar has been shown.
On 2016/01/20 03:03:01, Peter Kasting wrote:
> Nit: Simpler, and more parallel to other infobars' comments:
> 
> Creates a search redirection infobar and delegate and adds the infobar to
> |infobar_manager|.  Records in |pref_service| that such an infobar was shown.

Done.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.h:43: //
ConfirmInfoBarDelegate:
On 2016/01/20 03:03:01, Peter Kasting wrote:
> It seems like we really ought to have a Learn More link where we explain about
> desktop search redirection and how to disable it if necessary?

UI review decided not to have an option to disable desktop search redirection.
It is still possible to "disable" it by setting Bing as the default search
engine.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.h:49: //
infobars::InfoBarDelegate:
On 2016/01/20 03:03:01, Peter Kasting wrote:
> You don't directly subclass this class, so these overrides should be with the
> ConfirmInfoBarDelegate overrides, and listed first in that group.

Done.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_infobar_delegate_win_unittest.cc
(right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win_unittest.cc:33:
const base::char16 kSearchSettingsURL[] = L"chrome://settings/searchEngines";
On 2016/01/20 03:03:02, Peter Kasting wrote:
> I wouldn't use the real chrome: URL in a components/ test file.  Just use a
> bogus URL for testing.

Removed the test.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win_unittest.cc:37:
TEST(WindowsDesktopSearchInfobarDelegateTest, ManageSearchSettings) {
On 2016/01/20 03:03:02, Peter Kasting wrote:
> Does this test really matter?  It seems like it's not worth much to just check
> that calling Accept() on the infobar causes it to call OpenURL().  I mean, I
> guess it's not wrong, just... unlikely to actually catch a real bug ever?
> 
> It seems like testing that the prefs and histograms are updated as desired
would
> be a better thing to test, if we wanted to have tests at all.

Removed the test.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win_unittest.cc:40:
new infobars::InfoBar(scoped_ptr<infobars::InfoBarDelegate>(
On 2016/01/20 03:03:02, Peter Kasting wrote:
> Nit: make_scoped_ptr()?

Removed the test.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_utils_win.cc (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win.cc:58: 
On 2016/01/20 03:03:02, Peter Kasting wrote:
> Nit: I'd remove this blank line and the one on line 68.

Done.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win.cc:79: search_terms->clear();
On 2016/01/20 03:03:02, Peter Kasting wrote:
> This should have been done atop the function to handle early return cases.

Done.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win.cc:142:
"WindowsDesktopSearchRedirectionInfoBar_Shown"));
On 2016/01/20 03:03:02, Peter Kasting wrote:
> It seems like it would make more sense to record this in the infobar delegate
> code than here.
> 
> Also, why do we need both this and
> RecordWindowsDesktopSearchURLAction(REDIRECTION_INFOBAR_SHOWN)?

Done. I changed the code to record that the infobar is shown in the infobar
delegate.

rkaplow@ recommended mean to use both histograms and actions. asvitkine@:
Thoughts?

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_utils_win.h (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win.h:43: bool*
should_show_infobar);
On 2016/01/20 03:03:02, Peter Kasting wrote:
> Why doesn't this return a bool instead of taking it as an outparam?  Seems
like
> that would be simpler, and would simplify the test code slightly too.

Done.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_utils_win_unittest.cc (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win_unittest.cc:82:
{L"https://www.google.com", nullptr},
On 2016/01/20 03:03:02, Peter Kasting wrote:
> Nit: I think this would be clearer if we just had two char*s, one for the
> expected input and one for the expected output.  The caller could convert the
> input to a string16 as necessary, and call .spec() to convert GURLs back to
> strings for comparison.  In cases where you use nullptr for the output, we'd
> just duplicate the input string, eliminating the need for the ternary in the
> loop below.

Done.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_utils_win_unittest.cc:137: const GURL
expected_return_value(
On 2016/01/20 03:03:02, Peter Kasting wrote:
> This doesn't seem to be used.

Done.

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines_strings.grdp (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines_strings.grdp:9: Manage search settings
On 2016/01/20 03:03:02, Peter Kasting wrote:
> Why is going to the settings useful?  Is there going to be a checkbox there to
> disable search redirection?  If not it seems somewhat unrelated to what the
> infobar is actually doing, and liable to just frustrate users who wanted a way
> to turn off this functionality.

The user can change its default search engine from the settings. It's relevant
here because desktop searches are redirected to the the default search engine.

ainslie@: Thoughts?

[Peter Kasting, 2016-01-22 00:38:57]

LGTM.  I'm definitely interested in hearing rkaplow/asvitkine describe the
reason for using both actions and histograms instead of just one.

https://codereview.chromium.org/1598553003/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1598553003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5961: +          Enable the experimental
implementation of the "Cache-Control: stale-while-revalidate" directive. This
permits servers to specify that some resources may be revalidated in the
background to improve latency.
On 2016/01/21 21:02:33, fdoray wrote:
> On 2016/01/20 03:03:01, Peter Kasting wrote:
> > What changed on this line?
> 
> A white space was removed by my editor at the end of the line. I put it back
to
> avoid polluting the diff.

Oh, that's totally cool.  I would go ahead and remove that whitespace; I just
didn't know what the delta was :)

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines/desktop_search_infobar_delegate_win.h (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines/desktop_search_infobar_delegate_win.h:43: //
ConfirmInfoBarDelegate:
On 2016/01/21 21:02:33, fdoray wrote:
> On 2016/01/20 03:03:01, Peter Kasting wrote:
> > It seems like we really ought to have a Learn More link where we explain
about
> > desktop search redirection and how to disable it if necessary?
> 
> UI review decided not to have an option to disable desktop search redirection.
> It is still possible to "disable" it by setting Bing as the default search
> engine.

Fair enough.  Make sure you ultimately remove any flags or preferences related
to disabling this as soon as you no longer need them for kill switch purposes. 
(Often times when we decide to make something un-disablable, the relevant
switches never get removed from the codebase, and turn into a "hidden config
switch"; we don't want that sort of thing.)

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
File components/search_engines_strings.grdp (right):

https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
components/search_engines_strings.grdp:9: Manage search settings
On 2016/01/21 21:02:34, fdoray wrote:
> On 2016/01/20 03:03:02, Peter Kasting wrote:
> > Why is going to the settings useful?  Is there going to be a checkbox there
to
> > disable search redirection?  If not it seems somewhat unrelated to what the
> > infobar is actually doing, and liable to just frustrate users who wanted a
way
> > to turn off this functionality.
> 
> The user can change its default search engine from the settings. It's relevant
> here because desktop searches are redirected to the the default search engine.

OK, if "disable" is done by setting the DSE to Bing, I'm fine with this.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
File components/search_engines/desktop_search_redirection_infobar_delegate.cc
(right):

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_redirection_infobar_delegate.cc:54: 
Nit: No need for this blank line.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_redirection_infobar_delegate.cc:109:
RecordDesktopSearchInfobarCloseActionHistogram(
Nit: I'd probably put this statement down just under the "Close the infobar"
comment, since it's specifically about "closing" and not just the manage search
settings button.

At that point, you could probably remove the blank line between RecordAction()
and running the callback, since those two things seem closely related.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
File components/search_engines/desktop_search_redirection_infobar_delegate.h
(right):

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_redirection_infobar_delegate.h:26: //
engine. |manage_search_settings_callback| opens the search settings page.
Nit: opens -> should open

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_redirection_infobar_delegate.h:36:
virtual ~DesktopSearchRedirectionInfobarDelegate();
Nit: ~DesktopSearchRedirectionInfobarDelegate() override;

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
File components/search_engines/desktop_search_utils.cc (right):

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.cc:29: namespace {
Nit: Put anonymous namespace stuff just below the #includes, above any other
namespaces/declarations

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.cc:112: const TemplateURL*
default_search_engine =
Nit: Put this block just below the next comment since it's closely associated
with checking what the default search engine is.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.cc:114:
DCHECK(default_search_engine);
It's not safe to DCHECK this; users may not have a DSE.  Instead, make sure this
case returns false (and probably records an INVALID_SEARCH_ENGINE action).

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
File components/search_engines/desktop_search_utils.h (right):

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.h:23: }  // namespace prefs
Nit: I wouldn't bother with this comment

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.h:30: // redirection infobar has
been shown in |registry|.
Nit: I'd remove "in |registry|" since that's implied anyway and its placement
makes it seem like it's a modifier for where the infobar was shown.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.h:34: // Replaces |url| by a
default search engine URL and returns true if:
Nit: Remove "and returns true"

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
File components/search_engines/desktop_search_utils_unittest.cc (right):

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils_unittest.cc:23: namespace {
Nit: Blank line below this and above the corresponding closing brace line

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils_unittest.cc:75:
TEST_F(DesktopSearchUtilsTest, ReplaceDesktopSearchURL) {
Nit: Add a brief comment above each test describing what functionality the test
is verifying.

[Alexei Svitkine (slow), 2016-01-22 15:54:05]

Re: The reason for both actions & histograms:

Histograms are the best way to look at aggregated counts (in terms of what
kind of analysis you can do with our dashboards). However, histograms don't
carry timing information - so one thing actions allow since they do carry
timing is analyzing sequences - i.e. what actions do users perform in what
order.

On Thu, Jan 21, 2016 at 7:38 PM, <pkasting@chromium.org> wrote:

> LGTM.  I'm definitely interested in hearing rkaplow/asvitkine describe the
> reason for using both actions and histograms instead of just one.
>
>
>
>
https://codereview.chromium.org/1598553003/diff/20001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
>
>
>
https://codereview.chromium.org/1598553003/diff/20001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:5961: +          Enable the
> experimental implementation of the "Cache-Control:
> stale-while-revalidate" directive. This permits servers to specify that
> some resources may be revalidated in the background to improve latency.
> On 2016/01/21 21:02:33, fdoray wrote:
>
>> On 2016/01/20 03:03:01, Peter Kasting wrote:
>> > What changed on this line?
>>
>
> A white space was removed by my editor at the end of the line. I put
>>
> it back to
>
>> avoid polluting the diff.
>>
>
> Oh, that's totally cool.  I would go ahead and remove that whitespace; I
> just didn't know what the delta was :)
>
>
>
https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
> File components/search_engines/desktop_search_infobar_delegate_win.h
> (right):
>
>
>
https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
> components/search_engines/desktop_search_infobar_delegate_win.h:43: //
> ConfirmInfoBarDelegate:
> On 2016/01/21 21:02:33, fdoray wrote:
>
>> On 2016/01/20 03:03:01, Peter Kasting wrote:
>> > It seems like we really ought to have a Learn More link where we
>>
> explain about
>
>> > desktop search redirection and how to disable it if necessary?
>>
>
> UI review decided not to have an option to disable desktop search
>>
> redirection.
>
>> It is still possible to "disable" it by setting Bing as the default
>>
> search
>
>> engine.
>>
>
> Fair enough.  Make sure you ultimately remove any flags or preferences
> related to disabling this as soon as you no longer need them for kill
> switch purposes.  (Often times when we decide to make something
> un-disablable, the relevant switches never get removed from the
> codebase, and turn into a "hidden config switch"; we don't want that
> sort of thing.)
>
>
>
https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
> File components/search_engines_strings.grdp (right):
>
>
>
https://codereview.chromium.org/1598553003/diff/20001/components/search_engin...
> components/search_engines_strings.grdp:9: Manage search settings
> On 2016/01/21 21:02:34, fdoray wrote:
>
>> On 2016/01/20 03:03:02, Peter Kasting wrote:
>> > Why is going to the settings useful?  Is there going to be a
>>
> checkbox there to
>
>> > disable search redirection?  If not it seems somewhat unrelated to
>>
> what the
>
>> > infobar is actually doing, and liable to just frustrate users who
>>
> wanted a way
>
>> > to turn off this functionality.
>>
>
> The user can change its default search engine from the settings. It's
>>
> relevant
>
>> here because desktop searches are redirected to the the default search
>>
> engine.
>
> OK, if "disable" is done by setting the DSE to Bing, I'm fine with this.
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> File
> components/search_engines/desktop_search_redirection_infobar_delegate.cc
> (right):
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
>
> components/search_engines/desktop_search_redirection_infobar_delegate.cc:54:
>
> Nit: No need for this blank line.
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
>
> components/search_engines/desktop_search_redirection_infobar_delegate.cc:109:
> RecordDesktopSearchInfobarCloseActionHistogram(
> Nit: I'd probably put this statement down just under the "Close the
> infobar" comment, since it's specifically about "closing" and not just
> the manage search settings button.
>
> At that point, you could probably remove the blank line between
> RecordAction() and running the callback, since those two things seem
> closely related.
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> File
> components/search_engines/desktop_search_redirection_infobar_delegate.h
> (right):
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_redirection_infobar_delegate.h:26:
> // engine. |manage_search_settings_callback| opens the search settings
> page.
> Nit: opens -> should open
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_redirection_infobar_delegate.h:36:
> virtual ~DesktopSearchRedirectionInfobarDelegate();
> Nit: ~DesktopSearchRedirectionInfobarDelegate() override;
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> File components/search_engines/desktop_search_utils.cc (right):
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.cc:29: namespace {
> Nit: Put anonymous namespace stuff just below the #includes, above any
> other namespaces/declarations
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.cc:112: const
> TemplateURL* default_search_engine =
> Nit: Put this block just below the next comment since it's closely
> associated with checking what the default search engine is.
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.cc:114:
> DCHECK(default_search_engine);
> It's not safe to DCHECK this; users may not have a DSE.  Instead, make
> sure this case returns false (and probably records an
> INVALID_SEARCH_ENGINE action).
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> File components/search_engines/desktop_search_utils.h (right):
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.h:23: }  // namespace
> prefs
> Nit: I wouldn't bother with this comment
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.h:30: // redirection
> infobar has been shown in |registry|.
> Nit: I'd remove "in |registry|" since that's implied anyway and its
> placement makes it seem like it's a modifier for where the infobar was
> shown.
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.h:34: // Replaces |url|
> by a default search engine URL and returns true if:
> Nit: Remove "and returns true"
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> File components/search_engines/desktop_search_utils_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils_unittest.cc:23: namespace
> {
> Nit: Blank line below this and above the corresponding closing brace
> line
>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils_unittest.cc:75:
> TEST_F(DesktopSearchUtilsTest, ReplaceDesktopSearchURL) {
> Nit: Add a brief comment above each test describing what functionality
> the test is verifying.
>
> https://codereview.chromium.org/1598553003/
>

-- 
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.

[Cait (Slow), 2016-01-22 16:03:48]

components/components_strings.grd, components/components_tests.gyp,
components/search_engines_strings.grdp
LGTM

[Peter Kasting, 2016-01-22 22:41:05]

On 2016/01/22 15:54:05, Alexei Svitkine (very slow) wrote:
> Re: The reason for both actions & histograms:
> 
> Histograms are the best way to look at aggregated counts (in terms of what
> kind of analysis you can do with our dashboards). However, histograms don't
> carry timing information - so one thing actions allow since they do carry
> timing is analyzing sequences - i.e. what actions do users perform in what
> order.

Do we need the timing information?  What sorts of analyses are we hoping to do
here?  Will this be removed down the road or kept forever?

Is there a way to make recording the relevant actions give us enough data to
infer the stuff we would have gotten from the histograms?  If we're recording
"something happened" in the same places, just as an action and not a histogram,
seems like we ought to be able to get the same answers from that.

The reason I ask all these questions is that the data collection is perhaps the
most complex bit of the whole CL, and if we could reduce its footprint that
would help; also, I've never run into other places in the code that record both
actions and histograms in pretty much all the same places, so it feels
surprising.

[fdoray, 2016-02-01 15:15:32]

On 2016/01/22 22:41:05, Peter Kasting wrote:
> On 2016/01/22 15:54:05, Alexei Svitkine (very slow) wrote:
> > Re: The reason for both actions & histograms:
> > 
> > Histograms are the best way to look at aggregated counts (in terms of what
> > kind of analysis you can do with our dashboards). However, histograms don't
> > carry timing information - so one thing actions allow since they do carry
> > timing is analyzing sequences - i.e. what actions do users perform in what
> > order.
> 
> Do we need the timing information?  What sorts of analyses are we hoping to do
> here?  Will this be removed down the road or kept forever?
> 
> Is there a way to make recording the relevant actions give us enough data to
> infer the stuff we would have gotten from the histograms?  If we're recording
> "something happened" in the same places, just as an action and not a
histogram,
> seems like we ought to be able to get the same answers from that.
> 
> The reason I ask all these questions is that the data collection is perhaps
the
> most complex bit of the whole CL, and if we could reduce its footprint that
> would help; also, I've never run into other places in the code that record
both
> actions and histograms in pretty much all the same places, so it feels
> surprising.

asvitkine@: Can you reply to pkasting's comment #15? I think that the
Search.DesktopSearch.RedirectionInfobarCloseAction histogram isn't very useful.
Sure, an histogram is a good way to look at aggregated counts. But I believe
that the DesktopSearchRedirectionInfoBar_* user actions allows us to get all the
information that the histogram provides. Wdyt?

[fdoray, 2016-02-01 15:15:54]

asvitkine@: Can you reply to pkasting's comment #15? I think that the
Search.DesktopSearch.RedirectionInfobarCloseAction histogram isn't very useful.
Sure, an histogram is a good way to look at aggregated counts. But I believe
that the DesktopSearchRedirectionInfoBar_* user actions allows us to get all the
information that the histogram provides. Wdyt?

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
File components/search_engines/desktop_search_redirection_infobar_delegate.cc
(right):

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_redirection_infobar_delegate.cc:54: 
On 2016/01/22 00:38:57, Peter Kasting wrote:
> Nit: No need for this blank line.

Done.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_redirection_infobar_delegate.cc:109:
RecordDesktopSearchInfobarCloseActionHistogram(
On 2016/01/22 00:38:57, Peter Kasting wrote:
> Nit: I'd probably put this statement down just under the "Close the infobar"
> comment, since it's specifically about "closing" and not just the manage
search
> settings button.
> 
> At that point, you could probably remove the blank line between RecordAction()
> and running the callback, since those two things seem closely related.

Done.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
File components/search_engines/desktop_search_redirection_infobar_delegate.h
(right):

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_redirection_infobar_delegate.h:26: //
engine. |manage_search_settings_callback| opens the search settings page.
On 2016/01/22 00:38:57, Peter Kasting wrote:
> Nit: opens -> should open

Done.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_redirection_infobar_delegate.h:36:
virtual ~DesktopSearchRedirectionInfobarDelegate();
On 2016/01/22 00:38:57, Peter Kasting wrote:
> Nit: ~DesktopSearchRedirectionInfobarDelegate() override;

Done.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
File components/search_engines/desktop_search_utils.cc (right):

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.cc:29: namespace {
On 2016/01/22 00:38:57, Peter Kasting wrote:
> Nit: Put anonymous namespace stuff just below the #includes, above any other
> namespaces/declarations

Done.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.cc:112: const TemplateURL*
default_search_engine =
On 2016/01/22 00:38:57, Peter Kasting wrote:
> Nit: Put this block just below the next comment since it's closely associated
> with checking what the default search engine is.

Done.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.cc:114:
DCHECK(default_search_engine);
On 2016/01/22 00:38:57, Peter Kasting wrote:
> It's not safe to DCHECK this; users may not have a DSE.  Instead, make sure
this
> case returns false (and probably records an INVALID_SEARCH_ENGINE action).

Done.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
File components/search_engines/desktop_search_utils.h (right):

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.h:23: }  // namespace prefs
On 2016/01/22 00:38:57, Peter Kasting wrote:
> Nit: I wouldn't bother with this comment

Done.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.h:30: // redirection infobar has
been shown in |registry|.
On 2016/01/22 00:38:57, Peter Kasting wrote:
> Nit: I'd remove "in |registry|" since that's implied anyway and its placement
> makes it seem like it's a modifier for where the infobar was shown.

Done.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils.h:34: // Replaces |url| by a
default search engine URL and returns true if:
On 2016/01/22 00:38:57, Peter Kasting wrote:
> Nit: Remove "and returns true"

Done.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
File components/search_engines/desktop_search_utils_unittest.cc (right):

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils_unittest.cc:23: namespace {
On 2016/01/22 00:38:57, Peter Kasting wrote:
> Nit: Blank line below this and above the corresponding closing brace line

Done.

https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
components/search_engines/desktop_search_utils_unittest.cc:75:
TEST_F(DesktopSearchUtilsTest, ReplaceDesktopSearchURL) {
On 2016/01/22 00:38:57, Peter Kasting wrote:
> Nit: Add a brief comment above each test describing what functionality the
test
> is verifying.

Done.

[Alexei Svitkine (slow), 2016-02-01 16:19:38]

On Mon, Feb 1, 2016 at 10:15 AM, <fdoray@chromium.org> wrote:

>
> asvitkine@: Can you reply to pkasting's comment #15? I think that the
> Search.DesktopSearch.RedirectionInfobarCloseAction histogram isn't very
useful.
> Sure, an histogram is a good way to look at aggregated counts. But I believe
> that the DesktopSearchRedirectionInfoBar_* user actions allows us to get all
the
> information that the histogram provides. Wdyt?
>
>
It's up to you. If you just use actions, you lose a lot of dashboard
capabilities - e.g. such as slicing by many different filters (i.e. the
counts dashboard doesn't support filters). It *is* possible to do this kind
of analysis with actions, but the tools don't currently make it convenient.

FWIW, it would be nice if in the future we could define a "virtual" enum
histogram that just groups some actions and have the backend pipeline
generate these from the action data, but such a capability doesn't exist
currently.


>
>
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> File
> components/search_engines/desktop_search_redirection_infobar_delegate.cc
> (right):
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_redirection_infobar_delegate.cc:54
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
>
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > Nit: No need for this blank line.
>
> Done.
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_redirection_infobar_delegate.cc:109
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
> RecordDesktopSearchInfobarCloseActionHistogram(
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > Nit: I'd probably put this statement down just under the "Close the
> infobar"
> > comment, since it's specifically about "closing" and not just the
> manage search
> > settings button.
> >
> > At that point, you could probably remove the blank line between
> RecordAction()
> > and running the callback, since those two things seem closely related.
>
> Done.
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> File
> components/search_engines/desktop_search_redirection_infobar_delegate.h
> (right):
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_redirection_infobar_delegate.h:26
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
> // engine. |manage_search_settings_callback| opens the search settings
> page.
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > Nit: opens -> should open
>
> Done.
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_redirection_infobar_delegate.h:36
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
> virtual ~DesktopSearchRedirectionInfobarDelegate();
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > Nit: ~DesktopSearchRedirectionInfobarDelegate() override;
>
> Done.
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> File components/search_engines/desktop_search_utils.cc (right):
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.cc:29
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
namespace {
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > Nit: Put anonymous namespace stuff just below the #includes, above any
> other
> > namespaces/declarations
>
> Done.
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.cc:112
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
const
> TemplateURL* default_search_engine =
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > Nit: Put this block just below the next comment since it's closely
> associated
> > with checking what the default search engine is.
>
> Done.
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.cc:114
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
> DCHECK(default_search_engine);
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > It's not safe to DCHECK this; users may not have a DSE.  Instead, make
> sure this
> > case returns false (and probably records an INVALID_SEARCH_ENGINE
> action).
>
> Done.
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> File components/search_engines/desktop_search_utils.h (right):
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.h:23
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
}  // namespace
> prefs
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > Nit: I wouldn't bother with this comment
>
> Done.
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.h:30
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
// redirection
> infobar has been shown in |registry|.
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > Nit: I'd remove "in |registry|" since that's implied anyway and its
> placement
> > makes it seem like it's a modifier for where the infobar was shown.
>
> Done.
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils.h:34
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
// Replaces |url|
> by a default search engine URL and returns true if:
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > Nit: Remove "and returns true"
>
> Done.
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> File components/search_engines/desktop_search_utils_unittest.cc (right):
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils_unittest.cc:23
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
namespace
> {
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > Nit: Blank line below this and above the corresponding closing brace
> line
>
> Done.
>
https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...
> components/search_engines/desktop_search_utils_unittest.cc:75
<https://codereview.chromium.org/1598553003/diff/60001/components/search_engin...>:
> TEST_F(DesktopSearchUtilsTest, ReplaceDesktopSearchURL) {
> On 2016/01/22 00:38:57, Peter Kasting wrote:
> > Nit: Add a brief comment above each test describing what functionality
> the test
> > is verifying.
>
> Done.
> https://codereview.chromium.org/1598553003/
>
>

-- 
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.

[fdoray, 2016-02-02 16:36:18]

> It's up to you. If you just use actions, you lose a lot of dashboard
> capabilities - e.g. such as slicing by many different filters (i.e. the
> counts dashboard doesn't support filters). It *is* possible to do this kind
> of analysis with actions, but the tools don't currently make it convenient.
> 
> FWIW, it would be nice if in the future we could define a "virtual" enum
> histogram that just groups some actions and have the backend pipeline
> generate these from the action data, but such a capability doesn't exist
> currently.
> 
I'll keep the histogram and I'll see how I end up using it.

[fdoray, 2016-02-02 16:36:25]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-02-02 16:36:26]

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

[commit-bot: I haz the power, 2016-02-02 16:39:46]

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

[commit-bot: I haz the power, 2016-02-02 16:50:37]

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

[commit-bot: I haz the power, 2016-02-02 16:50:39]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  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_...)

[fdoray, 2016-02-02 20:31:25]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-02-02 20:31:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, asvitkine@chromium.org, caitkp@chromium.org
Link to the patchset: https://codereview.chromium.org/1598553003/#ps140001
(title: "fix build errors due to prefs being moved in components")

[commit-bot: I haz the power, 2016-02-02 20:32:39]

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

[commit-bot: I haz the power, 2016-02-02 21:56:49]

Description was changed from

==========
Implement the Windows desktop search redirection feature.

With this CL, Windows desktop searches are redirected to the default
search engine when the DesktopSearchRedirection feature is enabled.
The first time that a desktop search is redirected, an infobar is
shown to inform the user of the redirection.

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

to

==========
Implement the Windows desktop search redirection feature.

With this CL, Windows desktop searches are redirected to the default
search engine when the DesktopSearchRedirection feature is enabled.
The first time that a desktop search is redirected, an infobar is
shown to inform the user of the redirection.

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

[commit-bot: I haz the power, 2016-02-02 21:56:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-02 21:58:23]

Description was changed from

==========
Implement the Windows desktop search redirection feature.

With this CL, Windows desktop searches are redirected to the default
search engine when the DesktopSearchRedirection feature is enabled.
The first time that a desktop search is redirected, an infobar is
shown to inform the user of the redirection.

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

to

==========
Implement the Windows desktop search redirection feature.

With this CL, Windows desktop searches are redirected to the default
search engine when the DesktopSearchRedirection feature is enabled.
The first time that a desktop search is redirected, an infobar is
shown to inform the user of the redirection.

BUG=539963

Committed: https://crrev.com/2e7c477ce837b177ed32eaa7a2fd1e7911b67a87
Cr-Commit-Position: refs/heads/master@{#373049}
==========

[commit-bot: I haz the power, 2016-02-02 21:58:24]

Patchset 8 (id:??) landed as
https://crrev.com/2e7c477ce837b177ed32eaa7a2fd1e7911b67a87
Cr-Commit-Position: refs/heads/master@{#373049}