[MD History] Copy stats from the old history page.

[MD History] Copy stats from the old history page.

This CL reimplements the UMA actions and histograms that were in the old
history page.

The HistoryPage.RemoveEntryPosition histogram will only register
removals via the 'Remove from history' menu button.

BUG=614609
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/8a0c47c2aa6dfb3daca92636e4d8d45e7ab0d647
Cr-Commit-Position: refs/heads/master@{#413986}

[calamity, 2016-08-18 01:38:40]

Description was changed from

==========
[MD History] Copy stats from the old history page.

This CL reimplements the UMA actions and histograms that were in the old
history page.

BUG=614609
==========

to

==========
[MD History] Copy stats from the old history page.

This CL reimplements the UMA actions and histograms that were in the old
history page.

BUG=614609
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2016-08-18 03:11:45]

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

[calamity, 2016-08-18 03:11:46]

Please review your own CL.

[tsergeant, 2016-08-18 04:14:34]

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/constants.js (right):

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/constants.js:62:
LINK_RIGHT_CLICKED_DEPRECATED: 3,
Why are these deprecated? This one and 'SHOW_SESSION_MENU' seem like they could
still be collected.

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:87:
results.forEach(function(item, index) {
Can this be done with databinding?

index="[[index]]"

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list_behavior.js (right):

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list_behavior.js:175: var
browserService = md_history.BrowserService.getInstance();
You need to be really careful about grouped history here:

1. UMA code will trigger for deleting container arrays
2. The index is relative to the container array, rather than to the overall
list.

Both of these will work fine in regular history but give wonky values in
grouped.

https://codereview.chromium.org/2255033002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_metrics_test.js (right):

https://codereview.chromium.org/2255033002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_metrics_test.js:130:
MockInteractions.tap(app.$.toolbar.$$('#delete-button'));
You can check that RemoveSelected is recorded here.

[calamity, 2016-08-19 12:56:39]

Description was changed from

==========
[MD History] Copy stats from the old history page.

This CL reimplements the UMA actions and histograms that were in the old
history page.

BUG=614609
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD History] Copy stats from the old history page.

This CL reimplements the UMA actions and histograms that were in the old
history page.

BUG=614609
NOPRESUBMIT = true # crisper.js
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2016-08-19 13:59:44]

Ok.. My rebase has gone horribly wrong here so patchset diffs aren't going to
help.

The main difference that isn't next to your comments is the extra test that's
been added.

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/constants.js (right):

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/constants.js:62:
LINK_RIGHT_CLICKED_DEPRECATED: 3,
On 2016/08/18 04:14:34, tsergeant wrote:
> Why are these deprecated? This one and 'SHOW_SESSION_MENU' seem like they
could
> still be collected.

I suppose I can add these. They just didn't exist back in the old history page
anymore..

