334044 - build as standalone extension

Status: RESOLVED FIXED

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

Description

[Darin Fisher]

build as standalone extension

output to dist/xpi-stage/metrics

Comment 1

[Darin Fisher]

Attachment: v1 patch

This works, but there's a catch.  It only works if it is compiled with the same version of the VC runtime as Firefox.  That's because of the call to OpenANSIFileDesc.  Basically, that function requires that FF and the caller agree on the version of the VC runtime.  So, I think I need to use OpenNSPRFileDesc instead.  I'd like to do that as a follow-up patch once time permits.

Comment 2

[Brian Ryner (not reading)]

Comment on attachment 218530 [details] [diff] [review]
v1 patch

General comments:

An overall synopsis of what you changed here would have been very useful to me for reviewing.

Specific coments:

>--- src/nsLoadCollector.cpp	27 Mar 2006 22:48:40 -0000	1.3.2.8
>+++ src/nsLoadCollector.cpp	15 Apr 2006 15:33:27 -0000
>+#define MOZILLA_INTERNAL_API
>+#include "nsDocShellLoadTypes.h"
>+#undef MOZILLA_INTERNAL_API

Hm, this seems a little sketchy, as you pointed out in bug 326706.  I think we should revisit this and create a better API.  The fact that the docshell internally combines the load type and the flags into one field should probably not be exposed.  We should instead make sure the following two fields are exposed on nsIDocShell:

// load type as defined in nsIDocShellLoadInfo.idl
readonly attribute long loadType;

// load flags as defined in nsIWebNavigation.idl
readonly attribute unsigned long loadFlags;

Sound ok?

>@@ -196,78 +218,78 @@ nsLoadCollector::OnStateChange(nsIWebPro
>-    rv = nsMetricsUtils::PutUint16(props, NS_LITERAL_STRING("window"),
>+    rv = nsMetricsUtils::PutUint32(props, NS_LITERAL_STRING("window"),
>                                    nsMetricsService::GetWindowID(window));

There's no reason to have a nsMetricsUtils helper for this; I only had one for Uint16 because nsIWritablePropertyBag2 doesn't expose a setter for that type.  Just change it to directly call props->SetPropertyAsUint32(), and remove the nsMetricsUtils helper.

>--- src/nsMetricsConfig.cpp	28 Mar 2006 01:40:41 -0000	1.1.2.6
>+++ src/nsMetricsConfig.cpp	15 Apr 2006 15:33:28 -0000
>   // Make sure we are dealing with a <collector> element.
>   elem->GetLocalName(value);
>-  if (!value.EqualsLiteral("collector"))
>+  if (!value.Equals(NS_LITERAL_STRING("collector")))
>     return;
>-  value.Truncate();
>+  value.Cut(0, PR_UINT32_MAX);

This seems like a fairly non-intuitive use of the string API.  I can see from looking at the header that Truncate() is going to be somewhat inefficient -- it's going to force the string to get a mutable copy of its previous data even if it didn't have one before.

My preference would be for code clarity in nsMetricsConfig, so how about just changing this one to use separate strings for GetLocalName() and GetAttribute()?

>--- src/nsMetricsEventItem.h	7 Apr 2006 02:02:47 -0000	1.1.2.3
>+++ src/nsMetricsEventItem.h	15 Apr 2006 15:33:28 -0000
>-  nsCOMArray<nsIMetricsEventItem> mChildren;
>+  nsTArray< nsCOMPtr<nsIMetricsEventItem> > mChildren;

I eventually figured out that you did this because nsCOMArray isn't in the XPCOM glue library in ff1.5.  I'm not really a huge fan of changing to nsTArray, mostly because of the weirdness it creates with signed vs unsigned indices.  Would it make any sense to link nsCOMArray in directly, then simply stop doing so when we no longer want to support 1.5?

>--- src/nsMetricsService.cpp	1 Apr 2006 00:09:29 -0000	1.4.2.13
>+++ src/nsMetricsService.cpp	15 Apr 2006 15:33:29 -0000
>+// We need to suppress inclusion of nsString.h
>+#define nsString_h___
>+#include "nsIStringStream.h"
>+#undef nsString_h___

eh?

>+  } else {
>+    nsCOMPtr<nsIStringInputStream> errorStream =
>+        do_CreateInstance("@mozilla.org/io/string-input-stream;1");
>+    NS_ENSURE_STATE(errorStream);
>+
>+    rv = errorStream->SetData("no metrics data", -1);
>+    NS_ENSURE_SUCCESS(rv, rv);

What prompted you to add the error checking to this patch?  Is there a change in behavior of FileInputStream?

>+static NS_METHOD
>+CopySegmentToStream(nsIInputStream *inStr,
>+                    void *closure,
>+                    const char *buffer,
>+                    PRUint32 offset,
>+                    PRUint32 count,
>+                    PRUint32 *countWritten)
>+{
>+    nsIOutputStream *outStr = NS_STATIC_CAST(nsIOutputStream *, closure);

NS_METHOD declares the function explicitly to be |stdcall|, do we need that?

Also, indentation doesn't match the rest of the file.

>@@ -514,26 +583,26 @@ nsMetricsService::ProfileStartup()
>     }
>   }
>   
>   nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
>   NS_ENSURE_STATE(prefs);
>   nsresult rv = prefs->GetIntPref("metrics.event-count", &mEventCount);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // Update the session id pref for the new session
>   static const char kSessionIDPref[] = "metrics.last-session-id";
>   PRInt32 sessionID = -1;
>   prefs->GetIntPref(kSessionIDPref, &sessionID);
>-  mSessionID.Truncate();
>-  mSessionID.AppendInt(++sessionID);
>+  mSessionID.Cut(0, PR_UINT32_MAX);
>+  AppendInt(mSessionID, ++sessionID);

I guess my above comment about Truncate() vs Cut() applies here too.  This is the frozen string API though, I guess there's not much to be done about it.

>@@ -740,29 +814,29 @@ nsMetricsService::GetDataFileForUpload(n
>-  leafName.AppendLiteral(".bz2");
>+  leafName.Append(".bz2");

Append(NS_LITERAL_CSTRING(".bz2")) ?

>@@ -895,73 +970,89 @@ nsMetricsService::GetConfigFile(nsIFile 
> nsresult
> nsMetricsService::GenerateClientID(nsCString &clientID)
> {
>+  nsresult rv;
>+
>   nsCOMPtr<nsIUUIDGenerator> idgen =
>     do_GetService("@mozilla.org/uuid-generator;1");
>-  NS_ENSURE_STATE(idgen);
>+  if (!idgen) {
>+    // We must be running in an older version of Firefox.  Resort to another
>+    // method of generating a client ID.

I think I'd prefer just having a single codepath for generating a client id, rather than trying to use nsIUUIDGenerator when it's available.

The CryptoHash stuff seems kind of heavyweight, can't you just do what nsUUIDGenerator::GenerateUUIDInPlace() does (non-win32 case)?

Looks good otherwise but I think I'd like to see a new rev of the patch.

Comment 3

[Darin Fisher]

> An overall synopsis of what you changed here would have been very useful to 
> me for reviewing.

Sorry about that.


> >+#define MOZILLA_INTERNAL_API
> >+#include "nsDocShellLoadTypes.h"
> >+#undef MOZILLA_INTERNAL_API
> 
> Hm, this seems a little sketchy, as you pointed out in bug 326706.  

Yeah, I plan on removing those MOZILLA_INTERNAL_API guards from the header.


> We should instead make sure the following two fields are
> exposed on nsIDocShell:
> 
> // load type as defined in nsIDocShellLoadInfo.idl
> readonly attribute long loadType;
> 
> // load flags as defined in nsIWebNavigation.idl
> readonly attribute unsigned long loadFlags;

I can't change the nsIDocShell shipped with FF 1.5 :-(


> Just change it to directly call props->SetPropertyAsUint32(), and remove the
> nsMetricsUtils helper.

OK


> >--- src/nsMetricsConfig.cpp	28 Mar 2006 01:40:41 -0000	1.1.2.6
> >+++ src/nsMetricsConfig.cpp	15 Apr 2006 15:33:28 -0000
> >   // Make sure we are dealing with a <collector> element.
> >   elem->GetLocalName(value);
> >-  if (!value.EqualsLiteral("collector"))
> >+  if (!value.Equals(NS_LITERAL_STRING("collector")))
> >     return;
> >-  value.Truncate();
> >+  value.Cut(0, PR_UINT32_MAX);
> 
> This seems like a fairly non-intuitive use of the string API.  I can see from
> looking at the header that Truncate() is going to be somewhat inefficient --
> it's going to force the string to get a mutable copy of its previous data 
> even
> if it didn't have one before.

I'm confused.  I'm just doing what was already there.  On the 1.8 branch, Truncate is not defined for nsAString.  So, I needed to switch to using Cut, which is how Truncate is implemented on the trunk.  Under the hood, NS_StringSetDataRange ends up calling the internal version of Cut.  If there is inefficiency there, then we ought to fix it where the API is implemented.


> My preference would be for code clarity in nsMetricsConfig, so how about just
> changing this one to use separate strings for GetLocalName() and
> GetAttribute()?

OK


> >--- src/nsMetricsEventItem.h	7 Apr 2006 02:02:47 -0000	1.1.2.3
> >+++ src/nsMetricsEventItem.h	15 Apr 2006 15:33:28 -0000
> >-  nsCOMArray<nsIMetricsEventItem> mChildren;
> >+  nsTArray< nsCOMPtr<nsIMetricsEventItem> > mChildren;
> 
> I eventually figured out that you did this because nsCOMArray isn't in the
> XPCOM glue library in ff1.5.

You mean FF 2.0a, right?


> I'm not really a huge fan of changing to
> nsTArray, mostly because of the weirdness it creates with signed vs unsigned
> indices.  Would it make any sense to link nsCOMArray in directly, then simply
> stop doing so when we no longer want to support 1.5?

nsCOMArray.h includes nsVoidArray.h, which includes nsString.h, and that conflicts with nsStringAPI.h.  You can't mix frozen and non-frozen headers.  This has been cleaned up on the trunk so that you can actually make use of nsCOMArray there.

Perhaps I can come up with some better way of integrating with nsTArray.  I understand your concerns about the signed vs unsigned issue.

 
> >--- src/nsMetricsService.cpp	1 Apr 2006 00:09:29 -0000	1.4.2.13
> >+++ src/nsMetricsService.cpp	15 Apr 2006 15:33:29 -0000
> >+// We need to suppress inclusion of nsString.h
> >+#define nsString_h___
> >+#include "nsIStringStream.h"
> >+#undef nsString_h___
> 
> eh?

nsIStringStream on 1.8 branch includes nsString.h, so I need to prevent that.  It's a real ugly hack.  On the trunk, we've fixed nsIStringStream to only declare the interface, nsIStringInputStream.


> >+  } else {
> >+    nsCOMPtr<nsIStringInputStream> errorStream =
> >+        do_CreateInstance("@mozilla.org/io/string-input-stream;1");
> >+    NS_ENSURE_STATE(errorStream);
> >+
> >+    rv = errorStream->SetData("no metrics data", -1);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> 
> What prompted you to add the error checking to this patch?  Is there a change
> in behavior of FileInputStream?

No change in FileInputStream.  I just wanted to make sure that if we had no file to read that we displayed something in about:metrics.  If NewChannel fails, then the user's load of about:metrics seems to be ignored.


> >+static NS_METHOD
> >+CopySegmentToStream(nsIInputStream *inStr,
> >+                    void *closure,
> >+                    const char *buffer,
> >+                    PRUint32 offset,
> >+                    PRUint32 count,
> >+                    PRUint32 *countWritten)
> >+{
> >+    nsIOutputStream *outStr = NS_STATIC_CAST(nsIOutputStream *, closure);
> 
> NS_METHOD declares the function explicitly to be |stdcall|, do we need that?

Yes, it needs that.  This method is used with nsIInputStream::ReadSegments.


> Also, indentation doesn't match the rest of the file.

Good catch.  I copied this code from nsStreamUtils.cpp, where the indentation differs.


> >@@ -740,29 +814,29 @@ nsMetricsService::GetDataFileForUpload(n
> >-  leafName.AppendLiteral(".bz2");
> >+  leafName.Append(".bz2");
> 
> Append(NS_LITERAL_CSTRING(".bz2")) ?

using NS_LITERAL_CSTRING is a bit heavy weight.  it introduces two extra function calls when used with the frozen string API.  compared to a strlen call, this felt like the right trade-off.


> I think I'd prefer just having a single codepath for generating a client id,
> rather than trying to use nsIUUIDGenerator when it's available.
> 
> The CryptoHash stuff seems kind of heavyweight, can't you just do what
> nsUUIDGenerator::GenerateUUIDInPlace() does (non-win32 case)?

Sure, but that would more code.  I favored this approach because it was simpler.  However, it would be nice if we had consistent key generation.


> Looks good otherwise but I think I'd like to see a new rev of the patch.

OK

Comment 4

[Darin Fisher]

Attachment: v1.1 patch

revised patch.  i resolved the signed vs unsigned issue by changing the IDL.  i think it's better to use unsigned integers for indices.  i also fixed the FILE problem by implementing a bzip2 compression loop.

Comment 5

[Brian Ryner (not reading)]

(In reply to comment #3)
> I can't change the nsIDocShell shipped with FF 1.5 :-(

No, I think it would be worth fixing on the trunk though.

> I'm confused.  I'm just doing what was already there.  On the 1.8 branch,
> Truncate is not defined for nsAString.  So, I needed to switch to using Cut,
> which is how Truncate is implemented on the trunk.  Under the hood,
> NS_StringSetDataRange ends up calling the internal version of Cut.  If there is
> inefficiency there, then we ought to fix it where the API is implemented.

I misunderstood what NS_StringGetMutableData does -- since the SetLength happens before the BeginWriting call, there's no extra string copy here.

> > I eventually figured out that you did this because nsCOMArray isn't in the
> > XPCOM glue library in ff1.5.
> 
> You mean FF 2.0a, right?

Both, apparently.

> nsIStringStream on 1.8 branch includes nsString.h, so I need to prevent that. 
> It's a real ugly hack.  On the trunk, we've fixed nsIStringStream to only
> declare the interface, nsIStringInputStream.

Ok, can you make the comment explain that?

> > What prompted you to add the error checking to this patch?  Is there a change
> > in behavior of FileInputStream?
> 
> No change in FileInputStream.  I just wanted to make sure that if we had no
> file to read that we displayed something in about:metrics.  If NewChannel
> fails, then the user's load of about:metrics seems to be ignored.

I see.  Would it be hard to separate this out and do it as a separate patch?

> > >@@ -740,29 +814,29 @@ nsMetricsService::GetDataFileForUpload(n
> > >-  leafName.AppendLiteral(".bz2");
> > >+  leafName.Append(".bz2");
> > 
> > Append(NS_LITERAL_CSTRING(".bz2")) ?
> 
> using NS_LITERAL_CSTRING is a bit heavy weight.  it introduces two extra
> function calls when used with the frozen string API.  compared to a strlen
> call, this felt like the right trade-off.

OK, I missed that difference when I was reading nsStringAPI.h.  Sounds fine.

> Sure, but that would more code.  I favored this approach because it was
> simpler.  However, it would be nice if we had consistent key generation.

That's fine.  The difference in key generation across Firefox versions is what really had me worried.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 218736 [details] [diff] [review]
v1.1 patch

>--- public/nsIMetricsService.idl	1 Apr 2006 00:07:37 -0000	1.3.2.3
>+++ public/nsIMetricsService.idl	17 Apr 2006 22:39:54 -0000
> 
>   /**
>    * Returns the first occurrence of the given item in the child list,
>    * or -1 if the item is not in the child list.
>    */
>-  long indexOf(in nsIMetricsEventItem item);
>+  unsigned long indexOf(in nsIMetricsEventItem item);

I guess this is a reason I'd prefer signed indices here -- you can't safely use -1 as a "not found" value.  This is probably the path of least resistance right now though.  In any case, the above comment isn't quite right with this change, is it? (should it say PR_UINT32_MAX?)

>--- src/nsMetricsEventItem.cpp	1 Apr 2006 00:08:47 -0000	1.1.2.2
>+++ src/nsMetricsEventItem.cpp	17 Apr 2006 22:39:54 -0000
> NS_IMETHODIMP
>-nsMetricsEventItem::ChildAt(PRInt32 index, nsIMetricsEventItem **result)
>+nsMetricsEventItem::ChildAt(PRUint32 index, nsIMetricsEventItem **result)
> {
>-  if (index < 0 || index >= mChildren.Count()) {
>+  if (index < 0 || index >= mChildren.Length()) {

It's not possible for index to be < 0 now.

>--- src/nsMetricsService.cpp	1 Apr 2006 00:09:29 -0000	1.4.2.13
>+++ src/nsMetricsService.cpp	17 Apr 2006 22:39:54 -0000
>+#endif // NS_METRICS_SEND_UNCOMPRESSED_DATA

Really #endif // !defined(NS_METRICS_SEND_UNCOMPRESSED_DATA), right?

>--- src/nsMetricsService.h	1 Apr 2006 00:10:07 -0000	1.3.2.7
>+++ src/nsMetricsService.h	17 Apr 2006 22:39:54 -0000
>-  // Creates a new nsHashPropertyBag instance.
>-  static nsresult NewPropertyBag(nsHashPropertyBag **result);

NewProperty bag seems useful, to avoid replicating the contract id in each collector.  Can we keep it, and make it return a nsIWritablePropertyBag2?

>--- src/nsWindowCollector.cpp	27 Mar 2006 22:51:17 -0000	1.5.2.4
>+++ src/nsWindowCollector.cpp	17 Apr 2006 22:39:54 -0000
>   if (window) {
>-    rv = nsMetricsUtils::PutUint16(properties, NS_LITERAL_STRING("windowid"),
>-                                   nsMetricsService::GetWindowID(window));
>+    rv = properties->SetPropertyAsUint32(NS_LITERAL_STRING("windowid"),
>+                                     nsMetricsService::GetWindowID(window));

Can you re-indent the second line, if it fits?

Looks ok otherwise, see my above comment in the bug too regarding the error-checking additions.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 218736 [details] [diff] [review]
v1.1 patch

Actually, I don't think I like this way of addressing the signed/unsigned problem.  It makes it fairly cumbersome to use indexOf() from javascript, since you have to manually compare to the max uint32 value (unless I'm missing something).  Some quick testing in xpcshell shows that it doesn't automatically become equal to -1.

Given a choice, I'd rather move the nastiness into metrics and expose a nice API.  I think in this case that will mean going back to the internal casting in your previous patch.  We should continue to limit the max index to PR_INT32_MAX so that the API is consistent.

Comment 8

[Brian Ryner (not reading)]

Or, define a constant in the IDL (kNotFound or something) that the result from indexOf() can be compared against.

Comment 9

[Darin Fisher]

OK, I'll revert the interface to using "signed" offsets.

Comment 10

[Darin Fisher]

> >--- src/nsMetricsService.cpp	1 Apr 2006 00:09:29 -0000	1.4.2.13
> >+++ src/nsMetricsService.cpp	17 Apr 2006 22:39:54 -0000
> >+#endif // NS_METRICS_SEND_UNCOMPRESSED_DATA
> 
> Really #endif // !defined(NS_METRICS_SEND_UNCOMPRESSED_DATA), right?

I prefer just mentioning the define being #ifdef'd, but okay, I'll change it to what you suggest.


> >--- src/nsMetricsService.h	1 Apr 2006 00:10:07 -0000	1.3.2.7
> >+++ src/nsMetricsService.h	17 Apr 2006 22:39:54 -0000
> >-  // Creates a new nsHashPropertyBag instance.
> >-  static nsresult NewPropertyBag(nsHashPropertyBag **result);
> 
> NewProperty bag seems useful, to avoid replicating the contract id in each
> collector.  Can we keep it, and make it return a nsIWritablePropertyBag2?

OK


> Can you re-indent the second line, if it fits?

The indentation was purposeful because it didn't fit ;-)

Comment 11

[Darin Fisher]

Attachment: v1.2 patch

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 218892 [details] [diff] [review]
v1.2 patch

looks good

Comment 13

[Darin Fisher]

fixed-on-trunk, fixed1.8.1

Comment 14

[Brian Ryner (not reading)]

Attachment: fix assertions from nsMetricsEventItem::SetProperties()

I started hitting an assertion here when changing my profile collection code over from nsHashPropertyBag to nsIWritablePropertyBag2.  Since nsIWritablePropertyBag2 inherits from nsIPropertyBag, you can pass it directly to SetProperties, but, nsCOMPtr will assert because it's a different interface pointer than the one it gets from QI to nsIPropertyBag (that one is the nsIWritablePropertyBag pointer).

Let's just work around it here for now rather than QI'ing at every call site.

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 218917 [details] [diff] [review]
fix assertions from nsMetricsEventItem::SetProperties()

checked in, thanks.