Move MetroUtils logging features into WinUtils

RESOLVED FIXED in mozilla28

Status

()

Core
Widget: Win32
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

26 Branch
mozilla28
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created attachment 793412 [details] [diff] [review]
patch
Assignee: nobody → jmathies
(Assignee)

Comment 2

4 years ago
Created attachment 793415 [details] [diff] [review]
patch
Attachment #793412 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Blocks: 906350
(Assignee)

Comment 3

4 years ago
Created attachment 794187 [details] [diff] [review]
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.
Attachment #793415 - Attachment is obsolete: true
Attachment #794187 - Flags: review?(masayuki)
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.
Attachment #794187 - Flags: review?(masayuki) → review+
(Assignee)

Comment 5

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/7c8790d032b5
https://hg.mozilla.org/mozilla-central/rev/7c8790d032b5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
https://hg.mozilla.org/mozilla-central/rev/7c8790d032b5

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.