1219476 - Replace PRLogModuleInfo usage with LazyLogModule in gfx

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Eric Rahm [:erahm]]

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

Current usage:
> ./gfx/2d/Factory.cpp:61:    sLog = PR_NewLogModule("gfx2d");
> ./gfx/layers/Layers.cpp:2386:    sLog = PR_NewLogModule("Layers");
> ./gfx/thebes/gfxFT2FontList.cpp:57:        sLog = PR_NewLogModule("fontInfoLog");
> ./gfx/thebes/gfxPlatform.cpp:1817:        sFontlistLog = PR_NewLogModule("fontlist");
> ./gfx/thebes/gfxPlatform.cpp:1818:        sFontInitLog = PR_NewLogModule("fontinit");
> ./gfx/thebes/gfxPlatform.cpp:1819:        sTextrunLog = PR_NewLogModule("textrun");
> ./gfx/thebes/gfxPlatform.cpp:1820:        sTextrunuiLog = PR_NewLogModule("textrunui");
> ./gfx/thebes/gfxPlatform.cpp:1821:        sCmapDataLog = PR_NewLogModule("cmapdata");
> ./gfx/thebes/gfxPlatform.cpp:1822:        sTextPerfLog = PR_NewLogModule("textperf");
> ./gfx/thebes/gfxUserFontSet.cpp:32:        sLog = PR_NewLogModule("userfonts");

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8680459 [details] [diff] [review]
(part 4) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

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

Bas: gfx/2d/ has a couple of the few remaining occurrences of PR_LOGGING and MOZ_LOGGING in the tree. Is this necessary/deliberate? Moz2D appears to have its own logging that builds on top of NSPR logging (e.g. see BasicLogger::ShouldOutputMessage()).

Also, the definition of GetGFX2DLog() is protected by PR_LOGGING but the declaration and uses are protected by MOZ_LOGGING. That's clearly wrong, so I'll change the PR_LOGGING on the definition to MOZ_LOGGING unless you think it's ok to remove those checks altogether.

Comment 7

[Eric Rahm [:erahm]]

Comment on attachment 8680445 [details] [diff] [review]
(part 2) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

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

::: gfx/thebes/gfxFT2FontList.cpp
@@ +48,5 @@
>  #include <sys/stat.h>
>  
>  using namespace mozilla;
>  
> +static LazyLogModule sFontInfoLog;

Need to specify the log name:
> static LazyLogModule sFontInfoLog("fontInfoLog");

I'm surprised this compiled, we should file a bug for that.

Comment 8

[Eric Rahm [:erahm]]

Comment on attachment 8680450 [details] [diff] [review]
(part 3) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

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

::: gfx/layers/Layers.cpp
@@ -1821,5 @@
>  void
>  Layer::Log(const char* aPrefix)
>  {
> -  if (!IsLogEnabled())
> -    return;

All of these tests are still important, they check that the "Layer" log module has been set to output the Debug level and avoid potentially expensive string building if not.

@@ -2389,5 @@
> -/*static*/ bool
> -LayerManager::IsLogEnabled()
> -{
> -  MOZ_ASSERT(!!sLog,
> -             "layer manager must be created before logging is allowed");

I think we just need to remove the assert, but keep the function.

::: gfx/layers/Layers.h
@@ -619,5 @@
>    void PostPresent();
>  
>    void BeginTabSwitch();
>  
> -  static bool IsLogEnabled();

As noted above, we should keep this around.

@@ -1608,5 @@
>     * Return display list log.
>     */
>    void GetDisplayListLog(nsCString& log);
>  
> -  static bool IsLogEnabled() { return LayerManager::IsLogEnabled(); }

...and keep this if it's actually used.

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 8680459 [details] [diff] [review]
(part 4) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

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

The code changes look good, I agree the PR_LOGGING check should be changed. If I recall correctly there is (or was) a desire to ship moz2d w/o various libxul dependencies, hence the MOZ_LOGGING bit (which is set via --disable-logging). I'll let Bas confirm that.

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 8680461 [details] [diff] [review]
(part 5) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

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

Code looks good, just a couple minor comments.

::: gfx/thebes/gfxPlatform.cpp
@@ +1809,5 @@
> +    static LazyLogModule sFontInitLog("fontinit");
> +    static LazyLogModule sTextrunLog("textrun");
> +    static LazyLogModule sTextrunuiLog("textrunui");
> +    static LazyLogModule sCmapDataLog("cmapdata");
> +    static LazyLogModule sTextPerfLog("textperf");

AFAICT only "textrun", "textrunui", and "textperf" are used, we might want to just remove the others. That could be a follow up if you don't want to do it here.

@@ +1817,5 @@
> +    case eGfxLog_fontinit:  return sFontInitLog;
> +    case eGfxLog_textrun:   return sTextrunLog;
> +    case eGfxLog_textrunui: return sTextrunuiLog;
> +    case eGfxLog_cmapdata:  return sCmapDataLog;
> +    case eGfxLog_textperf:  return sTextPerfLog;

nit: Is this being updated to proper mozilla style or is that just personal preference? If the latter, maybe leave as the original author intended so that the style conforms w/ the rest of the file.

Comment 11

[Eric Rahm [:erahm]]

Oh and a small follow-up: you might want to update your patch titles to specify the subdirs.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

Updated version that changes the stray PR_LOGGING to MOZ_LOGGING and updates a
relevant comment.

Comment 15

[Nicholas Nethercote [inactive]]

> ::: gfx/thebes/gfxPlatform.cpp
> @@ +1809,5 @@
> > +    static LazyLogModule sFontInitLog("fontinit");
> > +    static LazyLogModule sTextrunLog("textrun");
> > +    static LazyLogModule sTextrunuiLog("textrunui");
> > +    static LazyLogModule sCmapDataLog("cmapdata");
> > +    static LazyLogModule sTextPerfLog("textperf");
> 
> AFAICT only "textrun", "textrunui", and "textperf" are used

No, there are GetLog() calls with the corresponding eGfxLog_* constants for all of them.

Comment 16

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c10a54f5688

Comment 17

[Bas Schouten (:bas.schouten)]

Comment on attachment 8680926 [details] [diff] [review]
(part 4) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/

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

We actually use this to log way more than just 2d. But this'll do for now.

Comment 18

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e22814c4a12037573ff53014026c0f847b1df18
Bug 1219476 (part 1) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c6f3ef493ca3ba9ba3b582bd41167bfab3385d
Bug 1219476 (part 2) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/901035724d856b70b52210e3d9aca7347fdf0f38
Bug 1219476 (part 3) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/72ce6aeb54e409f105227d41aeea87baf9f925e7
Bug 1219476 (part 4) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e8dcc47b7b55e2bfee8e064ab9d7ea14af259e9
Bug 1219476 (part 5) - Replace PRLogModuleInfo usage with LazyLogModule in gfx/. r=erahm.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e22814c4a12
https://hg.mozilla.org/mozilla-central/rev/a1c6f3ef493c
https://hg.mozilla.org/mozilla-central/rev/901035724d85
https://hg.mozilla.org/mozilla-central/rev/72ce6aeb54e4
https://hg.mozilla.org/mozilla-central/rev/9e8dcc47b7b5

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2e22814c4a12
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a1c6f3ef493c
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/901035724d85
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/72ce6aeb54e4
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9e8dcc47b7b5

Comment 21

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion