Replace PRLogModuleInfo usage with LazyLogModule in widget/

RESOLVED FIXED in Firefox 55

Status

()

Core
Widget
P4
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: erahm, Assigned: Gaith, Mentored)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox55 fixed)

Details

(Whiteboard: [lang=c++][good first bug][tpi:-])

Attachments

(1 attachment, 7 obsolete attachments)

This covers replacing PRLogModuleInfo w/ LazyLogModule in the 'widget/' directory.

Current usage:
> ./widget/android/nsAppShell.cpp:250:        gWidgetLog = PR_NewLogModule("Widget");
> ./widget/cocoa/NativeKeyBindings.mm:67:    gNativeKeyBindingsLog = PR_NewLogModule("NativeKeyBindings");
> ./widget/cocoa/nsChildView.mm:259:    sCocoaLog = PR_NewLogModule("nsCocoaWidgets");
> ./widget/cocoa/TextInputHandler.mm:320:    gLog = PR_NewLogModule("TextInputHandlerWidgets");
> ./widget/ContentCache.cpp:101:    sContentCacheLog = PR_NewLogModule("ContentCacheWidgets");
> ./widget/gtk/IMContextWrapper.cpp:197:        gGtkIMLog = PR_NewLogModule("nsGtkIMModuleWidgets");
> ./widget/gtk/nsAppShell.cpp:83:        gWidgetLog = PR_NewLogModule("Widget");
> ./widget/gtk/nsAppShell.cpp:85:        gWidgetFocusLog = PR_NewLogModule("WidgetFocus");
> ./widget/gtk/nsAppShell.cpp:87:        gWidgetDragLog = PR_NewLogModule("WidgetDrag");
> ./widget/gtk/nsAppShell.cpp:89:        gWidgetDrawLog = PR_NewLogModule("WidgetDraw");
> ./widget/gtk/nsDeviceContextSpecG.cpp:41:    sLog = PR_NewLogModule("DeviceContextSpecGTK");
> ./widget/gtk/nsDragService.cpp:136:        sDragLm = PR_NewLogModule("nsDragService");
> ./widget/gtk/nsGtkKeyUtils.cpp:167:        gKeymapWrapperLog = PR_NewLogModule("KeymapWrapperWidgets");
> ./widget/gtk/nsIdleServiceGTK.cpp:67:        sIdleLog = PR_NewLogModule("nsIIdleService");
> ./widget/nsIdleService.cpp:398:    sLog = PR_NewLogModule("idleService");
> ./widget/qt/nsAppShell.cpp:40:        gWidgetLog = PR_NewLogModule("Widget");
> ./widget/qt/nsAppShell.cpp:42:        gWidgetFocusLog = PR_NewLogModule("WidgetFocus");
> ./widget/qt/nsAppShell.cpp:44:        gWidgetIMLog = PR_NewLogModule("WidgetIM");
> ./widget/qt/nsAppShell.cpp:46:        gWidgetDrawLog = PR_NewLogModule("WidgetDraw");
> ./widget/qt/nsDeviceContextSpecQt.cpp:36:    PR_NewLogModule("DeviceContextSpecQt");
> ./widget/windows/IMMHandler.cpp:217:    gIMMLog = PR_NewLogModule("nsIMM32HandlerWidgets");
> ./widget/windows/KeyboardLayout.cpp:2260:      sKeyboardLayoutLogger = PR_NewLogModule("KeyboardLayoutWidgets");
> ./widget/windows/nsAppShell.cpp:35:    log = PR_NewLogModule("WinWakeLock");
> ./widget/windows/nsClipboard.cpp:55:    gWin32ClipboardLog = PR_NewLogModule("nsClipboard");
> ./widget/windows/nsDeviceContextSpecWin.cpp:47:PRLogModuleInfo * kWidgetPrintingLogMod = PR_NewLogModule("printing-widget");
> ./widget/windows/nsSound.cpp:113:      gWin32SoundLog = PR_NewLogModule("nsSound");
> ./widget/windows/TSFTextStore.cpp:5181:    sTextStoreLog = PR_NewLogModule("nsTextStoreWidgets");
> ./widget/windows/WinMouseScrollHandler.cpp:103:    gMouseScrollLog = PR_NewLogModule("MouseScrollHandlerWidgets");
> ./widget/windows/WinUtils.cpp:437:    gWindowsLog = PR_NewLogModule("Widget");
> ./widget/xremoteclient/XRemoteClient.cpp:70:    sRemoteLm = PR_NewLogModule("XRemoteClient");
Mentor: erahm@mozilla.com
Whiteboard: [lang=c++][good first bug]

Comment 1

2 years ago
Created attachment 8732504 [details] [diff] [review]
1219464.diff

