Add UMA action InstantExtended.ShowSRP.

Add UMA action InstantExtended.ShowSRP.

chromeactions.txt also picked up some hashes for other actions
that were added but didn't update the file.

BUG=178525
TEST=Click on a clickable doodle or search through
the omnibox and check that the new UMA action is
shown on chrome://user-actions.

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

[sreeram, 2013-03-06 22:23:57]

What's the definition of "ShowSRP"? As the CL stands, this action won't capture
search results that are shown, perhaps even for long periods of time, unless the
user commits the search (i.e., presses Enter or selects a query suggestion).
This is applicable to both the overlay as well as committed tabs such as the
NTP.

[Alexei Svitkine (slow), 2013-03-06 22:26:21]

On 2013/03/06 22:23:57, sreeram wrote:
> What's the definition of "ShowSRP"? As the CL stands, this action won't
capture
> search results that are shown, perhaps even for long periods of time, unless
the
> user commits the search (i.e., presses Enter or selects a query suggestion).
> This is applicable to both the overlay as well as committed tabs such as the
> NTP.

Interesting. I'm actually interested in interactions from the NTP and this is a
proxy for things like clicking on the doodle, which results in a search without
the user typing (user typing can be detected with a different action).

Perhaps ShowSRP is a bad name for it. Perhaps CommittedSRP?

[sreeram, 2013-03-07 05:42:38]

On 2013/03/06 22:26:21, Alexei Svitkine wrote:
> On 2013/03/06 22:23:57, sreeram wrote:
> > What's the definition of "ShowSRP"? As the CL stands, this action won't
> capture
> > search results that are shown, perhaps even for long periods of time, unless
> the
> > user commits the search (i.e., presses Enter or selects a query suggestion).
> > This is applicable to both the overlay as well as committed tabs such as the
> > NTP.
> 
> Interesting. I'm actually interested in interactions from the NTP and this is
a
> proxy for things like clicking on the doodle, which results in a search
without
> the user typing (user typing can be detected with a different action).
> 
> Perhaps ShowSRP is a bad name for it. Perhaps CommittedSRP?

So, you want to count only non-typed interactions that lead to search results? I
think that's going to be tricky. Even if you just add your UMA metric to where
we observe NAV_ENTRY_COMMITTED (e.g.: in search_tab_helper.cc), it isn't obvious
whether the navigation was because of the user clicking a link or the user
hitting Enter after typing in the omnibox.

[Alexei Svitkine (slow), 2013-03-07 19:36:34]

On 2013/03/07 05:42:38, sreeram wrote:
> On 2013/03/06 22:26:21, Alexei Svitkine wrote:
> > On 2013/03/06 22:23:57, sreeram wrote:
> > > What's the definition of "ShowSRP"? As the CL stands, this action won't
> > capture
> > > search results that are shown, perhaps even for long periods of time,
unless
> > the
> > > user commits the search (i.e., presses Enter or selects a query
suggestion).
> > > This is applicable to both the overlay as well as committed tabs such as
the
> > > NTP.
> > 
> > Interesting. I'm actually interested in interactions from the NTP and this
is
> a
> > proxy for things like clicking on the doodle, which results in a search
> without
> > the user typing (user typing can be detected with a different action).
> > 
> > Perhaps ShowSRP is a bad name for it. Perhaps CommittedSRP?
> 
> So, you want to count only non-typed interactions that lead to search results?
I
> think that's going to be tricky. Even if you just add your UMA metric to where
> we observe NAV_ENTRY_COMMITTED (e.g.: in search_tab_helper.cc), it isn't
obvious
> whether the navigation was because of the user clicking a link or the user
> hitting Enter after typing in the omnibox.

The idea is we can correlate it server-side with existing OmniboxInputInProgress
action, and only consider cases where ShowNTP -> ShowSRP without a
OmniboxInputInProgress in between, which would get us those cases.

[Alexei Svitkine (slow), 2013-03-08 17:47:40]

On 2013/03/07 19:36:34, Alexei Svitkine wrote:
> The idea is we can correlate it server-side with existing
OmniboxInputInProgress
> action, and only consider cases where ShowNTP -> ShowSRP without a
> OmniboxInputInProgress in between, which would get us those cases.

Does that answer your question?

[sreeram, 2013-03-08 19:14:45]

On 2013/03/07 19:36:34, Alexei Svitkine wrote:
> The idea is we can correlate it server-side with existing
OmniboxInputInProgress
> action, and only consider cases where ShowNTP -> ShowSRP without a
> OmniboxInputInProgress in between, which would get us those cases.

Thanks for the explanation. However, this CL doesn't quite solve the following
scenario:
1. You are on a search results page in tab A.
2. You create a new Instant NTP, say tab B.
--> ShowNTP is fired.
3. You switch to tab A.
--> ModeChanged() is fired, and thus ShowSRP.

IMHO, I think you shouldn't be trying to do this in Chrome code. As with that
other bug about counting the Instant pref checkbox state, I think this is best
done server-side, for several reasons:

1. It's more complicated than simply observing ModeChanged() or
NAV_ENTRY_COMMITTED or such. If you can come up with a simple solution that
handles all cases, great. It doesn't appear easy, at first glance.

