Adds the MostVisitedAction stat. This stat will provide a baseline to compare

Adds the MostVisitedAction stat. This stat will provide a baseline to compare
to for the SuggestedSitesAction stat.

BUG=118427
TEST=Go to the NTP, and switch between panes (e.g., "Apps" and "Most Visited"). Then, click on a link in the "Most Visited" pane. Open a new tab. If the Most Visited pane is not selected, select it, close the tab and open a new one. Type a URL in the Omnibox (and press enter). Open a new tab, and close it. Then, go to about:histograms, and verify that there are data points in 4 different bins for the NewTabPage.MostVisitedAction stat.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134205

[beaudoin, 2012-04-23 16:46:51]

Drive-by review.

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

https://chromiumcodereview.appspot.com/10182006/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp4/most_visited_page.js:306:
this.addEventListener('cardselected', this.handleCardSelected_);
handleCard* methods will not have |this| correctly defined. Either bind them to
|this| or use static methods. (I'd recommend the latter since they don't refer
to |this|.)

https://chromiumcodereview.appspot.com/10182006/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp4/most_visited_page.js:479:
document.addEventListener('ntploaded', ntp.MostVisitedPage.prototype.onLoaded);
The onLoaded method cannot have access to a valid |this| so use a static method.

https://chromiumcodereview.appspot.com/10182006/diff/1009/chrome/browser/reso...
File chrome/browser/resources/ntp4/suggestions_page.js (right):

https://chromiumcodereview.appspot.com/10182006/diff/1009/chrome/browser/reso...
chrome/browser/resources/ntp4/suggestions_page.js:303:
this.addEventListener('cardselected', this.handleCardSelected_);
I see it's being done this way here. I think having a method with |this| badly
defined is error prone and should be revisited. Check with the person who did
this.

https://chromiumcodereview.appspot.com/10182006/diff/1009/chrome/browser/reso...
chrome/browser/resources/ntp4/suggestions_page.js:476:
document.addEventListener('ntploaded', ntp.SuggestionsPage.prototype.onLoaded);
Same comment.

[macourteau, 2012-04-23 19:49:41]

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

https://chromiumcodereview.appspot.com/10182006/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp4/most_visited_page.js:306:
this.addEventListener('cardselected', this.handleCardSelected_);
On 2012/04/23 16:46:52, beaudoin wrote:
> handleCard* methods will not have |this| correctly defined. Either bind them
to
> |this| or use static methods. (I'd recommend the latter since they don't refer
> to |this|.)

We've discussed this (no pun intended) -- 'this' appears to be set properly in
the method calls (also this is also how it seems to be used elsewhere (e.g.
above with handleClick_)).

https://chromiumcodereview.appspot.com/10182006/diff/1/chrome/browser/resourc...
chrome/browser/resources/ntp4/most_visited_page.js:479:
document.addEventListener('ntploaded', ntp.MostVisitedPage.prototype.onLoaded);
On 2012/04/23 16:46:52, beaudoin wrote:
> The onLoaded method cannot have access to a valid |this| so use a static
method.

Done.

https://chromiumcodereview.appspot.com/10182006/diff/1009/chrome/browser/reso...
File chrome/browser/resources/ntp4/suggestions_page.js (right):

https://chromiumcodereview.appspot.com/10182006/diff/1009/chrome/browser/reso...
chrome/browser/resources/ntp4/suggestions_page.js:303:
this.addEventListener('cardselected', this.handleCardSelected_);
On 2012/04/23 16:46:52, beaudoin wrote:
> I see it's being done this way here. I think having a method with |this| badly
> defined is error prone and should be revisited. Check with the person who did
> this.

(see other comment).

https://chromiumcodereview.appspot.com/10182006/diff/1009/chrome/browser/reso...
chrome/browser/resources/ntp4/suggestions_page.js:476:
document.addEventListener('ntploaded', ntp.SuggestionsPage.prototype.onLoaded);
On 2012/04/23 16:46:52, beaudoin wrote:
> Same comment.

Done.

[macourteau, 2012-04-23 19:55:36]

Hi Evan,

Can you review this? This is adding the NewTabPage.MostVisitedAction stat (as
you suggested, so we can have a baseline to compare the
NewTabPage.SuggestedSitesAction stat to), and also addresses a TODO that I still
had in my code.

Thanks!
- macourteau

[Evan Stade, 2012-04-24 20:29:44]

https://chromiumcodereview.appspot.com/10182006/diff/12001/chrome/browser/res...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://chromiumcodereview.appspot.com/10182006/diff/12001/chrome/browser/res...
chrome/browser/resources/ntp4/new_tab.js:234: cr.dispatchSimpleEvent(document,
'ntploaded', true, true);
ntpLoaded

https://chromiumcodereview.appspot.com/10182006/diff/12001/chrome/browser/res...
File chrome/browser/resources/ntp4/suggestions_page.js (right):

