Adds about:user-actions page.

Adds about:user-actions page.

This page listens for all record-user-action events and displays them.
It's useful to help verify that the right user actions get sent at the right time.

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

[Mark P, 2012-11-20 23:01:30]

I know there are lot of files in this review, but everything is short and
straightforward.  It should be easy.

Evan - can you please review all the code besides the css/html/js?

Arv - can you please review the css/html/js?

thanks,
mark

[Evan Stade, 2012-11-20 23:09:57]

lgtm aside from nits

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
File chrome/browser/ui/webui/user_actions/user_actions_ui.cc (right):

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui.cc:22:
html_source->add_resource_path("user_actions.csss", IDR_USER_ACTIONS_CSS);
this looks like too many s's

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
File chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc (right):

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc:22: // No need
to explicitly unregister ourselves--it's done automatically
this comment is unecessary imo

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
File chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h (right):

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h:18: // Sets up
this class as an observer for NOTIFICATION_USER_ACTION.
remove comment

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h:19: explicit
UserActionsUIHandler();
no explicit

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h:20: 
remove newline

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h:24: //
observeUserAction in user_actions.js.
comment should be "NotificationObserver implementation." or similar

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h:31: virtual void
RegisterMessages();
OVERRIDE; comment should be "WebUIMessageHandler implementation." or similar;
should be in public section.

[Mark P, 2012-11-20 23:45:41]

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
File chrome/browser/ui/webui/user_actions/user_actions_ui.cc (right):

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui.cc:22:
html_source->add_resource_path("user_actions.csss", IDR_USER_ACTIONS_CSS);
On 2012/11/20 23:09:57, Evan Stade wrote:
> this looks like too many s's

Interesting.  I wonder why it works.

Fixed.

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
File chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc (right):

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc:22: // No need
to explicitly unregister ourselves--it's done automatically
On 2012/11/20 23:09:57, Evan Stade wrote:
> this comment is unecessary imo

I found the behavior confusing/surprising; I'd rather keep it.

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
File chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h (right):

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h:18: // Sets up
this class as an observer for NOTIFICATION_USER_ACTION.
On 2012/11/20 23:09:57, Evan Stade wrote:
> remove comment

Done.

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h:19: explicit
UserActionsUIHandler();
On 2012/11/20 23:09:57, Evan Stade wrote:
> no explicit

Done.

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h:20: 
On 2012/11/20 23:09:57, Evan Stade wrote:
> remove newline

Done.

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h:24: //
observeUserAction in user_actions.js.
On 2012/11/20 23:09:57, Evan Stade wrote:
> comment should be "NotificationObserver implementation." or similar

Removed entirely.  Putting it in the implementation felt stupid--it's redundant
with the short and easily readable code.

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.h:31: virtual void
RegisterMessages();
On 2012/11/20 23:09:57, Evan Stade wrote:
> OVERRIDE; comment should be "WebUIMessageHandler implementation." or similar;
> should be in public section.

Done.

[Evan Stade, 2012-11-20 23:59:16]

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
File chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc (right):

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc:22: // No need
to explicitly unregister ourselves--it's done automatically
On 2012/11/20 23:45:41, Mark P wrote:
> On 2012/11/20 23:09:57, Evan Stade wrote:
> > this comment is unecessary imo
> 
> I found the behavior confusing/surprising; I'd rather keep it.

this behavior is 99% of the reason registrar_ exists, and its usage is extremely
prevalent in chrome code. There are many instances of scoping classes in chrome
code: scoped_ptr, WeakPtrFactory, PrefChangeRegistrar, to name a few. If we
documented every use of every one of these objects, the readability of the
codebase would be diminished rather than improved.

[Mark P, 2012-11-21 00:18:14]

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
File chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc (right):

https://codereview.chromium.org/11415063/diff/2001/chrome/browser/ui/webui/us...
chrome/browser/ui/webui/user_actions/user_actions_ui_handler.cc:22: // No need
to explicitly unregister ourselves--it's done automatically
On 2012/11/20 23:59:16, Evan Stade wrote:
> On 2012/11/20 23:45:41, Mark P wrote:
> > On 2012/11/20 23:09:57, Evan Stade wrote:
> > > this comment is unecessary imo
> > 
> > I found the behavior confusing/surprising; I'd rather keep it.
> 
> this behavior is 99% of the reason registrar_ exists, and its usage is
extremely
> prevalent in chrome code. There are many instances of scoping classes in
chrome
> code: scoped_ptr, WeakPtrFactory, PrefChangeRegistrar, to name a few. If we
> documented every use of every one of these objects, the readability of the
> codebase would be diminished rather than improved.

