Closed Bug 1219476 Opened 4 years ago Closed 4 years ago

Replace PRLogModuleInfo usage with LazyLogModule in gfx

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: erahm, Assigned: njn)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(5 files, 3 obsolete files)

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");
Whiteboard: [gfx-noted]
Assignee: nobody → n.nethercote
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.
Attachment #8680459 - Flags: feedback?(bas)
Attachment #8680444 - Flags: review?(erahm) → review+
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.
Attachment #8680445 - Flags: review?(erahm) → review-
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.
Attachment #8680450 - Flags: review?(erahm) → review-
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.
Attachment #8680459 - Flags: review?(erahm) → review+
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.
Attachment #8680461 - Flags: review?(erahm) → review+
Oh and a small follow-up: you might want to update your patch titles to specify the subdirs.
Attachment #8680445 - Attachment is obsolete: true
Attachment #8680921 - Flags: review?(erahm) → review+
Attachment #8680450 - Attachment is obsolete: true
Updated version that changes the stray PR_LOGGING to MOZ_LOGGING and updates a
relevant comment.
Attachment #8680926 - Flags: feedback?(bas)
Attachment #8680459 - Attachment is obsolete: true
Attachment #8680459 - Flags: feedback?(bas)
Attachment #8680923 - Flags: review?(erahm) → review+
> ::: 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 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.
Attachment #8680926 - Flags: feedback?(bas) → feedback+
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.
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.