https://chromiumcodereview.appspot.com/10182006/diff/12001/chrome/browser/res...
chrome/browser/resources/ntp4/suggestions_page.js:349: if
(!document.documentElement.classList.contains('starting-up')) {
no curlies

https://chromiumcodereview.appspot.com/10182006/diff/12001/chrome/browser/res...
chrome/browser/resources/ntp4/suggestions_page.js:386: * Executed once the NTP
has loaded. Checks if the Suggested pane is
this comment doesn't say what it does if the suggested pane is or isn't shown

https://chromiumcodereview.appspot.com/10182006/diff/16001/chrome/browser/res...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

https://chromiumcodereview.appspot.com/10182006/diff/16001/chrome/browser/res...
chrome/browser/resources/ntp4/most_visited_page.js:14: var NtpFollowAction = {
this doesn't need to be defined twice, put it in new_tab.js and export it

https://chromiumcodereview.appspot.com/10182006/diff/16001/chrome/browser/ui/...
File chrome/browser/ui/webui/ntp/ntp_stats.h (right):

https://chromiumcodereview.appspot.com/10182006/diff/16001/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/ntp_stats.h:9: // Those values are defined in
histograms.xml. These represent the action that
"Those values"?

[macourteau, 2012-04-26 19:10:13]

https://chromiumcodereview.appspot.com/10182006/diff/12001/chrome/browser/res...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://chromiumcodereview.appspot.com/10182006/diff/12001/chrome/browser/res...
chrome/browser/resources/ntp4/new_tab.js:234: cr.dispatchSimpleEvent(document,
'ntploaded', true, true);
On 2012/04/24 20:29:44, Evan Stade wrote:
> ntpLoaded

Done.

https://chromiumcodereview.appspot.com/10182006/diff/12001/chrome/browser/res...
File chrome/browser/resources/ntp4/suggestions_page.js (right):

https://chromiumcodereview.appspot.com/10182006/diff/12001/chrome/browser/res...
chrome/browser/resources/ntp4/suggestions_page.js:349: if
(!document.documentElement.classList.contains('starting-up')) {
On 2012/04/24 20:29:44, Evan Stade wrote:
> no curlies

Done.

https://chromiumcodereview.appspot.com/10182006/diff/12001/chrome/browser/res...
chrome/browser/resources/ntp4/suggestions_page.js:386: * Executed once the NTP
has loaded. Checks if the Suggested pane is
On 2012/04/24 20:29:44, Evan Stade wrote:
> this comment doesn't say what it does if the suggested pane is or isn't shown

Done.

https://chromiumcodereview.appspot.com/10182006/diff/16001/chrome/browser/res...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

https://chromiumcodereview.appspot.com/10182006/diff/16001/chrome/browser/res...
chrome/browser/resources/ntp4/most_visited_page.js:14: var NtpFollowAction = {
On 2012/04/24 20:29:44, Evan Stade wrote:
> this doesn't need to be defined twice, put it in new_tab.js and export it

Done.

https://chromiumcodereview.appspot.com/10182006/diff/16001/chrome/browser/ui/...
File chrome/browser/ui/webui/ntp/ntp_stats.h (right):

https://chromiumcodereview.appspot.com/10182006/diff/16001/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/ntp_stats.h:9: // Those values are defined in
histograms.xml. These represent the action that
On 2012/04/24 20:29:44, Evan Stade wrote:
> "Those values"?

Done.

[Evan Stade, 2012-04-26 21:13:05]

lgtm

http://codereview.chromium.org/10182006/diff/35002/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

http://codereview.chromium.org/10182006/diff/35002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/most_visited_page.js:383: if (ntp.getCardSlider()
&& ntp.getCardSlider().currentCardValue) {
merge the two if conditions into one if

http://codereview.chromium.org/10182006/diff/35002/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/10182006/diff/35002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/new_tab.js:633: NtpFollowAction: NtpFollowAction
alphabetical (case insensitive)

[macourteau, 2012-04-26 21:51:33]

http://codereview.chromium.org/10182006/diff/35002/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

http://codereview.chromium.org/10182006/diff/35002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/most_visited_page.js:383: if (ntp.getCardSlider()
&& ntp.getCardSlider().currentCardValue) {
On 2012/04/26 21:13:05, Evan Stade wrote:
> merge the two if conditions into one if

Done.

http://codereview.chromium.org/10182006/diff/35002/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/10182006/diff/35002/chrome/browser/resources/n...
chrome/browser/resources/ntp4/new_tab.js:633: NtpFollowAction: NtpFollowAction
On 2012/04/26 21:13:05, Evan Stade wrote:
> alphabetical (case insensitive)

Done.

[commit-bot: I haz the power, 2012-04-26 21:54:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/macourteau@chromium.org/10182006/31006

[Dan Beam, 2012-04-27 00:51:49]

drive-by: I would seem useful to create a static utility method like
ntp.isLoading() (like I swear I've creating previously but perhaps never
committed?) to check for the .loading class on document.documentElement rather
than checking for this everywhere...

[commit-bot: I haz the power, 2012-04-27 01:39:08]

Change committed as 134205