328069 - collect data about UI (click / keyboard nav) events

Status: RESOLVED FIXED

(Toolkit Graveyard :: Data Collection/Metrics, defect)

Description

[Brian Ryner (not reading)]

Comment 1

[Marria Nazif]

Attachment: checkpointing ui event patch

Comment 2

[Marria Nazif]

Attachment: checkpoing nsUICommandCollector.cpp

no cvs access, so adding files to bug

Comment 3

[Marria Nazif]

Attachment: checkpointing nsUICommandCollector.h

Comment 4

[Marria Nazif]

Attachment: checkpointing jar.mn

Comment 5

[Marria Nazif]

Attachment: checkpointing metrics/content/nsUICommandWhitelist

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 217506 [details]
checkpoing nsUICommandCollector.cpp

A few comments on the parts of this that don't pertain to whitelisting (since that will be replaced by hashing):

>NS_IMETHODIMP
>nsUICommandCollector::Observe(nsISupports *subject,
>                              const char *topic,
>                              const PRUnichar *data)
>{
>  nsCOMPtr<nsIDOMEventTarget> window;
>
>  if (strcmp(topic, "toplevel-window-ready") == 0) {
>    // Attach a capturing command listener to the window
>    window = do_QueryInterface(subject);

Move the declaration of "window" into this scope.

>    NS_ENSURE_STATE(window);
>
>    nsresult rv = window->AddEventListener(NS_LITERAL_STRING("command"),
>                                           this,
>                                           PR_TRUE);
>    NS_ENSURE_SUCCESS(rv, rv);
>    
>    return rv;

Just fall through to the return NS_OK.

>  }
>
>  return NS_OK;
>}
>


