Closed Bug 867273 Opened 7 years ago Closed 7 years ago

Force logging on in files using MOZ_MTLOG

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: ekr, Assigned: snandaku)

Details

(Whiteboard: [WebRTC][blocking-webrtc-][qa-])

Attachments

(1 file, 7 obsolete files)

No description provided.
Attachment #743730 - Flags: review?(rjesup)
Attachment #743730 - Flags: review?(rjesup) → review+
Attachment #743730 - Attachment is obsolete: true
Whiteboard: [WebRTC][blocking-webrtc-]
Assignee: ekr → snandaku
Try link with the patch applied: 
  https://tbpl.mozilla.org/?tree=Try&rev=bc74fb61af57
Attachment #743782 - Attachment is obsolete: true
Attachment #751359 - Flags: review?(ekr)
Comment on attachment 751359 [details] [diff] [review]
Force PR Logging in Release builds for MOZ_MTLOG*

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

I don't think this is quite ready?

::: media/mtransport/logging.h
@@ +8,5 @@
>  
>  #ifndef logging_h__
>  #define logging_h__
>  
> +#if defined(MOZ_LOGGING)

Where is MOZ_LOGGING set?

::: media/mtransport/transportlayerdtls.cpp
@@ +20,5 @@
>  #include "nsNetCID.h"
>  #include "nsComponentManagerUtils.h"
>  #include "nsServiceManagerUtils.h"
>  
> +#include "logging.h"

Why did this have to move?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Original author: ekr@rtfm.com
>  
> +#ifdef MOZ_LOGGING

This seems like not what we want. The idea here is that MOZ_MTLOG always
logs and that you don't need to do something in every individual file.

@@ +43,5 @@
>  #include "runnable_utils.h"
>  
>  using namespace mozilla;
>  
> +// Logging context

Why did this move? There isn't a dependency between this and the following lines.
Attachment #751359 - Flags: review?(ekr) → review-
Comment on attachment 751359 [details] [diff] [review]
Force PR Logging in Release builds for MOZ_MTLOG*

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

::: media/mtransport/logging.h
@@ +8,5 @@
>  
>  #ifndef logging_h__
>  #define logging_h__
>  
> +#if defined(MOZ_LOGGING)

It is defined in configure and we use the same logic in signaling to FORCE log. Once i had this line FORCE_PR_LOG was visible in the release builds.

::: media/mtransport/transportlayerdtls.cpp
@@ +20,5 @@
>  #include "nsNetCID.h"
>  #include "nsComponentManagerUtils.h"
>  #include "nsServiceManagerUtils.h"
>  
> +#include "logging.h"

This move is not critical one but it was more to follow the convention in other files.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Original author: ekr@rtfm.com
>  
> +#ifdef MOZ_LOGGING

Unless i added this line before #include prlog.h in MediaPipeline.cpp, logging wasn't activated in the release builds.
This doesn't seem like quite what we want.

The overall objective is that if you include logging.h and do MOZ_MTLOG
it should work all the time, not that you have to do some funny include
juggling. So, I think that means we have a number of options:

1. Require that PR_LOGGING be set in the build/makefile and then have
MOZ_MTLOG check it.
2. Require that MOZ_MTLOG be included before prlog.h and have it set
the force logging value (and have it check that prlog.h hasn't been
included first.) It should also includ prlog.h itself, with the idea
being that it replaces prlog.h.

Neither of these is great, but IMO the second is better. Please take
a crack at it.

I don't see any reason for this to be conditional on MOZ_LOGGING.
Attachment #751359 - Attachment is obsolete: true
Attachment #751944 - Attachment is obsolete: true
Comment on attachment 751945 [details] [diff] [review]
Force PR_LOG in production builds for MOZ_MTLOG

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

Try Link : https://tbpl.mozilla.org/?tree=Try&rev=9f98127caf0f
Attachment #751945 - Flags: review?(ekr)
Comment on attachment 751945 [details] [diff] [review]
Force PR_LOG in production builds for MOZ_MTLOG

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

r+ with fixes below and provided that the try push succeeds.

::: media/mtransport/nriceresolver.cpp
@@ +40,5 @@
>  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
>  
> +#include "logging.h"

Please be consistent about whitespace.

::: media/mtransport/transportflow.cpp
@@ +11,1 @@
>  #include <prlog.h>

Please remove prlog.h includes in all files but logging.h

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +50,4 @@
>  // Logging context
>  MOZ_MTLOG_MODULE("mediapipeline")
>  
> +

Spurious whitespace.
Attachment #751945 - Flags: review?(ekr) → review+
Attachment #751945 - Attachment is obsolete: true
Attachment #751959 - Attachment is obsolete: true
Updated patch with nits addressed and try link for the same is : https://tbpl.mozilla.org/?tree=Try&rev=49144b6af283
Attachment #751960 - Attachment is obsolete: true
Comment on attachment 751989 [details] [diff] [review]
Force PR Logging in Release builds for MOZ_MTLOG*

Taking R+ from Ekr forward.
Attachment #751989 - Flags: checkin?(ethanhugg)
Attachment #751989 - Flags: checkin?(ethanhugg) → checkin+
https://hg.mozilla.org/mozilla-central/rev/493eca4ee162
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Whiteboard: [WebRTC][blocking-webrtc-] → [WebRTC][blocking-webrtc-][qa-]
You need to log in before you can comment on or make changes to this bug.