Closed Bug 1254888 Opened 8 years ago Closed 8 years ago

[Presentation API] Add NSPR_LOG to dom/presentation.

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

Details

Attachments

(2 files, 9 obsolete files)

2.32 KB, patch
schien
: review+
Details | Diff | Splinter Review
7.58 KB, patch
mantaroh
: review+
Details | Diff | Splinter Review
In order to debuggable, Add the NSPR_LOG. LogModule is [Presentation]. 
For first step, I added PRLogModuleInfo in PresentationService.
Attachment #8728283 - Attachment is obsolete: true
Attachment #8728284 - Flags: review?(schien)
Sorry, Previous patch contain the unnecessary print statement.
Attachment #8728284 - Attachment is obsolete: true
Attachment #8728284 - Flags: review?(schien)
Attachment #8728289 - Flags: review?(schien)
Attachment #8728306 - Flags: review?(schien)
Comment on attachment 8728289 [details] [diff] [review]
Part1.Add_NSPR_Log_to_PresentationAPI

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

You can use the mozilla::LazyLogModule to save some code, see example in https://dxr.mozilla.org/mozilla-central/source/dom/base/ThirdPartyUtil.cpp#26
Attachment #8728289 - Flags: review?(schien) → review-
Comment on attachment 8728306 [details] [diff] [review]
Part2. Add server socket log of PresentationAPI.

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

Can you help change the existing PresentationSessionInfo log to your logging module?
https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationSessionInfo.cpp#42
Thanks!

