907635 - Move MetroUtils logging features into WinUtils

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

These are handy routines that avoid reliance on nspr logging. I'd like to move them into WinUtils so we can use them everywhere.

http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroUtils.h#20

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch

Comment 3

[Jim Mathies [:jimm]]

Attachment: patch v.1

nspr logging sucks, it's cumbersome to set up, it's cumbersome to add statements, and it's not always on in release builds. 

In metro we've been using some logging routines that are Windows centric - they dump to the console if it exists, they dump to the widget nspr log if it exists, and they dump to a remote debugger if one is attached. They are always on in release builds too.

I'd like them to be accessible anywhere.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 794187 [details] [diff] [review]
patch v.1

Sorry, I forgot this review request completely. I'm very sorry.

>--- a/widget/windows/WinUtils.h	Thu Aug 22 12:04:16 2013 +0100
>+++ b/widget/windows/WinUtils.h	Thu Aug 22 14:11:12 2013 -0500
>@@ -53,16 +60,23 @@
>   };
>   static WinVersion GetWindowsVersion();
> 
>   // Retrieves the Service Pack version number.
>   // Returns true on success, false on failure.
>   static bool GetWindowsServicePackVersion(UINT& aOutMajor, UINT& aOutMinor);
> 
>   /**
>+   * Logging helpers that dump output to prlog module 'Widget', console, and
>+   * OutputDebugString. Note these output in both debug and release builds.
>+   */
>+  static void Log(const char *fmt, ...);
>+  static void LogW(const wchar_t *fmt, ...);

You explain these methods like this, but you don't add

> #ifdef MOZ_LOGGING
> #define FORCE_PR_LOG /* Allow logging in the release build */
> #endif // MOZ_LOGGING

before including prlog.h in WinUtils.cpp. That means the methods don't output the log via NSPR log module on release build.

Could you change the explanation or add these lines before including prlog.h?

>+// static
>+void
>+WinUtils::LogW(const wchar_t *fmt, ...)
>+{
>+  va_list args = NULL;
>+  if(!lstrlenW(fmt))
>+    return;

nit: Use {}

>+// static
>+void
>+WinUtils::Log(const char *fmt, ...)
>+{
>+  va_list args = NULL;
>+  if(!strlen(fmt))
>+    return;

nit: Use {}

>diff -r 89294cd501d9 widget/windows/nsWindow.cpp
>--- a/widget/windows/nsWindow.cpp	Thu Aug 22 12:04:16 2013 +0100
>+++ b/widget/windows/nsWindow.cpp	Thu Aug 22 14:11:12 2013 -0500
>@@ -244,17 +244,17 @@
>  *
>  * SECTION: globals variables
>  *
>  **************************************************************/
> 
> static const char *sScreenManagerContractID       = "@mozilla.org/gfx/screenmanager;1";
> 
> #ifdef PR_LOGGING
>-PRLogModuleInfo* gWindowsLog                      = nullptr;
>+extern PRLogModuleInfo* gWindowsLog;

Why do you need this? If this is not necessary, please remove this.

>diff -r 89294cd501d9 widget/windows/winrt/MetroApp.cpp
>--- a/widget/windows/winrt/MetroApp.cpp	Thu Aug 22 12:04:16 2013 +0100
>+++ b/widget/windows/winrt/MetroApp.cpp	Thu Aug 22 14:11:12 2013 -0500
>@@ -206,27 +207,27 @@
>   bool backgroundSessionClosed = argc > 1 && !wcsicmp(argv[1], L"-BackgroundSessionClosed");
>   LocalFree(argv);
>   return backgroundSessionClosed;
> }
> 
> bool
> XRE_MetroCoreApplicationRun()
> {
>+#ifdef PR_LOGGING
>+  if (!gWindowsLog) {
>+    gWindowsLog = PR_NewLogModule("Widget");
>+  }
>+#endif

Are you really sure this is necessary? Does this method runs before WinUtils::Init()? If so, it's okay.

Comment 5

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/7c8790d032b5

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c8790d032b5

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c8790d032b5