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)
Core
DOM: Core & HTML
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.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8728283 -
Attachment is obsolete: true
Attachment #8728284 -
Flags: review?(schien)
Assignee | ||
Comment 2•8 years ago
|
||
Sorry, Previous patch contain the unnecessary print statement.
Attachment #8728284 -
Attachment is obsolete: true
Attachment #8728284 -
Flags: review?(schien)
Attachment #8728289 -
Flags: review?(schien)
Assignee | ||
Comment 3•8 years ago
|
||
Added the server socket log of data transport. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a446f96a091b
Assignee | ||
Updated•8 years ago
|
Attachment #8728306 -
Flags: review?(schien)
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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)
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
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 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8728825 -
Attachment is obsolete: true
Attachment #8729358 -
Flags: review?(schien)
Assignee | ||
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8729358 -
Attachment is obsolete: true
Attachment #8729358 -
Flags: review?(schien)
Attachment #8729397 -
Flags: review?(schien)
Comment 16•8 years ago
|
||
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 17•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Assignee | ||
Comment 18•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bccc70a79ae1 https://hg.mozilla.org/integration/mozilla-inbound/rev/06033c2f79c4
Keywords: checkin-needed
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bccc70a79ae1 https://hg.mozilla.org/mozilla-central/rev/06033c2f79c4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•