Done, have a look.
Attachment #8732504 - Flags: review?(erahm)
Comment on attachment 8732504 [details] [diff] [review]
1219464.diff

Review of attachment 8732504 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the interest! We're going to need to change more than just swapping out the usage of PR_NewLogModule. Bug 1219461, comment 0 provides more guidance on how to go about this. I'll put a few notes inline as well. Let me know if you have any questions.

Also if you could attempt to compile your code prior to submission for review that would be very helpful.

::: widget/ContentCache.cpp
@@ -93,4 @@
>   * mozilla::ContentCache
>   *****************************************************************************/
>  
>  PRLogModuleInfo* sContentCacheLog = nullptr;

In a case like this we would change this to:

> static LazyLogModule sContentCacheLog("ContentCacheWidgets");

@@ -102,1 @@
>    }

...and remove the whole if block here.

::: widget/gtk/nsDeviceContextSpecG.cpp
@@ -33,5 @@
>  
>  using namespace mozilla;
>  
>  static PRLogModuleInfo *
>  GetDeviceContextSpecGTKLog()

In a case like this where the function is only used locally we prefer to just remove the function and replace it with:

> static LayzLogModule sDeviceContextSpecGTKLog("DeviceContextSpecGTK");

@@ -42,4 @@
>    return sLog;
>  }
>  /* Macro to make lines shorter */
>  #define DO_PR_DEBUG_LOG(x) MOZ_LOG(GetDeviceContextSpecGTKLog(), mozilla::LogLevel::Debug, x)

...and then replace calls the function with the new log module, ie:

> #define DO_PR_DEBUG_LOG(x) MOZ_LOG(sDeviceContextSpecGTKLog, mozilla::LogLevel::Debug, x)
Attachment #8732504 - Flags: review?(erahm)

Comment 3

2 years ago
I would like to try and solve for this bug, Can anyone help me on steps to reproduce the bug and solve for it?

Comment 4

2 years ago
Hi

I would like to work on this bug. Could you please assign it to me?

Comment 5

2 years ago
Created attachment 8739678 [details] [diff] [review]
Made changes as per comment 2. Kindly let me know if any more changes are to be made.
Attachment #8739678 - Flags: review?(erahm)
Comment on attachment 8739678 [details] [diff] [review]
Made changes as per comment 2. Kindly let me know if any more changes are to be made.

Review of attachment 8739678 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a good start chaithanya. I've provided some comments below, I provided more detail what to do in each case in bug 1219461, comment 0. Primarily this is the piece that matters:

File Scoped Global
==================
> PRLogModuleInfo* (s|g|k)FooLogger;
> ....
> void Foo::Foo()
>   if (!sFooLogger) sFooLogger = PR_NewLogModule("FooLogger");

This would become:
> static LazyLogModule (s|g|k)FooLogger("FooLogger");

So the declaration changes, and the if block initializing the logger is removed.

