970208 - make WebappManager.jsm output debug statements only if MOZ_DEBUG is set

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P3)

Description

[Myk Melez [:myk] [@mykmelez]]

Webapps.jsm only outputs debug statements to the console if MOZ_DEBUG is set, which makes console output cleaner in optimized builds while still enabling developers to see debugging messages in debug builds.  WebappManager.jsm should do the same.

Notwithstanding the recent change from "dump" to "log" in that file, if we're going to restrict logging to MOZ_DEBUG, then we should probably call it "debug", as we do in Webapps.jsm, making it something like:

  function debug(aMessage) {
  #ifdef MOZ_DEBUG
    dump(aMessage);
  #endif
  }

Note that we don't need to append a newline character to the message because WebappManager.jsm is Android-only, and Android automagically separates logcat entries with newlines.

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: Only print debug messages when in debug mode

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8374029 [details] [diff] [review]
Only print debug messages when in debug mode

You'll also need to modify mobile/android/modules/moz.build to add WebappManager.jsm to EXTRA_PP_JS_MODULES instead of EXTRA_JS_MODULES so it gets preprocessed.

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: Only print debug messages when in debug mode

updated code based on feedback

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8374157 [details] [diff] [review]
Only print debug messages when in debug mode

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

Looks good, although I'd prefer we rename this to *debug* (despite the additional changes that would require).

::: mobile/android/modules/WebappManager.jsm
@@ +33,5 @@
>    // have the INFO level of severity instead of the ERROR level.  And we don't
>    // append a newline character to the end of the message because *dump* spills
>    // into the Android native logging system, which strips newlines from messages
>    // and breaks messages into lines automatically at display time (i.e. logcat).
> +#ifdef MOZ_DEBUG  

Nit: trailing whitespace.

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: Only print debug messages when in debug mode

Changed log to debug

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: Only print debug messages when in debug mode

Oops - forgot to address nits!

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8374768 [details] [diff] [review]
Only print debug messages when in debug mode

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

This is missing the change to mobile/android/modules/moz.build again!

::: mobile/android/modules/WebappManager.jsm
@@ +33,5 @@
>    // have the INFO level of severity instead of the ERROR level.  And we don't
>    // append a newline character to the end of the message because *dump* spills
>    // into the Android native logging system, which strips newlines from messages
>    // and breaks messages into lines automatically at display time (i.e. logcat).
> +#ifdef MOZ_DEBUG  

Nit: trailing whitespace.

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: Only print debug messages when in debug mode

Added back moz build file.  Don't know how that one slipped through the gaps!

Comment 9

[Martyn Haigh (:mhaigh)]

Attachment: Only print debug messages when in debug mode

...and fixed the nit too!

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8377157 [details] [diff] [review]
Only print debug messages when in debug mode

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

Looks good!

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 8377157 [details] [diff] [review]
Only print debug messages when in debug mode

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

To be honest, I prefer hiding these behind prefs so that web/app-developers/QA can turn it on/off. But I'll let you guys make that call.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

(In reply to Wesley Johnston (:wesj) from comment #11)
> To be honest, I prefer hiding these behind prefs so that
> web/app-developers/QA can turn it on/off. But I'll let you guys make that
> call.

Ideally we'd simply log messages with a level (debug, warning, error, etc.) and then let the system/user decide which messages to write to log files or display.  Perhaps such a facility already exists in Fennec?  In the meantime, I've pushed this patch to inbound and filed bug 979907 on logging messages with a level.  Let's follow up on better logging in that bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/799bd9fbd045

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/799bd9fbd045