search: Log when bookmark bar is pinned.

search: Log when bookmark bar is pinned.

We want to understand how showing search terms in the omnibox interacts
with the presence of the bookmark bar. For users in a field trial that's
showing search terms, this change adds a CGI arg &bmbp=1 to search URLs
if the bookmark bar is pinned and &bmbp=0 otherwise.

This isn't privacy-sensitive, because in theory we could get this
information by looking at outerHeight - innerHeight from search page
Javascript. Logging it explicitly this way is just more reliable than
doing that.

BUG=291244
TEST=unit tests,manual

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

[Peter Kasting, 2013-10-01 00:51:56]

Instead of adding a CGI param, why not just extend Chrome's UMA data to capture
this?  We're already recording histograms of omnibox content similarity for
navigations to/from SERPs/non-SERPs.  We could just e.g. add another histogram
counting the total numbers of navigations of each type with the bookmark bar on
versus off.

It seems like the downside of that method is that it's harder to try and do
sophisticated analysis not just on whether the bookmark bar affects query
refinement but in precisely what ways it affects it (by analyzing the query
strings we see before/after refinement in all such cases).  UMA data would be
limited to perhaps recording the length of the common prefix, or splitting
search->search navigations into multiple buckets based on common prefix length.

[samarth, 2013-10-01 00:55:57]

On Mon, Sep 30, 2013 at 5:51 PM, <pkasting@chromium.org> wrote:

> It seems like the downside of that method is that it's harder to try and do
> sophisticated analysis not just on whether the bookmark bar affects query
> refinement but in precisely what ways it affects it (by analyzing the query
> strings we see before/after refinement in all such cases).


That's exactly the kinds of analysis we would like to do, and that this
change would enable.  More generally, we'd like to look at the search
metrics overall with and without the pinned bar.


>  UMA data would be
> limited to perhaps recording the length of the common prefix, or splitting
> search->search navigations into multiple buckets based on common prefix
> length.


>
https://codereview.chromium.**org/25324003/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2013-10-01 01:03:31]

OK than

