1219464 - Replace PRLogModuleInfo usage with LazyLogModule in widget/

Status: RESOLVED FIXED

(Core :: Widget, defect, P4)

Description

[Eric Rahm [:erahm]]

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");

Comment 1

[bugmozillahack]

Attachment: 1219464.diff

Done, have a look.

Comment 2

[Eric Rahm [: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)

Comment 3

[kshitija]

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

[chaithanya]

Hi

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

Comment 5

[chaithanya]

Attachment: Made changes as per comment 2. Kindly let me know if any more changes are to be made.

Comment 6

[Eric Rahm [: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");

Comment 7

[Gaith]

Attachment: Replace PRLogModuleInfo usage with LazyLogModule in widget/

Made changes after reading the comments, please let me know if other changes are required.

Comment 8

[Eric Rahm [: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()'

Comment 9

[Gaith]

Attachment: Replace PRLogModuleInfo usage with LazyLogModule in widget/

Updated patch according to https://bugzilla.mozilla.org/show_bug.cgi?id=1219464#c8

Comment 10

[Eric Rahm [:erahm]]

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.

Comment 11

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f346033587f7

Comment 12

[Gaith]

Attachment: 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.

Comment 13

[Eric Rahm [:erahm]]

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.

Comment 14

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b04815ae022

Comment 15

[Gaith]

Attachment: 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.

Comment 16

[Eric Rahm [: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.

Comment 17

[Gaith]

Attachment: Removed static from externed Logs

Comment 18

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=366f28c86976

Comment 19

[Gaith]

Eric, is it the patch what caused the build to fail on OS X?

Comment 20

[Eric Rahm [:erahm]]

(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

[Shubhodeep Mitra]

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.

Comment 22

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad7f8d33890fd31b1cb8f9aa9c4f29f03533353c

Comment 23

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d35c5ef90809d585656fd59600c5fd74ee59f722

Comment 24

[Eric Rahm [:erahm]]

Attachment: Replace PRLogModuleInfo usage with LazyLogModule in widget/

MozReview-Commit-ID: Ge7I8YlNqgM

Comment 25

[Eric Rahm [:erahm]]

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.

Comment 26

[Eric Rahm [: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

Comment 27

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed543a5a37a7f19e376c7e77de9fe178ea99b28
Bug 1219464 - Replace PRLogModuleInfo usage with LazyLogModule in widget/. r=erahm

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ed543a5a37a

Comment 29

[Eric Rahm [:erahm]]

Gaith, sorry it took so long, but it looks like your first contribution just got landed!