::: widget/android/nsAppShell.cpp
@@ +256,5 @@
>  nsresult
>  nsAppShell::Init()
>  {
>      if (!gWidgetLog)
> +        gWidgetLog = LazyLogModule("Widget");

We actually want to remove things like this, instead you should change the |PR_LogModuleInfo* gWidgetLog| further up in the file to |static LazyLogModule gWidgetLog("Widget")| and delete this code block.

This comment applies to the rest of this patch.

::: widget/gtk/nsDeviceContextSpecG.cpp
@@ +33,5 @@
>  
>  using namespace mozilla;
>  
> +static LayzLogModule 
> +sDeviceContextSpecGTKLog("DeviceContextSpecGTK");

Go ahead and delete the function body as well.

::: widget/windows/nsDeviceContextSpecWin.cpp
@@ +43,5 @@
>  
>  #include "mozilla/gfx/Logging.h"
>  
>  #include "mozilla/Logging.h"
> +PRLogModuleInfo * kWidgetPrintingLogMod = LazyLogModule("printing-widget");

This should become:

static LazyLogModule kWidgetPrintingLogMod("printing-widget");
Attachment #8739678 - Flags: review?(erahm) → feedback+
(Assignee)

Comment 7

2 years ago
Created attachment 8752594 [details] [diff] [review]
Replace PRLogModuleInfo usage with LazyLogModule in widget/

Made changes after reading the comments, please let me know if other changes are required.
Attachment #8752594 - Flags: review?(erahm)
Comment on attachment 8752594 [details] [diff] [review]
Replace PRLogModuleInfo usage with LazyLogModule in widget/

Review of attachment 8752594 [details] [diff] [review]:
-----------------------------------------------------------------

Gaith, thank you for your contribution. This looks really good, just a few pieces to update.

::: widget/cocoa/TextInputHandler.mm
@@ -319,5 @@
> -  // Clear() is always called when TISInputSourceWrappper is created.
> -  if (!gLog) {
> -    gLog = PR_NewLogModule("TextInputHandlerWidgets");
> -    TextInputHandler::DebugPrintAllKeyboardLayouts();
> -    IMEInputHandler::DebugPrintAllIMEModes();

We'll need to keep these two lines. A simple (though not perfect) solution is to use static bool for the if instead.

Something like the following should suffice:

> static bool initialized = false;
> if (!initialized) {
>   initialized = true;
>   TextInputHandler::DebugPrintAllKeyboardLayouts();
>   IMEInputHandler::DebugPrintAllIMEModes();
> }

::: widget/windows/nsAppShell.cpp
@@ +27,5 @@
>  
>  using namespace mozilla;
>  using namespace mozilla::widget;
>  
> +#define WAKE_LOCK_LOG(...) MOZ_LOG(gWinWakeLockLog(), mozilla::LogLevel::Debug, (__VA_ARGS__))

You should be able to remove the '()' from 'gWinWakeLockLog()'
Attachment #8752594 - Flags: review?(erahm) → feedback+
Attachment #8732504 - Attachment is obsolete: true
Attachment #8739678 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Created attachment 8753620 [details] [diff] [review]
Replace PRLogModuleInfo usage with LazyLogModule in widget/

Updated patch according to https://bugzilla.mozilla.org/show_bug.cgi?id=1219464#c8
Attachment #8753620 - Flags: review?(erahm)
(Assignee)

Updated

2 years ago
Attachment #8752594 - Attachment is obsolete: true
Comment on attachment 8753620 [details] [diff] [review]
Replace PRLogModuleInfo usage with LazyLogModule in widget/

Review of attachment 8753620 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, I'm going to push a try build to make sure all platforms are happy.
Attachment #8753620 - Flags: review?(erahm) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f346033587f7
(Assignee)

Comment 12

2 years ago
Created attachment 8753927 [details] [diff] [review]
Replace PRLogModuleInfo usage with LazyLogModule in widget/

Fixed the problem that caused the build to fail on Windows and OS X.
Could you please push a try build again.
Attachment #8753927 - Flags: review?(erahm)
(Assignee)

Updated

2 years ago
Attachment #8753620 - Attachment is obsolete: true
Comment on attachment 8753620 [details] [diff] [review]
Replace PRLogModuleInfo usage with LazyLogModule in widget/

Review of attachment 8753620 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like we had minor build failures on Windows and OS X. We just need to add the 'mozilla' namespace to a few declarations. Gaith, can you update the patch?

::: widget/cocoa/nsClipboard.mm
@@ +30,5 @@
>  
>  // Screenshots use the (undocumented) png pasteboard type.
>  #define IMAGE_PASTEBOARD_TYPES NSTIFFPboardType, @"Apple PNG pasteboard type", nil
>  
> +extern LazyLogModule sCocoaLog;

add 'mozilla::' to the type.

::: widget/cocoa/nsDragService.mm
@@ +30,5 @@
>  
>  using namespace mozilla;
>  using namespace mozilla::gfx;
>  
> +extern LazyLogModule sCocoaLog;

add 'mozilla::' to the type.

::: widget/windows/WinUtils.cpp
@@ +57,5 @@
>  #endif // #ifdef NS_ENABLE_TSF
>  
>  #include <shlwapi.h>
>  
> +static LazyLogModule gWindowsLog("Widget");

add 'mozilla::' to the type.

::: widget/windows/nsAppShell.cpp
@@ +28,5 @@
>  using namespace mozilla;
>  using namespace mozilla::widget;
>  
> +#define WAKE_LOCK_LOG(...) MOZ_LOG(gWinWakeLockLog, mozilla::LogLevel::Debug, (__VA_ARGS__))
> +static LazyLogModule gWinWakeLockLog("WinWakeLock");

add 'mozilla::' to the type.

::: widget/windows/nsClipboard.cpp
@@ +37,5 @@
>  #include "nsIObserverService.h"
>  
>  using mozilla::LogLevel;
>  
> +static LazyLogModule gWin32ClipboardLog("nsClipboard");

add 'mozilla::' to the type.

::: widget/windows/nsDeviceContextSpecWin.cpp
@@ +44,5 @@
>  
>  #include "mozilla/gfx/Logging.h"
>  
>  #include "mozilla/Logging.h"
> +static LazyLogModule kWidgetPrintingLogMod("printing-widget");

add 'mozilla::' to the type.

::: widget/windows/nsNativeThemeWin.cpp
@@ +41,5 @@
>  using mozilla::IsVistaOrLater;
>  using namespace mozilla;
>  using namespace mozilla::widget;
>  
> +extern LazyLogModule gWindowsLog;

add 'mozilla::' to the type.

::: widget/windows/nsSound.cpp
@@ +29,5 @@
>  #include "nsThreadUtils.h"
>  
>  using mozilla::LogLevel;
>  
> +static LazyLogModule gWin32SoundLog("nsSound");

add 'mozilla::' to the type.

::: widget/windows/nsWinGesture.cpp
@@ +20,5 @@
>  
>  using namespace mozilla;
>  using namespace mozilla::widget;
>  
> +extern LazyLogModule gWindowsLog;

add 'mozilla::' to the type.

::: widget/windows/nsWindow.cpp
@@ +328,5 @@
>   **************************************************************/
>  
>  static const char *sScreenManagerContractID       = "@mozilla.org/gfx/screenmanager;1";
>  
> +extern LazyLogModule gWindowsLog;

add 'mozilla::' to the type.

::: widget/windows/nsWindowDbg.cpp
@@ +11,5 @@
>  #include "WinUtils.h"
>  
>  using namespace mozilla::widget;
>  
> +extern LazyLogModule gWindowsLog;

add 'mozilla::' to the type.
Attachment #8753620 - Attachment is obsolete: false
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b04815ae022
(Assignee)

Comment 15

2 years ago
Created attachment 8753963 [details] [diff] [review]
Added the 'mozilla' namespace to the declarations

Sorry, both attachment 8753927 [details] [diff] [review] and attachment 8753620 [details] [diff] [review] had the same description.
I've added the 'mozilla' namespace to the declarations that you have mentioned in Comment 13.
Attachment #8753963 - Flags: review?(erahm)
Comment on attachment 8753963 [details] [diff] [review]
Added the 'mozilla' namespace to the declarations

Review of attachment 8753963 [details] [diff] [review]:
-----------------------------------------------------------------

Super close, it compiled okay but failed to link on Windows and OS X. It looks like we need to remove 'static' from the logs that are being externed.

::: widget/cocoa/nsChildView.mm
@@ +111,5 @@
>  // Don't put more than this many rects in the dirty region, just fluff
>  // out to the bounding-box if there are more
>  #define MAX_RECTS_IN_REGION 100
>  
> +static LazyLogModule sCocoaLog("nsCocoaWidgets");

Remove static here.

::: widget/windows/WinUtils.cpp
@@ +57,5 @@
>  #endif // #ifdef NS_ENABLE_TSF
>  
>  #include <shlwapi.h>
>  
> +static mozilla::LazyLogModule gWindowsLog("Widget");

Remove static.
Attachment #8753963 - Flags: review?(erahm) → feedback+
(Assignee)

Comment 17

2 years ago
Created attachment 8754018 [details] [diff] [review]
Removed static from externed Logs
Attachment #8754018 - Flags: review?(erahm)
(Assignee)

Updated

2 years ago
Attachment #8753620 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8753927 - Attachment is obsolete: true
Attachment #8753927 - Flags: review?(erahm)
(Assignee)

Updated

2 years ago
Attachment #8753963 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=366f28c86976
(Assignee)

Comment 19

2 years ago
Eric, is it the patch what caused the build to fail on OS X?
(In reply to Gaith from comment #19)
> Eric, is it the patch what caused the build to fail on OS X?

Yes we've seen issues like this before. Basically a logger is being used before logging is configured. I'll see if I can repro locally.

Comment 21

a year ago
Hello, I'm new and am really passionate about fixing my first bug, will anyone please help me out with fixing this bug or the steps that i need to follow.
Thank you.

Updated

a year ago
Priority: -- → P4
Whiteboard: [lang=c++][good first bug] → [lang=c++][good first bug][tpi:-]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad7f8d33890fd31b1cb8f9aa9c4f29f03533353c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d35c5ef90809d585656fd59600c5fd74ee59f722
Created attachment 8854061 [details] [diff] [review]
Replace PRLogModuleInfo usage with LazyLogModule in widget/

MozReview-Commit-ID: Ge7I8YlNqgM
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee: erahm → gaithhallak
Comment on attachment 8754018 [details] [diff] [review]
Removed static from externed Logs

I just attached a rebased patch, I think we can land this now.
Attachment #8754018 - Attachment is obsolete: true
Attachment #8754018 - Flags: review?(erahm)
Comment on attachment 8854061 [details] [diff] [review]
Replace PRLogModuleInfo usage with LazyLogModule in widget/

This is just a rebase of Gaith's patch. r=me
Attachment #8854061 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed543a5a37a7f19e376c7e77de9fe178ea99b28
Bug 1219464 - Replace PRLogModuleInfo usage with LazyLogModule in widget/. r=erahm

Comment 28

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9ed543a5a37a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Gaith, sorry it took so long, but it looks like your first contribution just got landed!
You need to log in before you can comment on or make changes to this bug.