https://codereview.chromium.org/25324003/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/25324003/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider_unittest.cc:2632:
ASSERT_EQ(AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
Nit: Can be EXPECT_EQ

https://codereview.chromium.org/25324003/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider_unittest.cc:2638:
TEST_F(SearchProviderTest, SetsBookmarkBarPinned) {
Nit: I'd combine these two tests into one, BookmarkBarStateReflected (or some
name like that)

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 65
You have to increment this.

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.cc:844: base::StringPrintf("%d",
search_terms_args.bookmark_bar_pinned),
Nit: What about just

  search_terms_args.bookmark_bar_pinned ? "1" : "0"

This seems clearer and faster, and since it's shorter, you could then collapse
the args onto fewer lines, like most of the cases around this try to do.

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.h:125: bool bookmark_bar_pinned;
Tiny nit: Let's move this up above append_extra_query_params, since that's
basically the end of the "non-image-search args" section.

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_unittest.cc:57:
field_trial_list_.reset(new base::FieldTrialList(
I'm not thrilled about going through the field trial machinery just to flip the
"enabled on SERP" bit on and off.  It seems better to have some sort of
test-only direct setter to toggle this state.

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_unittest.cc:1266:
TEST_F(TemplateURLTest, BookmarkBarPinnedArgReplaced) {
Nit: Again, I'd make this all one test, since it's all related.  It saves the
big block of code to set up the TemplateURL too.

This is easier when toggling the "suppressed on SERPs" bit is doable via a
simple method call as suggested above.

[Peter Kasting, 2013-10-01 01:04:10]

On 2013/10/01 01:03:31, Peter Kasting wrote:
> OK than

That was supposed to be "then", and it was supposed to be a reply to Samarth's
comment.  (oops)

[Jered, 2013-10-01 18:46:15]

https://codereview.chromium.org/25324003/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/25324003/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider_unittest.cc:2632:
ASSERT_EQ(AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
On 2013/10/01 01:03:31, Peter Kasting wrote:
> Nit: Can be EXPECT_EQ

Done.

https://codereview.chromium.org/25324003/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider_unittest.cc:2638:
TEST_F(SearchProviderTest, SetsBookmarkBarPinned) {
On 2013/10/01 01:03:31, Peter Kasting wrote:
> Nit: I'd combine these two tests into one, BookmarkBarStateReflected (or some
> name like that)

Done.

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 65
On 2013/10/01 01:03:31, Peter Kasting wrote:
> You have to increment this.

Done.

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.cc:844: base::StringPrintf("%d",
search_terms_args.bookmark_bar_pinned),
On 2013/10/01 01:03:31, Peter Kasting wrote:
> Nit: What about just
> 
>   search_terms_args.bookmark_bar_pinned ? "1" : "0"
> 
> This seems clearer and faster, and since it's shorter, you could then collapse
> the args onto fewer lines, like most of the cases around this try to do.

Thanks, that's much better. Done.

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url.h:125: bool bookmark_bar_pinned;
On 2013/10/01 01:03:31, Peter Kasting wrote:
> Tiny nit: Let's move this up above append_extra_query_params, since that's
> basically the end of the "non-image-search args" section.

Done.

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_unittest.cc:57:
field_trial_list_.reset(new base::FieldTrialList(
On 2013/10/01 01:03:31, Peter Kasting wrote:
> I'm not thrilled about going through the field trial machinery just to flip
the
> "enabled on SERP" bit on and off.  It seems better to have some sort of
> test-only direct setter to toggle this state.

This state is field trial state, so going through the machinery is a necessary
evil unless we replicate it somewhere else. The realistic choices for this CL
are a new global bool, or TemplateURLRef itself, so I opted to do it in
TemplateURLRef.

https://codereview.chromium.org/25324003/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_unittest.cc:1266:
TEST_F(TemplateURLTest, BookmarkBarPinnedArgReplaced) {
On 2013/10/01 01:03:31, Peter Kasting wrote:
> Nit: Again, I'd make this all one test, since it's all related.  It saves the
> big block of code to set up the TemplateURL too.
> 
> This is easier when toggling the "suppressed on SERPs" bit is doable via a
> simple method call as suggested above.

Done.

[Peter Kasting, 2013-10-01 21:54:55]

LGTM

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider_unittest.cc:2630: 
Nit: Remove this (since you don't have a similar blank line in the second block
below)

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.cc:189:
!chrome::ShouldSuppressInstantExtendedOnSRP();
Nit: Indent 4, not even

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.h:392: // Whether search terms are
shown in the omnibox on search results pages.
Nit: Maybe add "This is kept as a member so it can be overridden by tests."

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:1251: // Add the param
when InstantExtended is suppressed on SRPs.
Nit: _not_ suppressed

[Jered, 2013-10-01 23:07:07]

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider_unittest.cc:2630: 
On 2013/10/01 21:54:55, Peter Kasting wrote:
> Nit: Remove this (since you don't have a similar blank line in the second
block
> below)

Done.

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.cc (right):

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.cc:189:
!chrome::ShouldSuppressInstantExtendedOnSRP();
On 2013/10/01 21:54:55, Peter Kasting wrote:
> Nit: Indent 4, not even

Done.

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url.h:392: // Whether search terms are
shown in the omnibox on search results pages.
On 2013/10/01 21:54:55, Peter Kasting wrote:
> Nit: Maybe add "This is kept as a member so it can be overridden by tests."

Done.

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/25324003/diff/12001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:1251: // Add the param
when InstantExtended is suppressed on SRPs.
On 2013/10/01 21:54:55, Peter Kasting wrote:
> Nit: _not_ suppressed

Done.

[commit-bot: I haz the power, 2013-10-01 23:08:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jered@chromium.org/25324003/20001

[commit-bot: I haz the power, 2013-10-02 00:00:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Jered, 2013-10-02 00:03:30]

On 2013/10/02 00:00:56, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on win_x64_rel.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

Wasn't me. I'll dcommit when the other platforms finish.

[Jered, 2013-10-02 16:08:15]

On 2013/10/02 00:03:30, Jered wrote:
> On 2013/10/02 00:00:56, I haz the power (commit-bot) wrote:
> > Sorry for I got bad news for ya.
> > Compile failed with a clobber build on win_x64_rel.
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
> > Your code is likely broken or HEAD is junk. Please ensure your
> > code is not broken then alert the build sheriffs.
> > Look at the try server FAQ for more details.
> 
> Wasn't me. I'll dcommit when the other platforms finish.

Maybe win_rel will be better today. Let's try again.

[commit-bot: I haz the power, 2013-10-02 16:09:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jered@chromium.org/25324003/20001

[commit-bot: I haz the power, 2013-10-02 18:30:12]

Change committed as 226506