>NS_IMETHODIMP
>nsUICommandCollector::HandleEvent(nsIDOMEvent* aEvent)
>{
>  // First check that this is an event type that we expect
>  nsAutoString type;
>  nsresult rv = aEvent->GetType(type);
>  if (type != NS_LITERAL_STRING("command")) {
>
>#ifdef PR_LOGGING
>    MS_LOG(("UICommandCollector: Unexpected event type %s received",
>            type.get()));

This probably doesn't work, you need to convert the string to UTF-8 before it can be logged.

>  // Get the Original Target id - this is the target after text node
>  // retargeting.  If this id is blank it means the target is anonymous
>  // content.
>  nsCOMPtr<nsIDOMNSEvent> nsEvent = do_QueryInterface(aEvent, &rv);
>  NS_ENSURE_SUCCESS(rv, rv);

This is ok, though I'd probably just do NS_ENSURE_STATE(nsEvent), which is equivalent to NS_ENSURE_TRUE(nsEvent, NS_ERROR_UNEXPECTED).  Same applies to some other places where you're really just interested in whether the result is null.

>  // Get the target id - this is the target after all retargeting.
>  // We only really use this if the original target is anonymous content.
>  // In that case, since anonymous content doesn't have an id, we identify
>  // the target by that target id + the anonid of the anonymous content
>  nsCOMPtr<nsIDOMEvent> domEvent = do_QueryInterface(nsEvent, &rv);
>  NS_ENSURE_SUCCESS(rv, rv);

You can just use aEvent here, can't you?

>  // Get the window that this target is in, so that we can log the event
>  // with the appropriate window id.
>  nsCOMPtr<nsIDOMNode> targetNode = do_QueryInterface(target, &rv);
>  NS_ENSURE_SUCCESS(rv, rv);

It's theoretically possible that the event target is not a node.  In particular, this could happen if the target of the event was 'document' or 'window'.  If that happens, I'm not sure this method should really return "failure" (remember that's equivalent to throwing)... unusual extensions could do something like this.

>  // Actually log it
>  nsMetricsService *ms = nsMetricsService::get();
>  NS_ENSURE_STATE(ms);
>  rv = ms->LogEvent(NS_LITERAL_STRING("ui"), properties);
>

I'd check for success here, then your logging statement actually indicates success.

>#ifdef PR_LOGGING
>  MS_LOG(("Successfully logged UI Event"));
>#endif
>
>  return rv;

And then just change this to return NS_OK.

Comment 7

[Brian Ryner (not reading)]

(In reply to comment #6)
> It's theoretically possible that the event target is not a node.  In
> particular, this could happen if the target of the event was 'document' or
> 'window'.  If that happens, I'm not sure this method should really return
> "failure" (remember that's equivalent to throwing)... unusual extensions could
> do something like this.

Small correction: 'document' is a DOMNode.  'window' isn't.

Comment 8

[Marria Nazif]

Attachment: updated UI collector

Updated per bryner's comments.  Also ripped out whitelisting, in favor of hashing target IDs.  Also updated to build as a standalone extension.

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 219617 [details] [diff] [review]
updated UI collector

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ src/nsUICommandCollector.cpp	24 Apr 2006 18:15:43 -0000
>+nsresult
>+nsUICommandCollector::Init()
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIObserverService> obsSvc =
>+    do_GetService("@mozilla.org/observer-service;1");
>+  NS_ENSURE_STATE(obsSvc);
>+
>+  // Listen for newly opened windows, so that we can attach a command event
>+  // listener to each window
>+  rv = obsSvc->AddObserver(this, "domwindowopened", PR_FALSE);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+}

You need to return NS_OK at the end of the function.

>+NS_IMETHODIMP
>+nsUICommandCollector::Observe(nsISupports *subject,
>+                              const char *topic,
>+                              const PRUnichar *data)
>+{
>+  if (strcmp(topic, "domwindowopened") == 0) {
>+    // Attach a capturing command listener to the window

You might want to say briefly why it's capturing and not bubbling.

>+NS_IMETHODIMP
>+nsUICommandCollector::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+  // First check that this is an event type that we expect
>+  nsString type;
>+  nsresult rv = aEvent->GetType(type);

Either don't assign to rv, or check the result for failure (it doesn't matter too much which one).

>+  if (!type.Equals(NS_LITERAL_STRING("command"))) {
>+
>+#ifdef PR_LOGGING
>+    MS_LOG(("UICommandCollector: Unexpected event type %s received",
>+            NS_ConvertUTF16toUTF8(type.get())));

The macro argument is a little backwards.  You want:

NS_ConvertUTF16toUTF8(type).get()

because you want the final result to be a const char*.  It's kind of unfortunate that what you have compiles.  Also, you don't need to #ifdef it -- MS_LOG() is defined to empty if PR_LOGGING is turned off.  Same comment applies to other MS_LOG usage.

>+  // First try looking up the Original Target ID in our whitelist.  In some
>+  // cases this id might be empty (i.e. for anonymous content).  In that
>+  // case, fall back on looking for the Target ID in
>+  // our whitelist.  If we don't find that an ID is in our whitelist, return
>+  // without logging anything.

This comment needs to be updated.

>+  // Fill a property bag with what we want to log
>+  nsRefPtr<nsIWritablePropertyBag2> properties;

It doesn't _really_ matter, but all of the other collectors use nsCOMPtr for this, can you change it to be consistent?  Remove #include "nsAutoPtr.h" when you make this change.

>+  } else {
>+   nsCString hashedOrigId;
>+   nsresult rv = ms->Hash(orig_id, hashedOrigId);
>+   NS_ENSURE_SUCCESS(rv, rv);

So, I'm a little confused about why you use separate variables and separate codepaths for logging the "id" based on whether we're logging "anonid".  Can't you just always log the id the same way, and then also log the anonid if you need to?

>+  // Actually log it
>+  rv = ms->LogEvent(NS_LITERAL_STRING("ui"), properties);

The event name should be uielement.

Comment 10

[Marria Nazif]

(In reply to comment #9)

> 
> >+  } else {
> >+   nsCString hashedOrigId;
> >+   nsresult rv = ms->Hash(orig_id, hashedOrigId);
> >+   NS_ENSURE_SUCCESS(rv, rv);
> 
> So, I'm a little confused about why you use separate variables and separate
> codepaths for logging the "id" based on whether we're logging "anonid".  Can't
> you just always log the id the same way, and then also log the anonid if you
> need to?

Actually the kind of id we log is different depending on whether we're logging anonid or not.  First we try to log the _Original_ Target ID (target after text retargeting only) - if that is empty we log the Target ID (target after all retargeting) plus the anonid.  In that second case we need the target ID to distinguish between the same anonid used under different targets.  I've updated the comments to make this more clear.  (Note that I'm copying Original Target and Target terminology from the API :-))

> >+  // Actually log it
> >+  rv = ms->LogEvent(NS_LITERAL_STRING("ui"), properties);
> 
> The event name should be uielement.
> 

updated id to be targetidhash and anonid to be targetanonidhash as well


I have updated the patch according to all of your other comments

Comment 11

[Marria Nazif]

Attachment: updated patch

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 219690 [details] [diff] [review]
updated patch

cancelling review based on some changes we talked about offline

Comment 13

[Marria Nazif]

Attachment: updated patch

Updated to only ever log target id, since original target id = target id for content that is not anonymous.  Also added OnAttach, as per offline discussion.

Comment 14

[Brian Ryner (not reading)]

Comment on attachment 219815 [details] [diff] [review]
updated patch

>--- public/nsIMetricsCollector.idl	25 Apr 2006 04:01:07 -0000	1.1.2.2
>+++ public/nsIMetricsCollector.idl	25 Apr 2006 23:56:13 -0000
>@@ -44,20 +44,27 @@
>  * collector "foo" in namespace "http://www.mozilla.org/metrics",
>  * the contract id
>  * "@mozilla.org/metrics/collector;1?name=http://www.mozilla.org/metrics:foo"
>  * is instantiated (using getSerivce).  The collector is responsible for
>  * calling nsIMetricsService::logEvent() when it has something to log.
>  */
> [scriptable, uuid(b6cba4ad-363a-4780-90fa-9d7cc8115854)]
> interface nsIMetricsCollector : nsISupports
> {
>   /**
>+   * Notification that this collector should be enabled.  The collector
>+   * should register itself for observer and event notifications as
>+   * necessary.
>+   */
>+  void onAttach();
>+

You should change the uuid of the interface whenever you add/remove/change a method.

>--- src/nsMetricsService.cpp	25 Apr 2006 04:02:37 -0000	1.4.2.20
>+++ src/nsMetricsService.cpp	25 Apr 2006 23:56:14 -0000
>@@ -740,20 +740,21 @@ nsMetricsService::EnableCollectors()
>   for (i = 0; i < enabledCollectors.Length(); ++i) {
>     const nsString &name = enabledCollectors[i];
>     if (!mCollectorMap.GetWeak(name)) {
>       nsCString contractID("@mozilla.org/metrics/collector;1?name=");
>       contractID.Append(NS_ConvertUTF16toUTF8(name));
> 
>       nsCOMPtr<nsIMetricsCollector> coll = do_GetService(contractID.get());
>       if (coll) {
>         MS_LOG(("Created collector %s", contractID.get()));
>         mCollectorMap.Put(name, coll);
>+        coll->OnAttach();

It seems like we should call OnAttach before we put the collector into the hash table, and not insert it if OnAttach failed.

nsUICommandCollector.cpp:

>+/* static */
>+PLDHashOperator PR_CALLBACK nsUICommandCollector::RemoveCommandEventListener(
>+const void* aKey, PRUint32 windowID, void* aUserArg)
>+{
>+  const nsIDOMWindow* window = NS_STATIC_CAST(const nsIDOMWindow*, aKey);
>+  nsIDOMWindow* window2 = NS_CONST_CAST(nsIDOMWindow*, window);
>+  nsCOMPtr<nsIDOMEventTarget> windowTarget = do_QueryInterface(window2);
>+  if (!windowTarget) {
>+    MS_LOG(("Error casting domeventtarget"));
>+    return PL_DHASH_NEXT;
>+  }
>+
>+  nsIDOMEventListener* listener = NS_STATIC_CAST(nsIDOMEventListener*,
>+                                                 aUserArg);
>+  if (!listener) {

The cast can't really fail... it would probably be cleaner to just null check aUserArg at the start.

>+  nsresult rv = windowTarget->RemoveEventListener(NS_LITERAL_STRING("command"),
>+                                                  listener, PR_TRUE);
>+  if (!NS_SUCCEEDED(rv)) {

if (NS_FAILED(rv))

Looks good otherwise.

Comment 15

[Brian Ryner (not reading)]

(In reply to comment #14)
> It seems like we should call OnAttach before we put the collector into the hash
> table, and not insert it if OnAttach failed.

Actually, I changed my mind on this.  For consistency, with onDetach/onNewLog, we should just treat this as a notification, not a chance for the collector to cancel itself.

Comment 16

[Brian Ryner (not reading)]

Ok, checked in on trunk and branch.

I fixed a few other small things I should have caught in the review, hope that's ok.  Here's a list:

- removed now-unused nsUICommandCollector::sInstance
- removed unused Init() declarations from nsUICommandCollector.h and nsWindowCollector.h
- removed unneeded #include nsBaseHashtable.h in nsUICommandCollector.h
- changed argument names to uniformly not be prefixed with "a" for consistency with the rest of the extension
- got rid of the temporary "windows" variable in nsUICommandCollector::OnDetach, it's more readable to just call ms->WindowMap().EnumerateRead(...)