[NTP4] Track the position of each app that's launched.

[NTP4] Track the position of each app that's launched.

BUG=109210
R=kmadhusu@chromium.org,estade@chromium.org
TEST=about:histograms and data after a while.

[Dan Beam, 2012-01-19 08:18:12]

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:510: ['NewTabPage.AppClickIndex',
index + 1, 100]);
^ I just put 100 as a semi-arbitrary number, though in reality I'll bet nobody
(but possibly QA) has more than 100 apps on a page.

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/most_visited_page.js:144: ['NTP_MostVisited',
this.index, THUMBNAIL_COUNT]);
just cleanup

[kmadhusu, 2012-01-19 18:29:02]

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:504: * Record app position "clicked"
(or when a user presses Enter on an app
nit: "Enter"

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:506: * @param {number} index The
clicked index to record.
nit: @private

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:510: ['NewTabPage.AppClickIndex',
index + 1, 100]);
On 2012/01/19 08:18:14, Dan Beam wrote:
> ^ I just put 100 as a semi-arbitrary number, though in reality I'll bet nobody
> (but possibly QA) has more than 100 apps on a page.

Add this comment in the code for future references.

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:510: ['NewTabPage.AppClickIndex',
index + 1, 100]);
Can you declare a constant for this magic number?

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:510: ['NewTabPage.AppClickIndex',
index + 1, 100]);
Why do you want to record the app clicked position? (We just want to track all
the clicks right, can you just do the following:

chrome.send('NewTabPageAppClickedRecordInHistogram');

and in C++
 just do:

++current_app_click_count_;
UMA_HISTOGRAM_COUNTS("NewTabPage.AppClicked",                     
current_app_click_count_);

)

[Dan Beam, 2012-01-19 19:47:24]

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:504: * Record app position "clicked"
(or when a user presses Enter on an app
On 2012/01/19 18:29:03, kmadhusu wrote:
> nit: "Enter"

Why double quotes (from style guide: Use single quotes instead of double quotes
for all strings.,
http://www.chromium.org/developers/web-development-style-guide) and why quotes
at all?

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:506: * @param {number} index The
clicked index to record.
On 2012/01/19 18:29:03, kmadhusu wrote:
> nit: @private

Done.

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:510: ['NewTabPage.AppClickIndex',
index + 1, 100]);
On 2012/01/19 18:29:03, kmadhusu wrote:
> On 2012/01/19 08:18:14, Dan Beam wrote:
> > ^ I just put 100 as a semi-arbitrary number, though in reality I'll bet
nobody
> > (but possibly QA) has more than 100 apps on a page.
> 
> Add this comment in the code for future references.

Done.

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:510: ['NewTabPage.AppClickIndex',
index + 1, 100]);
On 2012/01/19 18:29:03, kmadhusu wrote:
> Can you declare a constant for this magic number?

Done.

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:510: ['NewTabPage.AppClickIndex',
index + 1, 100]);
On 2012/01/19 18:29:03, kmadhusu wrote:
> Why do you want to record the app clicked position? (We just want to track all
> the clicks right, can you just do the following:
> 
> chrome.send('NewTabPageAppClickedRecordInHistogram');
> 
> and in C++
>  just do:
> 
> ++current_app_click_count_;
> UMA_HISTOGRAM_COUNTS("NewTabPage.AppClicked",                     
> current_app_click_count_);
> 
> )