Done.

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list.js (right):

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list.js:87:
results.forEach(function(item, index) {
On 2016/08/18 04:14:34, tsergeant wrote:
> Can this be done with databinding?
> 
> index="[[index]]"

This is on the actual HistoryEntry backing object, not the history-item. I think
that if I bound it to the history-item, I would need the deleted history-item to
be rendered so I could read the index (which won't always be the case with
iron-list).

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_history/history_list_behavior.js (right):

https://codereview.chromium.org/2255033002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_history/history_list_behavior.js:175: var
browserService = md_history.BrowserService.getInstance();
On 2016/08/18 04:14:34, tsergeant wrote:
> You need to be really careful about grouped history here:
> 
> 1. UMA code will trigger for deleting container arrays
> 2. The index is relative to the container array, rather than to the overall
> list.
> 
> Both of these will work fine in regular history but give wonky values in
> grouped.

Oh wups, this is meant to be item.index.

Added more tests to catch all this stuff.

https://codereview.chromium.org/2255033002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_metrics_test.js (right):

https://codereview.chromium.org/2255033002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_metrics_test.js:130:
MockInteractions.tap(app.$.toolbar.$$('#delete-button'));
On 2016/08/18 04:14:34, tsergeant wrote:
> You can check that RemoveSelected is recorded here.

Done.

[tsergeant, 2016-08-22 00:23:40]

I've got a possible idea for how to deal with the removal index histogram. I'll
chat to you in person to explain.

Also, I wonder how useful the index-based metrics are for grouped history. It's
possible for something to have an index of >100, but be displayed right at the
top of the page...

https://codereview.chromium.org/2255033002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/list_container.js (right):

https://codereview.chromium.org/2255033002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:227:
browserService.deleteItems([itemData.item])
You'll need to refresh indexes here.

This is a big bummer, I wish there was a nice way to do this automatically. I'm
not keen on doing a bunch of upkeep work to get a metric to record.

https://codereview.chromium.org/2255033002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_metrics_test.js (right):

https://codereview.chromium.org/2255033002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_metrics_test.js:87:
test('history-list', function() {
Having these here is okay for now, but we should be open to integrating them
with the rest of the tests if they prove to be a maintenance burden.

[calamity, 2016-08-23 04:52:25]

https://codereview.chromium.org/2255033002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_history/list_container.js (right):

https://codereview.chromium.org/2255033002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_history/list_container.js:227:
browserService.deleteItems([itemData.item])
On 2016/08/22 00:23:40, tsergeant wrote:
> You'll need to refresh indexes here.
> 
> This is a big bummer, I wish there was a nice way to do this automatically.
I'm
> not keen on doing a bunch of upkeep work to get a metric to record.

N/A

https://codereview.chromium.org/2255033002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_history/history_metrics_test.js (right):

https://codereview.chromium.org/2255033002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_history/history_metrics_test.js:87:
test('history-list', function() {
On 2016/08/22 00:23:40, tsergeant wrote:
> Having these here is okay for now, but we should be open to integrating them
> with the rest of the tests if they prove to be a maintenance burden.

My intuition is that there would be a larger maintenance burden if we munged
this into normal tests since random actions high up in integration tests etc
would likely tickle various histograms. We'll play it by ear I suppose.

[tsergeant, 2016-08-23 07:38:41]

lgtm

You document the change in meaning for the removal histogram somewhere so that
we can let the owner of the metric (rpop@) know.

https://codereview.chromium.org/2255033002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/compiled_resources2.gyp (right):

https://codereview.chromium.org/2255033002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/compiled_resources2.gyp:68: 'constants',
Nit: Alphabetical order

[tsergeant, 2016-08-23 07:41:33]

On 2016/08/23 07:38:41, tsergeant wrote:
> lgtm
> 
> You document the change in meaning for the removal histogram somewhere so that
> we can let the owner of the metric (rpop@) know.
> 

Uh, "you should document"

[calamity, 2016-08-24 01:56:40]

Description was changed from

==========
[MD History] Copy stats from the old history page.

This CL reimplements the UMA actions and histograms that were in the old
history page.

BUG=614609
NOPRESUBMIT = true # crisper.js
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD History] Copy stats from the old history page.

This CL reimplements the UMA actions and histograms that were in the old
history page.

The HistoryPage.RemoveEntryPosition histogram will only register
removals via the 'Remove from history' menu button.

BUG=614609
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2016-08-24 01:57:41]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 01:58:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2016-08-24 02:01:43]

https://codereview.chromium.org/2255033002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_history/compiled_resources2.gyp (right):

https://codereview.chromium.org/2255033002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_history/compiled_resources2.gyp:68: 'constants',
On 2016/08/23 07:38:41, tsergeant wrote:
> Nit: Alphabetical order

Done.

[calamity, 2016-08-24 02:54:55]

The CQ bit was checked by calamity@chromium.org

[calamity, 2016-08-24 02:54:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2255033002/#ps120001
(title: "learn_the_alphabet")

[commit-bot: I haz the power, 2016-08-24 02:55:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 05:18:26]

Description was changed from

==========
[MD History] Copy stats from the old history page.

This CL reimplements the UMA actions and histograms that were in the old
history page.

The HistoryPage.RemoveEntryPosition histogram will only register
removals via the 'Remove from history' menu button.

BUG=614609
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD History] Copy stats from the old history page.

This CL reimplements the UMA actions and histograms that were in the old
history page.

The HistoryPage.RemoveEntryPosition histogram will only register
removals via the 'Remove from history' menu button.

BUG=614609
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-24 05:18:27]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-24 05:21:39]

Description was changed from

==========
[MD History] Copy stats from the old history page.

This CL reimplements the UMA actions and histograms that were in the old
history page.

The HistoryPage.RemoveEntryPosition histogram will only register
removals via the 'Remove from history' menu button.

BUG=614609
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD History] Copy stats from the old history page.

This CL reimplements the UMA actions and histograms that were in the old
history page.

The HistoryPage.RemoveEntryPosition histogram will only register
removals via the 'Remove from history' menu button.

BUG=614609
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/8a0c47c2aa6dfb3daca92636e4d8d45e7ab0d647
Cr-Commit-Position: refs/heads/master@{#413986}
==========

[commit-bot: I haz the power, 2016-08-24 05:21:40]

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