Add Apps.AppListSearchResultDistanceFromOrigin UMA stat.

Add Apps.AppListSearchResultDistanceFromOrigin UMA stat.

This CL adds a UMA stat that tracks how far an opened app list search
result is from the top left of the search results view. This will provide
information on the quality of search result ranking.

BUG=485449
Committed: https://crrev.com/9aa69c916466e9d1f3527847f1ece710eeeab7ab
Cr-Commit-Position: refs/heads/master@{#329368}

[calamity, 2015-05-11 05:47:39]

calamity@chromium.org changed reviewers:
+ mgiuca@chromium.org

[Matt Giuca, 2015-05-11 06:50:41]

Wow, pretty hard code for stats.

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_container_view.h (right):

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_container_view.h:48: virtual void
NotifyFirstResultYIndex(int y_index) = 0;
The documentation should say what this function *does*, not what its caller
does.

Also "relative to all other result containers" doesn't make sense; it is
absolute, not relative to any particular container.

How about:
// Updates the distance_from_origin() properties of the results in this
container. |y_index| is the absolute y-index of the first result of this
container (counting from the top of the app list).

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_list_view.cc (right):

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_list_view.cc:150:
result_view->result()->set_distance_from_origin(i + y_index);
nit: Can you just combine this into one statement (since you only use
|result_view| once, there is little value in storing it in a variable).

If so, no curly braces unless it goes over 2 lines.

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_page_view.cc (right):

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_page_view.cc:167: if (view->UpdateScheduled())
Shouldn't you still take into account the GetYSize() of a container that has not
finished updating?

i.e.:

if (!view->UpdateScheduled())
  view->NotifyFirstResultYIndex(...);

result_y_index += view->GetYSize();  // (unconditionally)

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_tile_item_list_view.cc (right):

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_tile_item_list_view.cc:62: for (size_t i = 0; i
< static_cast<size_t>(num_results()); ++i) {
nit: No curlies.

[calamity, 2015-05-11 07:13:54]

Patchset #2 (id:20001) has been deleted

[calamity, 2015-05-11 07:15:02]

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

[calamity, 2015-05-11 07:15:03]

+asvitkine for histograms OWNERS.

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_container_view.h (right):

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_container_view.h:48: virtual void
NotifyFirstResultYIndex(int y_index) = 0;
On 2015/05/11 06:50:40, Matt Giuca wrote:
> The documentation should say what this function *does*, not what its caller
> does.
> 
> Also "relative to all other result containers" doesn't make sense; it is
> absolute, not relative to any particular container.
> 
> How about:
> // Updates the distance_from_origin() properties of the results in this
> container. |y_index| is the absolute y-index of the first result of this
> container (counting from the top of the app list).

Done.

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_list_view.cc (right):

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_list_view.cc:150:
result_view->result()->set_distance_from_origin(i + y_index);
On 2015/05/11 06:50:41, Matt Giuca wrote:
> nit: Can you just combine this into one statement (since you only use
> |result_view| once, there is little value in storing it in a variable).
> 
> If so, no curly braces unless it goes over 2 lines.

Done.

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_page_view.cc (right):

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_page_view.cc:167: if (view->UpdateScheduled())
On 2015/05/11 06:50:41, Matt Giuca wrote:
> Shouldn't you still take into account the GetYSize() of a container that has
not
> finished updating?
> 
> i.e.:
> 
> if (!view->UpdateScheduled())
>   view->NotifyFirstResultYIndex(...);
> 
> result_y_index += view->GetYSize();  // (unconditionally)

Yeah.. I thought about this too. Ideally we'd clear all result containers at the
beginning of a search. I think there might other issues with that though..

I'll do this for now I guess. Practically, it doesn't actually come up because
both lists are updated basically simultaneously.

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_tile_item_list_view.cc (right):

https://codereview.chromium.org/1132943004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_tile_item_list_view.cc:62: for (size_t i = 0; i
< static_cast<size_t>(num_results()); ++i) {
On 2015/05/11 06:50:41, Matt Giuca wrote:
> nit: No curlies.

Gah, it got git cl formatted and I didn't notice it didn't need braces anymore.

[Matt Giuca, 2015-05-11 07:22:05]

lgtm

[Alexei Svitkine (slow), 2015-05-11 15:00:39]

lgtm

[calamity, 2015-05-12 01:14:02]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2015-05-12 01:15:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132943004/40001

[commit-bot: I haz the power, 2015-05-12 02:35:02]

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

[commit-bot: I haz the power, 2015-05-12 02:35:03]

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

[calamity, 2015-05-12 02:46:54]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2015-05-12 02:47:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132943004/40001

[commit-bot: I haz the power, 2015-05-12 04:20:29]

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

[commit-bot: I haz the power, 2015-05-12 04:20:30]

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

[calamity, 2015-05-12 05:32:51]

The CQ bit was checked by calamity@chromium.org

[calamity, 2015-05-12 05:32:51]

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

[commit-bot: I haz the power, 2015-05-12 05:33:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132943004/60001

[commit-bot: I haz the power, 2015-05-12 07:25:35]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-05-12 07:26:13]

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