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

RESOLVED FIXED in Firefox 45

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: seanlin, Assigned: Lenzak)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [good-first-bug])

Attachments

(1 attachment, 4 obsolete attachments)

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
(Assignee)

Comment 1

2 years ago
Created attachment 8693416 [details] [diff] [review]
PresentationSessionInfo.patch

A patch that changes the static inline function to a direct-accessed file-scoped static variable gLogModuleInfo
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 3

2 years ago
Created attachment 8693984 [details] [diff] [review]
bug1227030.patch

Patch update : Corrects patch format.
(Assignee)

Updated

2 years ago
Attachment #8693984 - Flags: review?(kechang)
Attachment #8693416 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 4

2 years ago
Created attachment 8694059 [details] [diff] [review]
bug1227030 - Change log module to LazyLogModule
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)
(Assignee)

Updated

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

Comment 6

2 years ago
Created attachment 8694091 [details] [diff] [review]
bug1227030 - Change log module to LazyLogModule and update LOG function in MACRO
Attachment #8694091 - Flags: review?(selin)
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+
(Assignee)

Comment 8

2 years ago
Created attachment 8694126 [details] [diff] [review]
Change log module to LazyLogModule with a more self-explantory name

(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+
(Assignee)

Updated

2 years ago
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
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/097b28578f26
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.