Closed
Bug 347305
Opened 19 years ago
Closed 19 years ago
make subframe loads easier to track
Categories
(Toolkit Graveyard :: Data Collection/Metrics, defect)
Toolkit Graveyard
Data Collection/Metrics
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bryner, Assigned: bryner)
Details
Attachments
(2 files, 1 obsolete file)
3.28 KB,
patch
|
Details | Diff | Splinter Review | |
5.55 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•19 years ago
|
||
Attachment #232074 -
Flags: first-review?(darin)
Assignee | ||
Comment 2•19 years ago
|
||
Attachment #232079 -
Flags: first-review?(darin)
Comment 3•19 years ago
|
||
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
Attachment #232074 -
Flags: first-review?(darin) → first-review+
Comment 4•19 years ago
|
||
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
Attachment #232079 -
Flags: first-review?(darin) → first-review+
Assignee | ||
Comment 5•19 years ago
|
||
Comment on attachment 232074 [details] [diff] [review]
patch
Thanks, code was right, comment was wrong.
Assignee | ||
Comment 6•19 years ago
|
||
I checked in the first patch as-is, but then decided to go ahead and refactor to reduce code and comment duplication.
Attachment #232079 -
Attachment is obsolete: true
Attachment #232194 -
Flags: first-review?(darin)
Assignee | ||
Updated•19 years ago
|
Attachment #232194 -
Flags: first-review?(darin) → first-review?(brettw)
Comment 7•19 years ago
|
||
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) {
Attachment #232194 -
Flags: first-review?(brettw) → first-review+
Assignee | ||
Comment 8•19 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•