868120 - B2G MMS: Add 'mms.debugging.enabled' pref for MMS debug messages to make sure whether message is sent by SMS or MMS.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Paul Yang [:pyang] (away)]

It is a required feature.
For testing MMS, QA need a way to know whether a message is sent by SMS or MMS. [:ctai] suggests to use debug log for this issue.
As https://bugzilla.mozilla.org/show_bug.cgi?id=811605 mentioned, log level can be raised by implementing an option in gecko and modify user.js to enable it.  Please help to solve by above or some other approaches, thanks.

Comment 1

[Paul Yang [:pyang] (away)]

Hi, please help to work on this issue, thanks.

Comment 2

[Paul Yang [:pyang] (away)]

(In reply to Paul Yang from comment #0)
> It is a required feature.
Sorry, It is a feature request^

> For testing MMS, QA need a way to know whether a message is sent by SMS or
> MMS. [:ctai] suggests to use debug log for this issue.
> As https://bugzilla.mozilla.org/show_bug.cgi?id=811605 mentioned, log level
> can be raised by implementing an option in gecko and modify user.js to
> enable it.  Please help to solve by above or some other approaches, thanks.

Comment 3

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.0

Comment 4

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.1

Comment 5

[Alex Keybl [:akeybl]]

leo+ because QA needs this. Tempted to make this NPOTB, but it does require code change.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 744860 [details] [diff] [review]
Patch v1.1

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

::: dom/mms/src/ril/MmsService.js
@@ +20,5 @@
> +
> +// Read debug setting from pref
> +let debugPref = false;
> +try {
> +  debugPref = Services.prefs.getBoolPref("mms.debugging.enabled");

You'll also need an entry in `modules/libpref/src/init/all.js`

@@ +1379,5 @@
>                              null,
>                              aIsSentSuccess ? DELIVERY_SENT : DELIVERY_ERROR,
>                              aIsSentSuccess ? null : DELIVERY_STATUS_ERROR,
>                              function notifySetDeliveryResult(aRv, aDomMessage) {
>          debug("Marking the delivery state/staus is done. Notify sent or failed.");

nit: since you're here, please also guard all debug messages with DEBUG flag.

Comment 7

[Gene Lian (I already quit Mozilla)]

Comment on attachment 744860 [details] [diff] [review]
Patch v1.1

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

A drive-by review.

::: dom/mms/src/ril/MmsService.js
@@ +15,5 @@
>  
>  const RIL_MMSSERVICE_CONTRACTID = "@mozilla.org/mms/rilmmsservice;1";
>  const RIL_MMSSERVICE_CID = Components.ID("{217ddd76-75db-4210-955d-8806cd8d87f9}");
>  
> +var DEBUG = false;

nit: s/var/let.

@@ +17,5 @@
>  const RIL_MMSSERVICE_CID = Components.ID("{217ddd76-75db-4210-955d-8806cd8d87f9}");
>  
> +var DEBUG = false;
> +
> +// Read debug setting from pref

nit: all the comment should follow the format:

//{a_white_space}{upper_case_letter}...{a_period}

so:

// Read debug setting from pref.

@@ +18,5 @@
>  
> +var DEBUG = false;
> +
> +// Read debug setting from pref
> +let debugPref = false;

why not just using:

  let DEBUG = false;

so that you don't need an extra |debugPref|.

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

(In reply to Gene Lian [:gene] from comment #7)
> Comment on attachment 744860 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 744860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A drive-by review.
> 
> ::: dom/mms/src/ril/MmsService.js
> @@ +15,5 @@
> >  
> >  const RIL_MMSSERVICE_CONTRACTID = "@mozilla.org/mms/rilmmsservice;1";
> >  const RIL_MMSSERVICE_CID = Components.ID("{217ddd76-75db-4210-955d-8806cd8d87f9}");
> >  
> > +var DEBUG = false;
> 
> nit: s/var/let.
> 
> @@ +17,5 @@
> >  const RIL_MMSSERVICE_CID = Components.ID("{217ddd76-75db-4210-955d-8806cd8d87f9}");
> >  
> > +var DEBUG = false;
> > +
> > +// Read debug setting from pref
> 
> nit: all the comment should follow the format:
> 
> //{a_white_space}{upper_case_letter}...{a_period}
> 
> so:
> 
> // Read debug setting from pref.
> 
> @@ +18,5 @@
> >  
> > +var DEBUG = false;
> > +
> > +// Read debug setting from pref
> > +let debugPref = false;
> 
> why not just using:
> 
>   let DEBUG = false;
> 
> so that you don't need an extra |debugPref|.
debugPref comes from getBoolPref. And it might be fail to get.

Comment 9

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.2

Comment 10

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.3

Comment 11

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.4

Comment 12

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

To enable mms debug log, you need to add one line, 'pref("mms.debugging.enabled", true);', into /system/b2g/defaults/pref/user.js in device. Then reboot device or restart b2g service.

detailed step:
adb remount
adb pull /system/b2g/defaults/pref/user.js user.js
[update user.js with adding 'pref("mms.debugging.enabled", true);']
adb push user.js /system/b2g/defaults/pref/user.js
adb shell reboot

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 745391 [details] [diff] [review]
Patch v1.4

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

Can't really tell what's the relationship between the commit summary and the patch body. Could you re-title it to some thing like "add 'mms.debugging.enabled' pref for MMS debug messages"?

Comment 14

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Patch v1.5

Comment 15

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 745925 [details] [diff] [review]
Patch v1.5

Re-titled

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/bdaaedfc41f7

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bdaaedfc41f7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/65049cb236f4