ntp4: record app launch source

ntp4: record app launch source

dispense with anchor pings (these are limiting because you have to use anchor elements) and just use chrome.send apis.

Also note that ntp3 is broken: NTP_RECENTLY_CLOSED app launches will never be recorded because preventDefault prevents the ping from being sent.

BUG=none
TEST=manual

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

[Rick Byers, 2011-08-03 14:41:11]

http://codereview.chromium.org/7550015/diff/1/chrome/browser/resources/ntp4/a...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7550015/diff/1/chrome/browser/resources/ntp4/a...
chrome/browser/resources/ntp4/apps_page.js:197:
appSpan.addEventListener('click', this.onClick_.bind(this));
Why add a handler to both the img and span.  Would it be equivalent to put just
a single handler on the appContents?

http://codereview.chromium.org/7550015/diff/1/chrome/browser/resources/ntp4/m...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

http://codereview.chromium.org/7550015/diff/1/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.js:124:
chrome.send('recordAppLaunchByURL',
Is replacing the ping with a click handler really equivalent?  I thought ping
was designed to handle a broader set of situations - eg. right click and "open
in new tab".  Too bad there's no "on anchor activate" DOM event - that's really
what you want I think.  I suppose we have to decide whether we'd rather use
pings, switch the anchors to plain divs, or miss out on some launches.

http://codereview.chromium.org/7550015/diff/1/chrome/browser/ui/webui/ntp/app...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7550015/diff/1/chrome/browser/ui/webui/ntp/app...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:196: 
Remove extra blank line

http://codereview.chromium.org/7550015/diff/1/chrome/browser/ui/webui/ntp/app...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:709:
CHECK(args->GetString(1, &source));
Curious why you're taking a string argument here and adding a TODO saying to
make it an int.  Isn't it trivial to just make it an int now (just need a cast
below instead of calling ParseLaunchSource)?

http://codereview.chromium.org/7550015/diff/1/chrome/browser/ui/webui/ntp/app...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

http://codereview.chromium.org/7550015/diff/1/chrome/browser/ui/webui/ntp/app...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:105: // Callback for the
"recordAppLaunchByURL" message.
In my opinion, these function declaration comments don't really conform to the
Google style guide which requires that they "describe what the function does and
how to use it".  I'd suggest adding something like:
Given a URL  and an AppLaunchBucket (encoded as a string), records a launch of
the application with the specified URL, if any (otherwise does nothing).  

I'd also love to know here (if you can determine the answer easily) to what
extent the URL needs to match.  Do hash values matter in the match?  Eg., if I'm
using an application that has set a fragment identifier, then close it, then
re-open from the recently closed list, do I get a launch recorded?  What about
query parameters.  I know of some Google apps in particular that both redirect
to urls with different query params and also set the location hash.

Personally I believe we'd benefit from a better specified contract between the
C++ and JS portions of WebUI code (I've spent a bunch of time myself trying to
reverse engineer these contracts - in retrospect I probably should have updated
comments as I did it).

[Evan Stade, 2011-08-03 22:11:22]

http://codereview.chromium.org/7550015/diff/1/chrome/browser/resources/ntp4/a...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7550015/diff/1/chrome/browser/resources/ntp4/a...
chrome/browser/resources/ntp4/apps_page.js:197:
appSpan.addEventListener('click', this.onClick_.bind(this));
On 2011/08/03 14:41:11, Rick Byers wrote:
> Why add a handler to both the img and span.  Would it be equivalent to put
just
> a single handler on the appContents?

the clickable contour is a little different

http://codereview.chromium.org/7550015/diff/1/chrome/browser/resources/ntp4/m...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

