Record NewTabPage.MostVisited histogram on Android.

Record NewTabPage.MostVisited histogram on Android.

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

[newt (away), 2013-11-14 04:03:56]

+tedchoc for both files
+dbeam for new_tab_ui.cc

[Dan Beam, 2013-11-14 04:44:38]

lgtm w/nit

https://codereview.chromium.org/70083009/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_android/ntp_android.js (right):

https://codereview.chromium.org/70083009/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_android/ntp_android.js:948: });
nit: I'd strip off this duplicate/volatile data and just query the DOM, imo:

  /**
   * @param {!HTMLElement} el The most visited element.
   * @return {number} The most visited index of item.
   */
  function mouseVisitedIndex(el) {
    // If they're siblings.
    return Array.prototype.indexOf.call(el.parentNode.childNodes, el);
  }

or something like:

    var all = document.querySelectorAll('.most-visited');
    return Array.prototype.indexOf.call(all, el);

if they're not siblings.  i think this is less error prone.  if this is *really*
bad for performance on mobile, eh, maybe ignore.

https://codereview.chromium.org/70083009/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_android/ntp_android.js:1213: if (contextMenuItem !=
null) {
nit: if (contextMenuItem) {

[newt (away), 2013-11-14 18:44:01]

https://codereview.chromium.org/70083009/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_android/ntp_android.js (right):

https://codereview.chromium.org/70083009/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_android/ntp_android.js:948: });
On 2013/11/14 04:44:38, Dan Beam wrote:
> nit: I'd strip off this duplicate/volatile data and just query the DOM, imo:
> 
>   /**
>    * @param {!HTMLElement} el The most visited element.
>    * @return {number} The most visited index of item.
>    */
>   function mouseVisitedIndex(el) {
>     // If they're siblings.
>     return Array.prototype.indexOf.call(el.parentNode.childNodes, el);
>   }
> 
> or something like:
> 
>     var all = document.querySelectorAll('.most-visited');
>     return Array.prototype.indexOf.call(all, el);
> 
> if they're not siblings.  i think this is less error prone.  if this is
*really*
> bad for performance on mobile, eh, maybe ignore.

If it's OK with you, I think I'll leave this as is. I've already verified the
current solution, and since this code will be deleted soon, I'd prefer not to
spend extra time on it.

https://codereview.chromium.org/70083009/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_android/ntp_android.js:1213: if (contextMenuItem !=
null) {
On 2013/11/14 04:44:38, Dan Beam wrote:
> nit: if (contextMenuItem) {

Done.

[Dan Beam, 2013-11-14 19:16:02]

https://codereview.chromium.org/70083009/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_android/ntp_android.js (right):

https://codereview.chromium.org/70083009/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_android/ntp_android.js:948: });
On 2013/11/14 18:44:02, newt wrote:
> On 2013/11/14 04:44:38, Dan Beam wrote:
> > nit: I'd strip off this duplicate/volatile data and just query the DOM, imo:
> > 
> >   /**
> >    * @param {!HTMLElement} el The most visited element.
> >    * @return {number} The most visited index of item.
> >    */
> >   function mouseVisitedIndex(el) {
> >     // If they're siblings.
> >     return Array.prototype.indexOf.call(el.parentNode.childNodes, el);
> >   }
> > 
> > or something like:
> > 
> >     var all = document.querySelectorAll('.most-visited');
> >     return Array.prototype.indexOf.call(all, el);
> > 
> > if they're not siblings.  i think this is less error prone.  if this is
> *really*
> > bad for performance on mobile, eh, maybe ignore.
> 
> If it's OK with you, I think I'll leave this as is. I've already verified the
> current solution, and since this code will be deleted soon, I'd prefer not to
> spend extra time on it.

that's fine.

[newt (away), 2013-11-15 18:04:49]

ted, do you want to take a look?

[Ted C, 2013-11-15 19:21:09]

On 2013/11/15 18:04:49, newt wrote:
> ted, do you want to take a look?

lgtm

[commit-bot: I haz the power, 2013-11-15 19:42:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/70083009/60001

[commit-bot: I haz the power, 2013-11-15 22:19:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/70083009/60001

[commit-bot: I haz the power, 2013-11-16 00:53:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/70083009/60001

[commit-bot: I haz the power, 2013-11-18 08:31:20]

Change committed as 235664