223694 - [FIXr]Add NSPR logging to content dispatch

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

This may make some debugging simpler.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like this

Comment 2

[Darin Fisher]

Comment on attachment 134140 [details] [diff] [review]
Like this

>Index: uriloader/base/nsURILoader.cpp

> nsresult nsDocumentOpenInfo::Open(nsIChannel *aChannel)
> {
>+  LOG(("[0x%p] nsDocumentOpenInfo::Open"));

looks like a parameter is missing...


>   // no content from this load - that's OK.
>   if (rv == NS_ERROR_DOM_RETVAL_UNDEFINED ||
>       rv == NS_ERROR_NO_CONTENT) {
>+    LOG(("  rv is NS_ERROR_DOM_RETVAL_UNDEFINED or NS_ERROR_NO_CONTENT -- doing nothing"));
>       rv = NS_OK;
>   }

indentation is off.. maybe |cvs diff -w|?


> nsDocumentOpenInfo::TryContentListener(nsIURIContentListener* aListener,
>                                        nsIChannel* aChannel)
> {
>+  LOG(("[0x%p] nsDocumentOpenInfo::TryContentListener; mIsContentPreferred = %s",
>+       this, mIsContentPreferred ? "true" : "false"));

if you end up enabling FORCE_PR_LOG, then i would do away with this pretty
printing of a boolean value... same goes for "yes"/"no" in other places.


>+nsresult
>+nsURILoader::Init()
>+{
>+#ifdef PR_LOGGING
>+  if (!mLog) {
>+    mLog = PR_NewLogModule("URILoader");
>+    if (!mLog)
>+      return NS_ERROR_OUT_OF_MEMORY;
>+  }
>+#endif
>+  return NS_OK;
>+}

bleh.. why not just do this in the constructor.  in many many other
places we don't bother to null check PR_NewLogModule.  no need i think
since this is not compiled into optimized builds.  but, the Init method
is going to be compiled into optimized builds :(


sr=darin

Comment 3

[Boris Zbarsky [:bzbarsky]]

> looks like a parameter is missing...

Nice catch...  Fixed.

> indentation is off.. maybe |cvs diff -w|?

No, the body of the if was mis-indented.  Fixed.

Would it help if I inlined init?  Otherwise, I can indeed move that code over
into the constructor.

Comment 4

[Darin Fisher]

>Would it help if I inlined init?  Otherwise, I can indeed move that code over
>into the constructor.

maybe... i'm okay with this either way.  only meant to point out that most
modules just allocate log modules in the ctor and assume it won't fail.  though
not exactly tidy, it seems reasonable.. especially since this is debug only. 
*shrug*

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 134140 [details] [diff] [review]
Like this

+	 LOG(("  Content handler taking over load"));
hm... if HandleContent failed, this is a lie, I think... maybe only print this
if NS_SUCCEEDED(rv) (or in the else branch of the if below)

r=biesi otherwise

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 7

[Boris Zbarsky [:bzbarsky]]

Patch checked in for 1.6b.