http://codereview.chromium.org/7550015/diff/1/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.js:124:
chrome.send('recordAppLaunchByURL',
On 2011/08/03 14:41:11, Rick Byers wrote:
> Is replacing the ping with a click handler really equivalent?  I thought ping
> was designed to handle a broader set of situations - eg. right click and "open
> in new tab".  Too bad there's no "on anchor activate" DOM event - that's
really
> what you want I think.  I suppose we have to decide whether we'd rather use
> pings, switch the anchors to plain divs, or miss out on some launches.

I would have thought you were right, but ping doesn't seem to work that way
(open in new tab/window from the context menu doesn't send a ping).

Also, right click + open URL in new tab kind of sucks for recently closed,
because it doesn't restore session state (e.g. back/forward history).

http://codereview.chromium.org/7550015/diff/1/chrome/browser/ui/webui/ntp/app...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7550015/diff/1/chrome/browser/ui/webui/ntp/app...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:196: 
On 2011/08/03 14:41:11, Rick Byers wrote:
> Remove extra blank line

Done.

http://codereview.chromium.org/7550015/diff/1/chrome/browser/ui/webui/ntp/app...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:709:
CHECK(args->GetString(1, &source));
On 2011/08/03 14:41:11, Rick Byers wrote:
> Curious why you're taking a string argument here and adding a TODO saying to
> make it an int.  Isn't it trivial to just make it an int now (just need a cast
> below instead of calling ParseLaunchSource)?

Done.

http://codereview.chromium.org/7550015/diff/1/chrome/browser/ui/webui/ntp/app...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

http://codereview.chromium.org/7550015/diff/1/chrome/browser/ui/webui/ntp/app...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:105: // Callback for the
"recordAppLaunchByURL" message.
On 2011/08/03 14:41:11, Rick Byers wrote:
> In my opinion, these function declaration comments don't really conform to the
> Google style guide which requires that they "describe what the function does
and
> how to use it".  I'd suggest adding something like:
> Given a URL  and an AppLaunchBucket (encoded as a string), records a launch of
> the application with the specified URL, if any (otherwise does nothing).  

done

> 
> I'd also love to know here (if you can determine the answer easily) to what
> extent the URL needs to match.  Do hash values matter in the match?  Eg., if
I'm
> using an application that has set a fragment identifier, then close it, then
> re-open from the recently closed list, do I get a launch recorded?  What about
> query parameters.  I know of some Google apps in particular that both redirect
> to urls with different query params and also set the location hash.

the app for a given url is determined by the extensions system, I don't know the
details.


> Personally I believe we'd benefit from a better specified contract between the
> C++ and JS portions of WebUI code (I've spent a bunch of time myself trying to
> reverse engineer these contracts - in retrospect I probably should have
updated
> comments as I did it).

[Rick Byers, 2011-08-04 00:32:20]

LGTM with nit

http://codereview.chromium.org/7550015/diff/1/chrome/browser/resources/ntp4/m...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

http://codereview.chromium.org/7550015/diff/1/chrome/browser/resources/ntp4/m...
chrome/browser/resources/ntp4/most_visited_page.js:124:
chrome.send('recordAppLaunchByURL',
On 2011/08/03 22:11:22, Evan Stade wrote:
> On 2011/08/03 14:41:11, Rick Byers wrote:
> > Is replacing the ping with a click handler really equivalent?  I thought
ping
> > was designed to handle a broader set of situations - eg. right click and
"open
> > in new tab".  Too bad there's no "on anchor activate" DOM event - that's
> really
> > what you want I think.  I suppose we have to decide whether we'd rather use
> > pings, switch the anchors to plain divs, or miss out on some launches.
> 
> I would have thought you were right, but ping doesn't seem to work that way
> (open in new tab/window from the context menu doesn't send a ping).

That sucks - seems like a bug according to the spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#hyperl....
 Anyway, it's your call, but I'd prefer if you at least left a comment pointing
out that the app launches are only recorded for regular clicks.  It might be
worth getting Arv's input on this when he's back.

> Also, right click + open URL in new tab kind of sucks for recently closed,
> because it doesn't restore session state (e.g. back/forward history).

http://codereview.chromium.org/7550015/diff/5003/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

http://codereview.chromium.org/7550015/diff/5003/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:105: // Callback for the
"recordAppLaunchByURL" message. Takes a URL and a launch
nit: s/URL/escaped URL
I never would have guessed this was supposed to be URI-escaped, seems strange
but OK as long as it's clear that's the contract.

[Evan Stade, 2011-08-04 03:10:25]

comments updated