2. AFAIK, UMA actions or histograms are meant to be consumed via the dashboard,
which just counts things. There's no time-sequence based analysis. Of course,
you can write your own saw scripts to do the analysis; the system doesn't make
it easy. In contrast, search logs analysis is often performed on sessions logs,
which are time-sequenced. So this type of analysis (ShowNTP -> ShowSRP) will
fall quite naturally into that paradigm.

3. The server already has all these pieces of information. It knows when the
homepage was revelead (ShowNTP). It knows when a search was performed (ShowSRP).
It knows this per-tab (cf: the psi CGI argument).

[Alexei Svitkine (slow), 2013-03-08 19:21:47]

Re: Points 2. and 3. - see some of the discussions on b/8209891.

Per the discussion there, we're interested in doing this analysis using UMA
actions.

Let's follow-up on the internal bug.


On Fri, Mar 8, 2013 at 2:14 PM, <sreeram@chromium.org> wrote:

> On 2013/03/07 19:36:34, Alexei Svitkine wrote:
>
>> The idea is we can correlate it server-side with existing
>>
> OmniboxInputInProgress
>
>> action, and only consider cases where ShowNTP -> ShowSRP without a
>> OmniboxInputInProgress in between, which would get us those cases.
>>
>
> Thanks for the explanation. However, this CL doesn't quite solve the
> following
> scenario:
> 1. You are on a search results page in tab A.
> 2. You create a new Instant NTP, say tab B.
> --> ShowNTP is fired.
> 3. You switch to tab A.
> --> ModeChanged() is fired, and thus ShowSRP.
>
> IMHO, I think you shouldn't be trying to do this in Chrome code. As with
> that
> other bug about counting the Instant pref checkbox state, I think this is
> best
> done server-side, for several reasons:
>
> 1. It's more complicated than simply observing ModeChanged() or
> NAV_ENTRY_COMMITTED or such. If you can come up with a simple solution that
> handles all cases, great. It doesn't appear easy, at first glance.
>
> 2. AFAIK, UMA actions or histograms are meant to be consumed via the
> dashboard,
> which just counts things. There's no time-sequence based analysis. Of
> course,
> you can write your own saw scripts to do the analysis; the system doesn't
> make
> it easy. In contrast, search logs analysis is often performed on sessions
> logs,
> which are time-sequenced. So this type of analysis (ShowNTP -> ShowSRP)
> will
> fall quite naturally into that paradigm.
>
> 3. The server already has all these pieces of information. It knows when
> the
> homepage was revelead (ShowNTP). It knows when a search was performed
> (ShowSRP).
> It knows this per-tab (cf: the psi CGI argument).
>
>
https://codereview.chromium.**org/12546011/<https://codereview.chromium.org/1...
>

[Alexei Svitkine (slow), 2013-03-08 22:06:42]

I've moved InstantExtended.ShowNTP into ModeChanged() now too, per your
suggestion.

PTAL

[sreeram, 2013-03-08 22:41:08]

lgtm

[commit-bot: I haz the power, 2013-03-08 22:41:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/12546011/12001

[commit-bot: I haz the power, 2013-03-08 22:42:07]

Presubmit check for 12546011-12001 failed and returned exit status 1.

INFO:root:Found 2 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/tools/chromeactions.txt
    chrome/browser/ui/browser_instant_controller.cc

Presubmit checks took 3.9s to calculate.

[Alexei Svitkine (slow), 2013-03-08 22:42:53]

+sky for owners

[samarth, 2013-03-08 22:44:18]

https://codereview.chromium.org/12546011/diff/12001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12546011/diff/12001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:245:
content::RecordAction(UserMetricsAction("InstantExtended.ShowNTP"));
Sorry, I tried to follow Sreeram and your discussion and this still doesn't make
sense to me.  So if you switch away from an NTP tab and then switch back, this
will fire a second ShowNTP.  That doesn't seem right to me.

[samarth, 2013-03-08 22:48:11]

lgtm

https://codereview.chromium.org/12546011/diff/12001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12546011/diff/12001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:245:
content::RecordAction(UserMetricsAction("InstantExtended.ShowNTP"));
On 2013/03/08 22:44:18, samarth wrote:
> Sorry, I tried to follow Sreeram and your discussion and this still doesn't
make
> sense to me.  So if you switch away from an NTP tab and then switch back, this
> will fire a second ShowNTP.  That doesn't seem right to me.

OK, Sreeram pointed me to the internal thread that explains why this makes
sense.  It's probably worth adding a comment for why we log this way since it
really isn't obvious otherwise.

[sky, 2013-03-11 15:02:06]

LGTM

[Alexei Svitkine (slow), 2013-03-11 15:13:42]

https://codereview.chromium.org/12546011/diff/12001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12546011/diff/12001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:245:
content::RecordAction(UserMetricsAction("InstantExtended.ShowNTP"));
On 2013/03/08 22:48:11, samarth wrote:
> On 2013/03/08 22:44:18, samarth wrote:
> > Sorry, I tried to follow Sreeram and your discussion and this still doesn't
> make
> > sense to me.  So if you switch away from an NTP tab and then switch back,
this
> > will fire a second ShowNTP.  That doesn't seem right to me.
> 
> OK, Sreeram pointed me to the internal thread that explains why this makes
> sense.  It's probably worth adding a comment for why we log this way since it
> really isn't obvious otherwise.

Comment added.

[commit-bot: I haz the power, 2013-03-11 15:13:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/12546011/25001

[commit-bot: I haz the power, 2013-03-11 17:17:14]

Change committed as 187308