From the bug:
Ideally segmented by position (similar to the NewTabPage.MostVisited histogram).
(http://crbug.com/106575)

Later in the bug:
Re 4, I'd like to understand the effect of icon position on CTR.  Is there a way
to add clicks by index position just as there is for "Most Visited"
(http://crbug.com/106575#c7)

[kmadhusu, 2012-01-19 20:27:20]

lgtm

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:504: * Record app position "clicked"
(or when a user presses Enter on an app
On 2012/01/19 19:47:24, Dan Beam wrote:
> On 2012/01/19 18:29:03, kmadhusu wrote:
> > nit: "Enter"
> 
> Why double quotes (from style guide: Use single quotes instead of double
quotes
> for all strings.,
> http://www.chromium.org/developers/web-development-style-guide) and why quotes
> at all?

I thought it would be a good idea to emphasis on "Enter" key. That's why I
suggested quotes. Single or double quotes (whatever matches our coding style
guidelines).

[Dan Beam, 2012-01-19 22:29:15]

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://chromiumcodereview.appspot.com/9254023/diff/1/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:504: * Record app position "clicked"
(or when a user presses Enter on an app
On 2012/01/19 20:27:21, kmadhusu wrote:
> On 2012/01/19 19:47:24, Dan Beam wrote:
> > On 2012/01/19 18:29:03, kmadhusu wrote:
> > > nit: "Enter"
> > 
> > Why double quotes (from style guide: Use single quotes instead of double
> quotes
> > for all strings.,
> > http://www.chromium.org/developers/web-development-style-guide) and why
quotes
> > at all?
> 
> I thought it would be a good idea to emphasis on "Enter" key. That's why I
> suggested quotes. Single or double quotes (whatever matches our coding style
> guidelines).

Done. (was just wondering why, :)

[Dan Beam, 2012-01-20 01:54:59]

due to some recent revelations, this CL is waiting on clarification from either:

 - rahulrc@ whether % of x/y is OK, in which case we make the logged measurement
% of app's x position / number of apps in a row (and same for columns)
 - jar@ if there's a way to log graph data with UMA

[Dan Beam, 2012-01-20 06:56:06]

>  - rahulrc@ whether % of x/y is OK, in which case we make the logged
measurement

And just got reply from rahulrc@ that this is OK.  Will do tomorrow.

[Dan Beam, 2012-01-21 00:33:53]

+estade for chrome/browser/resources/ntp4/

[Evan Stade, 2012-01-21 00:44:23]

https://chromiumcodereview.appspot.com/9254023/diff/12009/chrome/browser/reso...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://chromiumcodereview.appspot.com/9254023/diff/12009/chrome/browser/reso...
chrome/browser/resources/ntp4/apps_page.js:495:
this.recordAppClickIndex_(rect.left + rect.width/2,
spaces around operators

https://chromiumcodereview.appspot.com/9254023/diff/12009/chrome/browser/reso...
chrome/browser/resources/ntp4/apps_page.js:508: * @param {number} x The client X
at which the app was clicked.
you don't specify what units x is in, but it looks like pixels. Why are you
tracking pixels?

[kmadhusu, 2012-01-21 00:53:35]

Deferring the UMA related code review to estade@.

Thanks.

[Dan Beam, 2012-01-21 01:49:23]

https://chromiumcodereview.appspot.com/9254023/diff/12009/chrome/browser/reso...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://chromiumcodereview.appspot.com/9254023/diff/12009/chrome/browser/reso...
chrome/browser/resources/ntp4/apps_page.js:508: * @param {number} x The client X
at which the app was clicked.
On 2012/01/21 00:44:23, Evan Stade wrote:
> you don't specify what units x is in, but it looks like pixels.

Done.

> Why are you tracking pixels?

So we're able to track where people click more accurately than just which tile. 
The goal is to understand how position affects the NTP's CTR, so it didn't seem
to hurt (nor add much code complexity, as we'd still have to do these tricks
with app indices to get % x/y measurements).

[Dan Beam, 2012-01-21 01:49:49]

whoops, this also

https://chromiumcodereview.appspot.com/9254023/diff/12009/chrome/browser/reso...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://chromiumcodereview.appspot.com/9254023/diff/12009/chrome/browser/reso...
chrome/browser/resources/ntp4/apps_page.js:495:
this.recordAppClickIndex_(rect.left + rect.width/2,
On 2012/01/21 00:44:23, Evan Stade wrote:
> spaces around operators

Done.