Okay.  Good argument.  I haven't look widely through the code base to know how
familiar are most devs with these things.

Done.

[arv (Not doing code reviews), 2012-11-21 15:09:07]

LGTM

https://codereview.chromium.org/11415063/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/user_actions/user_actions.js (right):

https://codereview.chromium.org/11415063/diff/10001/chrome/browser/resources/...
chrome/browser/resources/user_actions/user_actions.js:7: * This is used to debug
user actons recording.  It displays a live
s/acton/action/

https://codereview.chromium.org/11415063/diff/10001/chrome/browser/resources/...
chrome/browser/resources/user_actions/user_actions.js:7: * This is used to debug
user actons recording.  It displays a live
Single space after .

https://codereview.chromium.org/11415063/diff/10001/chrome/browser/resources/...
chrome/browser/resources/user_actions/user_actions.js:11: * The simple object
defined in this javascript file listens from
listens for

https://codereview.chromium.org/11415063/diff/10001/chrome/browser/resources/...
chrome/browser/resources/user_actions/user_actions.js:30: td.textContent = new
Date().getTime() / 1000;  // in seconds since epoch
Date.now()

[Mark P, 2012-11-21 16:05:45]

https://codereview.chromium.org/11415063/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/user_actions/user_actions.js (right):

https://codereview.chromium.org/11415063/diff/10001/chrome/browser/resources/...
chrome/browser/resources/user_actions/user_actions.js:7: * This is used to debug
user actons recording.  It displays a live
On 2012/11/21 15:09:08, arv wrote:
> Single space after .

Done.

https://codereview.chromium.org/11415063/diff/10001/chrome/browser/resources/...
chrome/browser/resources/user_actions/user_actions.js:7: * This is used to debug
user actons recording.  It displays a live
On 2012/11/21 15:09:08, arv wrote:
> s/acton/action/

Done.

https://codereview.chromium.org/11415063/diff/10001/chrome/browser/resources/...
chrome/browser/resources/user_actions/user_actions.js:11: * The simple object
defined in this javascript file listens from
On 2012/11/21 15:09:08, arv wrote:
> listens for

Done.

https://codereview.chromium.org/11415063/diff/10001/chrome/browser/resources/...
chrome/browser/resources/user_actions/user_actions.js:30: td.textContent = new
Date().getTime() / 1000;  // in seconds since epoch
On 2012/11/21 15:09:08, arv wrote:
> Date.now()

Done.

[commit-bot: I haz the power, 2012-11-21 16:06:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/11415063/3014

[commit-bot: I haz the power, 2012-11-21 16:06:10]

Presubmit check for 11415063-3014 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.9s to calculate.

[Mark P, 2012-11-21 16:12:15]

Arv & Evan,

Do either of you have a suggestion on who I should get to rubberstamp this as a
top-level OWNER?

thanks,
mark

[Ilya Sherman, 2012-11-21 20:35:34]

+Nico for chrome/ OWNERship

[Nico, 2012-11-21 20:57:05]

Please file a bug for this and mention it in the bug line (so that there's a
place to discuss if the benefits are worth the code size hit, or potential other
followup discussion)

[Mark P, 2012-11-21 21:34:53]

On 2012/11/21 20:57:05, Nico wrote:
> Please file a bug for this and mention it in the bug line (so that there's a
> place to discuss if the benefits are worth the code size hit, or potential
other
> followup discussion)

Done.  Please see:
https://code.google.com/p/chromium/issues/detail?id=162309

[Nico, 2012-11-21 22:16:55]

lgtm stamp, thanks

[commit-bot: I haz the power, 2012-11-21 22:21:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/11415063/3014

[commit-bot: I haz the power, 2012-11-21 22:51:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/11415063/16

[commit-bot: I haz the power, 2012-11-22 00:47:00]

Change committed as 169176