::: dom/presentation/PresentationSessionInfo.cpp
@@ +638,5 @@
> +  int32_t port;
> +  nsresult rv = aTransport->GetPort(&port);
> +  if (!NS_WARN_IF(NS_FAILED(rv))) {
> +    MOZ_LOG(PresentationService::GetPresentationLog(), LogLevel::Debug,
> +            ("[%s] receive from port[%ld]\n", __func__, port));

[%ld] => [%d]

::: dom/presentation/PresentationSessionTransport.cpp
@@ +363,5 @@
>  NS_IMETHODIMP
>  PresentationSessionTransport::Close(nsresult aReason)
>  {
> +  MOZ_LOG(PresentationService::GetPresentationLog(), LogLevel::Debug,
> +          ("[%s]: reason[d]\n", __func__, aReason));

[d] => [%x]

@@ +407,5 @@
>                                                  int64_t aProgress,
>                                                  int64_t aProgressMax)
>  {
> +  MOZ_LOG(PresentationService::GetPresentationLog(), LogLevel::Debug,
> +          ("[%s]: aStatus[%d]\n", __func__, aStatus));

use %x to format status code will be easier to read.

@@ +458,5 @@
>                                              nsISupports* aContext,
>                                              nsresult aStatusCode)
>  {
> +  MOZ_LOG(PresentationService::GetPresentationLog(), LogLevel::Debug,
> +          ("[%s]: aStatusCode\n", __func__, aStatusCode));

aStatusCode is not used in format string.

@@ +507,5 @@
>      return rv;
>    }
>  
> +  MOZ_LOG(PresentationService::GetPresentationLog(), LogLevel::Debug,
> +          ("[%s]: data[%s]\n", __func__, data.get()));

logging each message seems extreme to me and I'm worry about the performance.
Attachment #8728306 - Flags: review?(schien)
We have log to print out the offer and answer in TCPPresentationServer.js [1], do it suit for your needs for debugging?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/provider/TCPPresentationServer.js#12
Flags: needinfo?(mantaroh)
Thanks schien.

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #6)
> We have log to print out the offer and answer in TCPPresentationServer.js
> [1], do it suit for your needs for debugging?
I would like to know the status of PresentationSession's Server socket, because I would like to confirm that the handshaking information is corresponded.
So I added the NSPR_LOG to PresentationSession.

I think that this modification have a too small influence on performance, because this modification increase one condition statement if not set the environment parameter(NSPR_LOG_MODULES).[1]

[1] https://dxr.mozilla.org/mozilla-central/rev/af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86/xpcom/base/Logging.h#191
Flags: needinfo?(mantaroh)
Attached patch Part1.Add_Logging_macro. (obsolete) — Splinter Review
Thanks.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #4)
> You can use the mozilla::LazyLogModule to save some code, see example in
> https://dxr.mozilla.org/mozilla-central/source/dom/base/ThirdPartyUtil.cpp#26
OK. I fixed to using LazyLogModule.
Attachment #8728289 - Attachment is obsolete: true
Attachment #8728822 - Flags: review?(schien)
Thanks schien. 

I addressed the format specifier, and removed the logging statement from onDataAvailable().
Could you please confirm the patch?
Attachment #8728306 - Attachment is obsolete: true
Attachment #8728825 - Flags: review?(schien)
Comment on attachment 8728822 [details] [diff] [review]
Part1.Add_Logging_macro.

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

::: dom/presentation/PresentationLog.h
@@ +9,5 @@
> +
> +//
> +// NSPR_LOG_MODULES=Presentation:5
> +//
> +static mozilla::LazyLogModule gPresentationLog("Presentation");

You shouldn't put static variable in header file. Just put |mozilla::LazyLogModul gPresentationLog("Presentation");| in PresentationService.cpp, put |extern mozilla::LazyLogModule aPresentationLog| in PresentationLog.h, and put |#include "PresentationLog.h" in other .cpp files.

HttpLog is a good example to follow: https://dxr.mozilla.org/mozilla-central/search?q=ghttplog+path%3Ahttp&redirect=false&case=false

@@ +12,5 @@
> +//
> +static mozilla::LazyLogModule gPresentationLog("Presentation");
> +
> +#undef ERR
> +#define ERR(arg, ...) MOZ_LOG(gPresentationLog, mozilla::LogLevel::Error, ("Presentation(%p)::%s: " arg, this, __func__, ##__VA_ARGS__))

You don't need to create prefix manually, and I'll suggest to only create "DEBUG" macro since that is the only one being used.
Attachment #8728822 - Flags: review?(schien)
Comment on attachment 8728825 [details] [diff] [review]
Part2. Add log to PresentationSessionInfo and Transport.

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

Please help migrate the original LogModule in PresentationSessionInfo to your logging macro. Thanks!
https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationSessionInfo.cpp#42

::: dom/presentation/PresentationSessionInfo.cpp
@@ +637,5 @@
>  {
> +  int32_t port;
> +  nsresult rv = aTransport->GetPort(&port);
> +  if (!NS_WARN_IF(NS_FAILED(rv))) {
> +    DEBUG("receive from port[%d]\n",port);

nit: space between ',' and 'port'

::: dom/presentation/PresentationSessionTransport.cpp
@@ +154,5 @@
>    if (serverHost.IsEmpty()) {
>      return NS_ERROR_INVALID_ARG;
>    }
>  
> +  DEBUG("ServerHost[%s],ServerPort[%d]\n",serverHost.get(), serverPort);

nit: space between ',' and 'serverHost'
Attachment #8728825 - Flags: review?(schien)
Thanks schien!

I addressed your comment #10. move the LogModule definition to the |PresentationService|.
Could you confirm this patch again?
Attachment #8728822 - Attachment is obsolete: true
Attachment #8729356 - Flags: review?(schien)
Attachment #8728825 - Attachment is obsolete: true
Attachment #8729358 - Flags: review?(schien)
Sorry I changed the DEBUG->PRES_DEBUG.
(Previous modification causes the override issue in Linux/Mac.)
Attachment #8729356 - Attachment is obsolete: true
Attachment #8729356 - Flags: review?(schien)
Attachment #8729389 - Flags: review?(schien)
Attachment #8729358 - Attachment is obsolete: true
Attachment #8729358 - Flags: review?(schien)
Attachment #8729397 - Flags: review?(schien)
Comment on attachment 8729389 [details] [diff] [review]
Part1.Add logging macro to dom/presentation.

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

lgtm!
Attachment #8729389 - Flags: review?(schien) → review+
Comment on attachment 8729397 [details] [diff] [review]
Part2. Add log to PresentationSessionInfo and Transport.

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

r+ with my comment addressed.

::: dom/presentation/PresentationSessionInfo.cpp
@@ +98,5 @@
>  
>  NS_IMETHODIMP
>  PresentationNetworkHelper::OnError(const nsACString & aReason)
>  {
> +  ERR("PresentationNetworkHelper::OnError: %s",

Use PRES_DEBUG is enough I think, and you can remove the "ERR" definition from PresentationLog.h.
Attachment #8729397 - Flags: review?(schien) → review+
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Thanks schien.

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #17)
> Comment on attachment 8729397 [details] [diff] [review]
> Part2. Add log to PresentationSessionInfo and Transport.
> 
> Review of attachment 8729397 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with my comment addressed.
> 
I fixed above your comment.
This patch is carrying forward r+ from schien.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8078ffd56f1
Attachment #8729397 - Attachment is obsolete: true
Attachment #8730042 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bccc70a79ae1
https://hg.mozilla.org/mozilla-central/rev/06033c2f79c4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: