Closed Bug 531859 Opened 11 years ago Closed 10 years ago

Remove printf console output from PluginInstance*

Categories

(Core :: Plug-ins, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jimm, Assigned: benjamin)

References

Details

Attachments

(1 file, 2 obsolete files)

Currently log output is printed to console:

#undef _MOZ_LOG
#define _MOZ_LOG(s) printf("[PluginProcessParent] %s\n", s)

This should be tied to low level nspr log output for plugins. Current console output can really drag debug builds down.
Blocks: OOPP
No longer depends on: OOPP
I have a patch.
Assignee: jmathies → benjamin
When I was being lazy I just replace _MOZ_LOG(__FUNCTION__); with PLUGING_LOG_DEBUG_FUNCTION, but in some cases I was more careful and added more detailed parameter logging.
Attachment #418420 - Flags: review?(jones.chris.g)
Whoops, removing necessary code is bad.
Attachment #418420 - Attachment is obsolete: true
Attachment #418430 - Flags: review?(jones.chris.g)
Attachment #418420 - Flags: review?(jones.chris.g)
Comment on attachment 418430 [details] [diff] [review]
Fix logging by using NSPR logging instead of printf, rev. 1.1

>diff --git a/dom/plugins/Makefile.in b/dom/plugins/Makefile.in
>--- a/dom/plugins/Makefile.in
>+++ b/dom/plugins/Makefile.in
>@@ -111,8 +111,10 @@ LOCAL_INCLUDES = \
>   -I$(topsrcdir)/modules/plugin/base/src/ \
>   $(NULL)
> 
> include $(topsrcdir)/config/config.mk
> include $(topsrcdir)/ipc/chromium/chromium-config.mk
> endif
> 
> include $(topsrcdir)/config/rules.mk
>+
>+DEFINES += -DFORCE_PR_LOG
>\ No newline at end of file

Might as well fix this while we're at it.

>diff --git a/dom/plugins/PluginInstanceChild.cpp b/dom/plugins/PluginInstanceChild.cpp
>--- a/dom/plugins/PluginInstanceChild.cpp
>+++ b/dom/plugins/PluginInstanceChild.cpp
> 
> NPError
> PluginInstanceChild::NPN_SetValue(NPPVariable aVar, void* aValue)
> {
>-    printf ("[PluginInstanceChild] NPN_SetValue(%s, %ld)\n",
>-            NPPVariableToString(aVar), reinterpret_cast<intptr_t>(aValue));
>     AssertPluginThread();
> 

Any reason we're not logging this anymore?

>diff --git a/dom/plugins/PluginMessageUtils.h b/dom/plugins/PluginMessageUtils.h
>--- a/dom/plugins/PluginMessageUtils.h
>+++ b/dom/plugins/PluginMessageUtils.h
>@@ -42,32 +42,39 @@
> #include "IPC/IPCMessageUtils.h"
> 
> #include "npapi.h"
> #include "npruntime.h"
> #include "npfunctions.h"
> #include "nsAutoPtr.h"
> #include "nsStringGlue.h"
> #include "nsThreadUtils.h"
>+#include "prlog.h"
> 
> namespace mozilla {
> 
> // XXX might want to move these to nscore.h or something, they can be
> // generally useful
> struct void_t { };
> struct null_t { };
> 
> namespace ipc {
> 
> typedef intptr_t NPRemoteIdentifier;
> 
> } /* namespace ipc */
> 
> namespace plugins {
> 
>+extern PRLogModuleInfo* gPluginLog;
>+
>+#define PLUGIN_LOG_DEBUG(args) PR_LOG(gPluginLog, PR_LOG_DEBUG, args)
>+#define PLUGIN_LOG_DEBUG_FUNCTION PR_LOG(gPluginLog, PR_LOG_DEBUG, ("%s", __FUNCTION__))
>+#define PLUGIN_LOG_DEBUG_METHOD PR_LOG(gPluginLog, PR_LOG_DEBUG, ("%s<%p>", __FUNCTION__, (void*) this))
>+

It really stinks that this logging doesn't have class/file name.  We could get log output like

  NPP_Foo
  NPP_Foo
  NPP_Foo
  NPP_Foo

Maybe we could log __FILE__ along with __FUNCTION__?  Another option could be a MOZ_NPAPI_THING that each .cpp defines for itself, e.g. |#define MOZ_NPAPI_THING "PluginInstanceChild"|, and PLUGIN_LOG_* magically uses.
Attachment #418430 - Flags: review?(jones.chris.g) → review-
Ideally I think that IPDL should do forced PR logging and a lot of this logging is irrelevant, but until then this should do the trick.
Attachment #418430 - Attachment is obsolete: true
Attachment #420572 - Flags: review?(jones.chris.g)
(In reply to comment #5)
> Ideally I think that IPDL should do forced PR logging and a lot of this logging
> is irrelevant, but until then this should do the trick.

Yeah, some of this is redundant.  We only need to log (i) messages with params we want to print; (ii) calls to NP* functions that might not result in IPC, like NPN_GetValue().  IPDL can log the rest.
Comment on attachment 420572 [details] [diff] [review]
Fix logging by using NSPR logging instead of printf, rev. 1.2

Cool.  I didn't know about __PRETTY_FUNCTION__.
Attachment #420572 - Flags: review?(jones.chris.g) → review+
http://hg.mozilla.org/mozilla-central/rev/1ef03c8f6ad1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.