Closed Bug 1227030 Opened 4 years ago Closed 4 years ago

[Presentation WebAPI] Change the way to access PresentationSessionInfo log module

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: selin, Assigned: cleu)

Details

(Whiteboard: [good-first-bug])

Attachments

(1 file, 4 obsolete files)

PresentationSessionInfo log module is currently maintained and accessible in a static function [1]. Yet it could be refactored as a simple static variable. (Please refer to [2] for example.)

[1] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationSessionInfo.cpp#40-45
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/mdns/libmdns//MDNSResponderOperator.cpp#32
Whiteboard: [good-first-bug]
Assignee: nobody → cleu
Attached patch PresentationSessionInfo.patch (obsolete) — Splinter Review
A patch that changes the static inline function to a direct-accessed file-scoped static variable gLogModuleInfo
Attachment #8693416 - Flags: review?(kechang)
Comment on attachment 8693416 [details] [diff] [review]
PresentationSessionInfo.patch

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

The format of your patch is not correct. Please read [1] for how to create a patch.
If you use git, you can also check [2].

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#I%27m_all_used_to_Git_but_how_can_I_provide_Mercurial-ready_patches
Attachment #8693416 - Flags: review?(kechang)
Attached patch bug1227030.patch (obsolete) — Splinter Review
Patch update : Corrects patch format.
Attachment #8693984 - Flags: review?(kechang)
Attachment #8693416 - Attachment is obsolete: true
Attachment #8693984 - Attachment is obsolete: true
Attachment #8693984 - Flags: review?(kechang)
Attachment #8694059 - Flags: review?(selin)
Comment on attachment 8694059 [details] [diff] [review]
bug1227030 - Change log module to LazyLogModule

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

::: dom/presentation/PresentationSessionInfo.cpp
@@ +37,5 @@
>  using namespace mozilla::dom;
>  using namespace mozilla::services;
>  
> +
> +static LazyLogModule gPRSessionInfoLog("PresentationSessionInfo");

nit: s/gPRSessionInfoLog/gPresentationSessionInfoLog

@@ +43,2 @@
>  #undef LOG
>  #define LOG(...) MOZ_LOG(GetPresentationSessionInfoLog(), mozilla::LogLevel::Error, (__VA_ARGS__))

You should update this line too. Otherwise, it doesn't seem to work.
Attachment #8694059 - Flags: review?(selin)
Attachment #8694059 - Attachment is obsolete: true
Comment on attachment 8694091 [details] [diff] [review]
bug1227030 - Change log module to LazyLogModule and update LOG function in MACRO

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

r=me after the following comments are addressed. Please also push your patch to try to ensure it doesn't break anything.

::: dom/presentation/PresentationSessionInfo.cpp
@@ +37,5 @@
>  using namespace mozilla::dom;
>  using namespace mozilla::services;
>  
> +
> +static LazyLogModule gPRSessionInfoLog("PresentationSessionInfo");

s/gPRSessionInfoLog/gPresentationSessionInfoLog

"Presentation" is more self-explanatory than "PR" to me.

@@ +43,2 @@
>  #undef LOG
> +#define LOG(...) MOZ_LOG(gPRSessionInfoLog, mozilla::LogLevel::Error, (__VA_ARGS__))

Ditto.
Attachment #8694091 - Flags: review?(selin) → review+
(In reply to Sean Lin [:seanlin] from comment #7)
> Comment on attachment 8694091 [details] [diff] [review]
> bug1227030 - Change log module to LazyLogModule and update LOG function in
> MACRO
> 
> Review of attachment 8694091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me after the following comments are addressed. Please also push your patch
> to try to ensure it doesn't break anything.
> 
> ::: dom/presentation/PresentationSessionInfo.cpp
> @@ +37,5 @@
> >  using namespace mozilla::dom;
> >  using namespace mozilla::services;
> >  
> > +
> > +static LazyLogModule gPRSessionInfoLog("PresentationSessionInfo");
> 
> s/gPRSessionInfoLog/gPresentationSessionInfoLog
> 
> "Presentation" is more self-explanatory than "PR" to me.
> 
> @@ +43,2 @@
> >  #undef LOG
> > +#define LOG(...) MOZ_LOG(gPRSessionInfoLog, mozilla::LogLevel::Error, (__VA_ARGS__))
> 
> Ditto.

OK, the variable names have been changed, thanks for your suggestion.
Attachment #8694091 - Attachment is obsolete: true
Attachment #8694126 - Flags: review+
Attachment #8694126 - Attachment description: 0001-bug1227030-Change-log-module-to-LazyLogModule-with-a.patch → Change log module to LazyLogModule with a more self-explantory name
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/097b28578f26
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.