347305 - make subframe loads easier to track

Status: RESOLVED FIXED

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

Description

[Brian Ryner (not reading)]

Right now it's possible, but not easy, to determine whether a document load/destroy event corresponds to a frame/iframe -- you need to locate the create event for the window, then locate the create event for its parent window, and check whether that is chrome.  We can make subframe identification a lot more straightforward by just using a boolean attribute on the <document> events.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[Brian Ryner (not reading)]

Attachment: same thing for window events

Comment 3

[Darin Fisher]

Comment on attachment 232074 [details] [diff] [review]
patch

>Index: extensions/metrics/src/nsLoadCollector.cpp

>+      // Consider the load to be a subframe if the docshell is not chrome,
>+      // and doesn't have a sameTypeParent.
>+      PRBool subframe = PR_FALSE;
>+      nsCOMPtr<nsIDocShellTreeItem> treeItem = do_QueryInterface(docShell);
>+      if (treeItem) {
>+        PRInt32 itemType;
>+        treeItem->GetItemType(&itemType);
>+        if (itemType == nsIDocShellTreeItem::typeContent) {
>+          nsCOMPtr<nsIDocShellTreeItem> parent;
>+          treeItem->GetSameTypeParent(getter_AddRefs(parent));
>+          if (parent) {
>+            subframe = PR_TRUE;
>+            rv = props->SetPropertyAsBool(NS_LITERAL_STRING("subframe"),
>+                                          PR_TRUE);
>+            NS_ENSURE_SUCCESS(rv, rv);
>+          }
>+        }
>+      }

Is the comment correct?  The code seems to set subframe to true when
the sameTypeParent property is non-null.  Wouldn't that imply that the
docshell _does_ have a sameTypeParent?

r=darin

Comment 4

[Darin Fisher]

Comment on attachment 232079 [details] [diff] [review]
same thing for window events

Perhaps the comment from the other patch should be echoed here?

r=darin

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 232074 [details] [diff] [review]
patch

Thanks, code was right, comment was wrong.

Comment 6

[Brian Ryner (not reading)]

Attachment: refactored

I checked in the first patch as-is, but then decided to go ahead and refactor to reduce code and comment duplication.

Comment 7

[Brett Wilson]

Comment on attachment 232194 [details] [diff] [review]
refactored

>Index: extensions/metrics/src/nsLoadCollector.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/metrics/src/nsLoadCollector.cpp,v
>retrieving revision 1.3.2.18
>diff -u -p -r1.3.2.18 nsLoadCollector.cpp
>--- extensions/metrics/src/nsLoadCollector.cpp	4 Aug 2006 19:23:59 -0000	1.3.2.18
>+++ extensions/metrics/src/nsLoadCollector.cpp	4 Aug 2006 20:09:16 -0000
>@@ -42,7 +42,6 @@
> #endif
> 
> #include "nsLoadCollector.h"
>-#include "nsWindowCollector.h"
> #include "nsMetricsService.h"
> #include "nsCOMPtr.h"
> #include "nsCURILoader.h"
>@@ -359,23 +358,12 @@ nsLoadCollector::OnStateChange(nsIWebPro
>         return NS_ERROR_UNEXPECTED;
>       }
> 
>-      // Consider the load to be a subframe if the docshell is not chrome,
>-      // and has a sameTypeParent.
>-      PRBool subframe = PR_FALSE;
>-      nsCOMPtr<nsIDocShellTreeItem> treeItem = do_QueryInterface(docShell);
>-      if (treeItem) {
>-        PRInt32 itemType;
>-        treeItem->GetItemType(&itemType);
>-        if (itemType == nsIDocShellTreeItem::typeContent) {
>-          nsCOMPtr<nsIDocShellTreeItem> parent;
>-          treeItem->GetSameTypeParent(getter_AddRefs(parent));
>-          if (parent) {
>-            subframe = PR_TRUE;
>-            rv = props->SetPropertyAsBool(NS_LITERAL_STRING("subframe"),
>-                                          PR_TRUE);
>-            NS_ENSURE_SUCCESS(rv, rv);
>-          }
>-        }
>+      nsCOMPtr<nsIDocShellTreeItem> item = do_QueryInterface(docShell);
>+      NS_ENSURE_STATE(item);
>+      PRBool subframe = nsMetricsUtils::IsSubframe(item);
>+      if (subframe) {
>+        rv = props->SetPropertyAsBool(NS_LITERAL_STRING("subframe"), PR_TRUE);
>+        NS_ENSURE_SUCCESS(rv, rv);
>       }
> 
>       nsMetricsService *ms = nsMetricsService::get();
>Index: extensions/metrics/src/nsMetricsService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/metrics/src/nsMetricsService.cpp,v
>retrieving revision 1.4.2.33
>diff -u -p -r1.4.2.33 nsMetricsService.cpp
>--- extensions/metrics/src/nsMetricsService.cpp	12 Jul 2006 16:54:12 -0000	1.4.2.33
>+++ extensions/metrics/src/nsMetricsService.cpp	4 Aug 2006 20:09:16 -0000
>@@ -83,6 +83,7 @@
> #ifndef MOZILLA_1_8_BRANCH
> #include "nsIClassInfoImpl.h"
> #endif
>+#include "nsIDocShellTreeItem.h"
> #include "nsDocShellCID.h"
> #include "nsMemory.h"
> #include "nsIBadCertListener.h"
>@@ -1738,3 +1739,24 @@ nsMetricsUtils::CreateElement(nsIDOMDocu
>   return ownerDoc->CreateElementNS(NS_LITERAL_STRING(NS_METRICS_NAMESPACE),
>                                    tag, element);
> }
>+
>+
>+/* static */ PRBool
>+nsMetricsUtils::IsSubframe(nsIDocShellTreeItem* docShell)
>+{
>+  // Consider the docshell to be a subframe if it's is content, not chrome,
>+  // and has a parent docshell which is also content.
>+  if (!docShell) {
>+    return PR_FALSE;
>+  }
>+
>+  PRInt32 itemType;
>+  docShell->GetItemType(&itemType);
>+  if (itemType != nsIDocShellTreeItem::typeContent) {
>+    return PR_FALSE;
>+  }
>+
>+  nsCOMPtr<nsIDocShellTreeItem> parent;
>+  docShell->GetSameTypeParent(getter_AddRefs(parent));
>+  return (parent != nsnull);
>+}
>Index: extensions/metrics/src/nsMetricsService.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/metrics/src/nsMetricsService.h,v
>retrieving revision 1.3.2.23
>diff -u -p -r1.3.2.23 nsMetricsService.h
>--- extensions/metrics/src/nsMetricsService.h	12 Jul 2006 16:54:12 -0000	1.3.2.23
>+++ extensions/metrics/src/nsMetricsService.h	4 Aug 2006 20:09:16 -0000
>@@ -66,6 +66,7 @@ class nsIDOMWindow;
> class nsIDOMDocument;
> class nsIDOMNode;
> class nsIMetricsCollector;
>+class nsIDocShellTreeItem;
> 
> #ifdef PR_LOGGING
> // Shared log for the metrics service and collectors.
>@@ -309,6 +310,9 @@ public:
>   // ownerDocument and tag.
>   static nsresult CreateElement(nsIDOMDocument *ownerDoc,
>                                 const nsAString &tag, nsIDOMElement **element);
>+
>+  // Returns true if the docshell should be considered a subframe.
>+  static PRBool IsSubframe(nsIDocShellTreeItem *docShell);
> };
> 
> #endif  // nsMetricsService_h__
>Index: extensions/metrics/src/nsWindowCollector.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/metrics/src/nsWindowCollector.cpp,v
>retrieving revision 1.5.2.8
>diff -u -p -r1.5.2.8 nsWindowCollector.cpp
>--- extensions/metrics/src/nsWindowCollector.cpp	26 Apr 2006 04:18:51 -0000	1.5.2.8
>+++ extensions/metrics/src/nsWindowCollector.cpp	4 Aug 2006 20:09:16 -0000
>@@ -205,6 +205,11 @@ nsWindowCollector::Observe(nsISupports *
>       rv = properties->SetPropertyAsBool(NS_LITERAL_STRING("chrome"), PR_TRUE);
>       NS_ENSURE_SUCCESS(rv, rv);
>     }
>+    if (nsMetricsUtils::IsSubframe(item)) {
>+      rv = properties->SetPropertyAsBool(NS_LITERAL_STRING("subframe"),
>+                                         PR_TRUE);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>   } else if (strcmp(topic, "domwindowopened") == 0) {
>     // We'd like to log a window open event now, but the window opener
>     // has not yet been set when we receive the domwindowopened notification.
>@@ -229,6 +234,14 @@ nsWindowCollector::Observe(nsISupports *
>     // Log a window destroy event.
>     action.Assign("destroy");
>     window = do_GetInterface(subject);
>+
>+    nsCOMPtr<nsIDocShellTreeItem> item = do_QueryInterface(subject);
>+    NS_ENSURE_STATE(item);
>+    if (nsMetricsUtils::IsSubframe(item)) {
>+      rv = properties->SetPropertyAsBool(NS_LITERAL_STRING("subframe"),
>+                                         PR_TRUE);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>   }
> 
>   if (window) {

Comment 8

[Brian Ryner (not reading)]

checked in