Gene Lian (I already quit Mozilla) Reporter
	Description • 11 years ago

+++ This bug was initially created as a clone of Bug #935060 +++

Gecko should be able to send an empty SMS. Please see some details at bug 935060, comment #19.

Hi, ndel314, are you interested in this also? :)

	Nicolás Del Piano Assignee
	Comment 1 • 11 years ago

(In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #0)
> +++ This bug was initially created as a clone of Bug #935060 +++
> 
> Gecko should be able to send an empty SMS. Please see some details at bug
> 935060, comment #19.
> 
> Hi, ndel314, are you interested in this also? :)

Yes, I'm interested :).

Thank you Gene. This is an important issue to resolve #935060.

	Gene Lian (I already quit Mozilla) Reporter
	Comment 2 • 11 years ago

Nice! Please let me know if you encounter any issues. I'd be glad to guide.

	Nicolás Del Piano Assignee
	Comment 3 • 11 years ago

Hello Gene!

I encountered that is not allowed to send a null text according to the spec: http://www.w3.org/2012/sysapps/messaging/#methods-1 . 
Is there any problem if we change that behaviour?

So, the code that we are looking for is in gecko/dom/mobilemessage/src/gonk/MmsService.js, am I right?

Cheers!

	Gene Lian (I already quit Mozilla) Reporter
	Comment 4 • 11 years ago

> I encountered that is not allowed to send a null text according to the spec:
> http://www.w3.org/2012/sysapps/messaging/#methods-1 . 
> Is there any problem if we change that behaviour?

That spec is still under construction and can be modified, which means it can be wrong. Don't worry about that if the RIL is allowed to send an empty text by its nature.

> 
> So, the code that we are looking for is in
> gecko/dom/mobilemessage/src/gonk/MmsService.js, am I right?

Please see the bottom of bug 935060, comment #17. The SMS logic is still located in the RadioInterfaceLayer.js (one day we'll move it out to an independent SmsService.js like MMS).

	Nicolás Del Piano Assignee
	Comment 5 • 11 years ago

Hi Gene!

Well, I think I fixed it.

I spent a lot of time analyzing the RadioInterfaceLayer.js and MobileMessageDB.js, and I encountered very interesting things.
The problem is what I thought at the begining: the fragmentText encoding function doesn't do what is expected for empty messages.

So, fragmentText7Bit function, at the end has this code:

if (len) {
      ret.push({
        body: body,
        encodedBodyLength: len,
      });
    }

And this is wrong, because if the message is blank, the len is 0 (you can see what the function does[1]), and doesn't add anything to "ret".

So, removing the "if" we can send empty messages. At least I got the same behaviour as the non-empty messages (I'm testing with an ARM emulator).
We have to add a similar change (adding an empty case) for fragmentTextUCS2 function to fix this problem for this case of encoding.

What do you think of this?

Regards!

[1] : http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#3191

	Julien Wajsberg [:julienw]
	Comment 6 • 11 years ago

Hey "ndel" (you should edit your Bugzilla account information to add your name ;) ), can you provide a patch ?

Here is how I do for Gecko:

  hg qnew -e bug_number-meaningful-name

=> add the name of this bug in the first line, with "r=gene" at the end
(which gives: "Bug 957084 - Unable to send empty SMS' (Gecko side) r=gene)

If you do any changes, you can run

  hg qrefresh

Then to export it to a patch, you can do:

  hg export tip > name_of_the_patch.patch

and attach the patch to this bug, requesting review from Gene.

See [1] for help on how to configure your Hg installation for proper use for us, and [2] for help on how to configure and use the mercurial MQ extension

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2] http://mercurial.selenic.com/wiki/MqExtension

Also, you'll probably need to update the existing mochitests, see [3] for more information on how to run them. Maybe Gene will be able to give more information ;)

[3] https://developer.mozilla.org/en-US/docs/Mochitest

Also, don't hesitate to ask on IRC as there's always some helpful people!

	Gene Lian (I already quit Mozilla) Reporter
	Comment 7 • 11 years ago

Awesome! ndel! I think that's exactly the root cause! :) Please go ahead to work out a patch and thanks for Julien's guide.

	Nicolás Del Piano Assignee
	Comment 8 • 11 years ago

Thank you both :).
I'm working on the patch, but I'm having some problems doing it.
I don't know why git is too slow when cloning.
Is there a way to clone hg, but only gecko part?

Regards!

	Julien Wajsberg [:julienw]
	Comment 9 • 11 years ago

Nicolas, I'd like to know your setup.

* either you use mercurial and you run:

  hg clone http://hg.mozilla.org/mozilla-central/

and then you can use my guide from comment 6.

* either you use the git repository that is in your B2G directory and you can create a branch, commit your changes, and run this when you're ready:

  git format-patch -U8 upstream

But I don't know very well the git workflow for Gecko. And please adjust to what you prefer, the goal is to create a patch with your name in it.

	Nicolás Del Piano Assignee
	Comment 12 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Nicolas, I'd like to know your setup.
> 
> * either you use mercurial and you run:
> 
>   hg clone http://hg.mozilla.org/mozilla-central/
> 
> and then you can use my guide from comment 6.
> 
> * either you use the git repository that is in your B2G directory and you
> can create a branch, commit your changes, and run this when you're ready:
> 
>   git format-patch -U8 upstream
> 
> But I don't know very well the git workflow for Gecko. And please adjust to
> what you prefer, the goal is to create a patch with your name in it.

Julien,

I could do it with git :). Thank you for your help and motivation!

I'll wait for the response.

Cheers!

	Julien Wajsberg [:julienw]
	Comment 13 • 11 years ago

Comment on attachment 8359754 [details] [diff] [review]
Bug 957084 - Unable to send empty SMS' (Gecko side) r=gene

Requesting review from Gene.

Thanks Nicolas !

	Gene Lian (I already quit Mozilla) Reporter
	Comment 14 • 11 years ago

Comment on attachment 8359754 [details] [diff] [review]
Bug 957084 - Unable to send empty SMS' (Gecko side) r=gene

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

This patch is nice! But I'd like to go another round because it's your first patch. ;) Please fix my comments and ask for the review again. Should be able to give you review+ after the next review. Well done!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3255,5 @@
>          len += inc;
>        }
>      }
>  
> +    // Bug 957084, if the message is empty, we need to push the empty

Don't need to put the bug number in the patch. Just leave:

// If the message is empty, we need to push the empty.

(note: add a period at the end of each comment)

@@ +3260,5 @@
> +    // message to ret too.
> +    ret.push({
> +      body: body,
> +      encodedBodyLength: len,
> +    });

Could you do something like what you did in _fragmentTextUCS2 to make it early return? like:

// If the message is empty, we need to push the empty message to ret.
if (text.length == 0) {
  // ...
  return ret;
}

@@ +3278,5 @@
>     */
>    _fragmentTextUCS2: function(text, segmentChars) {
>      let ret = [];
> +
> +    // Bug 957084, if the message is empty, we push the empty message to ret.

Ditto. Just leave:

// If the message is empty, we push the empty message to ret.

@@ +3279,5 @@
>    _fragmentTextUCS2: function(text, segmentChars) {
>      let ret = [];
> +
> +    // Bug 957084, if the message is empty, we push the empty message to ret.
> +    if (!text.length) {

This is fine, but I'd prefer:

if (text.length == 0)

@@ +3283,5 @@
> +    if (!text.length) {
> +      ret.push({
> +        body: text,
> +        encodedBodyLength: text.length,
> +      });      

Please remove the extra spaces and call:

return;

to do a early return.

	Nicolás Del Piano Assignee
	Comment 15 • 11 years ago

Hey Gene,

I fixed the issues :).

Regards!

	Gene Lian (I already quit Mozilla) Reporter
	Comment 17 • 11 years ago

Comment on attachment 8360385 [details] [diff] [review]
Bug 957084 - Unable to send empty SMS' (Gecko side) r=gene

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

Well done! Please put "checkin-needed" in the keywords field then someone will help you land your patch.

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/658e616bd231

Thanks for the patch, Nicolás! Is this something we could write tests for? Also, I tweaked your commit message a bit before landing. In general, the commit message should briefly describe what the patch is doing rather than restating the bug title.

	Nicolás Del Piano Assignee
	Comment 19 • 11 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #18)
> https://hg.mozilla.org/integration/b2g-inbound/rev/658e616bd231
> 
> Thanks for the patch, Nicolás! Is this something we could write tests for?
> Also, I tweaked your commit message a bit before landing. In general, the
> commit message should briefly describe what the patch is doing rather than
> restating the bug title.

Hi Ryan!

Yes, the commit message was a little bit annoying, now it's great :).

I think that if we write a test, it will be a send of an empty message, and as this patch is part of another bug (https://bugzilla.mozilla.org/show_bug.cgi?id=935060), we could leave the tests to this one. What do you think? 

Thanks for your advices :).

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 11 years ago

Backed out for marionette-webapi failures (speaking of tests...).
https://hg.mozilla.org/integration/b2g-inbound/rev/8ee9054a5b59

https://tbpl.mozilla.org/php/getParsedLog.php?id=33043720&tree=B2g-Inbound

As for tests to write for this bug, that's really for you to discuss with Gene :)

	Nicolás Del Piano Assignee
	Comment 21 • 11 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #20)
> Backed out for marionette-webapi failures (speaking of tests...).
> https://hg.mozilla.org/integration/b2g-inbound/rev/8ee9054a5b59
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=33043720&tree=B2g-Inbound

That means that the patch has an error? Have I to fix something or make a test?

> As for tests to write for this bug, that's really for you to discuss with
> Gene :)

Yes, you're right, the question goes to Gene then :).

	Julien Wajsberg [:julienw]
	Comment 22 • 11 years ago

(In reply to Nicolás Del Piano from comment #21)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #20)
> > Backed out for marionette-webapi failures (speaking of tests...).
> > https://hg.mozilla.org/integration/b2g-inbound/rev/8ee9054a5b59
> > 
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=33043720&tree=B2g-Inbound
> 
> That means that the patch has an error? Have I to fix something or make a
> test?

Or the test has an error ;)

You'll need to run them locally to figure this out.

> 
> > As for tests to write for this bug, that's really for you to discuss with
> > Gene :)
> 
> Yes, you're right, the question goes to Gene then :).

Needinfo Gene who will help you both fixing the failure and adding a new test (if necessary).

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 23 • 11 years ago

It seems that the expected segment count of empty "" SMS text body shall take 1 segment instead of 0:
http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_getsegmentinfofortext.js#115

	Gene Lian (I already quit Mozilla) Reporter
	Comment 24 • 11 years ago

Yeap! Like Bevis said, we should consider the empty message as one segment. You need to fix the test case as well. Sorry I missed that bit also.

	Gene Lian (I already quit Mozilla) Reporter
	Comment 25 • 11 years ago

To make sure your codes don't break any existing functions, you also need to learn how to commit your patches to the Mozilla Try Server [1] to run thorough the automatic tests. You can see your testing results at [2] to make sure everything is green before asking for a check-in (some non-green items might be due to random bugs, which can be ignored). Btw, although you cannot commit the codes to the central for now, you can commit them to the Try Server, where the former needs level-3 permission and the latter only needs level-1 permission. Therefore, please apply for your level-1 permission while you're working on this bug because that application usually takes some time and some paper work to be done [3].

Don't hesitate to ask questions when you're stuck. Thanks!

[1] https://wiki.mozilla.org/Build:TryServer
[2] https://tbpl.mozilla.org/?tree=Try
[3] http://www.mozilla.org/hacking/commit-access-policy/

	Nicolás Del Piano Assignee
	Comment 26 • 11 years ago

Thank you all!

I have a few questions about.

How can I run the tests locally?

If I run my tests locally, with the error test fixed, have I to attach it like a patch or what is the correct way?

How can I request level-1 permission? (I read Gene's link number 3 but I can't figure out correctly, have I to report a bug to do that?).

Sorry for the nuisance!

Regards!

	Julien Wajsberg [:julienw]
	Comment 27 • 11 years ago

I don't know well how marionette tests are run, and I think that you'll need to build an emulator. That said, the issue looks quite simple to fix here, you'll need someone to push to try for you until you can do that yourself as Gene suggested.

I see also these errors:

09:53:16     INFO -  AssertionError: See errors below and more information in Bug 880643
09:53:16     INFO -  RadioInterfaceLayer.js: line 3322, col 21, Use '===' to compare with '0'.
09:53:16     INFO -  RadioInterfaceLayer.js: line 3400, col 21, Use '===' to compare with '0'.
09:53:16     INFO -  TEST-UNEXPECTED-FAIL | test_ril_code_quality.py test_ril_code_quality.TestRILCodeQuality.test_RadioInterfaceLayer | See errors above and more information in Bug 880643

Reading the code in the patch, you need to replace "==" with "===", or simply use "if (!text.length)" (if we're sure that "text" is a string).

You just need to include everything inside your previous patch.

Hope this is clear!

	Gene Lian (I already quit Mozilla) Reporter
	Comment 28 • 11 years ago

(In reply to Nicolás Del Piano from comment #26)
> How can I run the tests locally?

You need to build up an emulator version to run the marionette test, which might take you some time to set up [1]. The short patch is just like what Julien said, apply for your level-1 access to upload patches on the try server, letting it run the test for you. However, it's still worth it setting up your local environment to run the test in the long term.

There are 2 tests you need to fix. One is the test_getsegmentinfofortext.js and the other one is the coding style in RIL codes.

> 
> If I run my tests locally, with the error test fixed, have I to attach it
> like a patch or what is the correct way?

You can directly include that in the same patch or do it in a separate one like [2].

> 
> How can I request level-1 permission? (I read Gene's link number 3 but I
> can't figure out correctly, have I to report a bug to do that?).

Yes, you need to fire a bug for that. Example at [3].

[1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=885679
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=874878

	Nicolás Del Piano Assignee
	Comment 29 • 11 years ago

Thanks Julien and Gene!

Here I attach the new version with the RIL test error fixed ('===' instead of '==').

I'm working on the fix of the marionette test, any news will be posted soon.

Regards!

	Gene Lian (I already quit Mozilla) Reporter
	Comment 30 • 11 years ago

Comment on attachment 8362104 [details] [diff] [review]
Bug 957084 - Unable to send empty SMS' (Gecko side) r=gene

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

Nice! Waiting for the test case before landing this.

Also, how do you generate the patch? The commit message doesn't look like an HG one. Well, Ryan might be able to help with that when landing but that would be nice if you could correct it by yourself, so we don't need to bother him.

	Julien Wajsberg [:julienw]
	Comment 31 • 11 years ago

Yeah, this is a "git format-patch"-generated patch.

see [1] for a way to convert it to a hg patch. That said, maybe Ryan doesn't mind ;)

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#I%27m_all_used_to_git.2C_but_how_can_I_provide_Mercurial-ready_patches_.3F

	Nicolás Del Piano Assignee
	Comment 32 • 11 years ago

Yes, as Julien said, I'm working with git instead of hg.
I'll start to work with hg :).

I guess that my level 1 access will be committed in a few days, so I upload the "fix" (I didn't tested it yet) of the Marionette test.

Can anyone test it?

If nobody can, I will try to run the test locally.

Regards, and sorry for this unusual patch (If this is a wrong behaviour please let me know).

	Julien Wajsberg [:julienw]
	Comment 33 • 11 years ago

"hg qimport" could import the git-formatted patches just fine.

I triggered a Try build there: https://tbpl.mozilla.org/?tree=Try&rev=69fba807be18
(hope I didn't forget something, I don't do this often either ;) )

	Julien Wajsberg [:julienw]
	Comment 34 • 11 years ago

Try looks fine.

https://tbpl.mozilla.org/php/getParsedLog.php?id=33326696&tree=Try
=> 04:32:06     INFO -  test_RadioInterfaceLayer (test_ril_code_quality.TestRILCodeQuality) ... ok

And the test_getsegmentinfofortext.js assertions all pass.

Then marking checkin-needed, but Ryan, if you can double-check, it would be nice :)

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/c53f9048c8f6

I folded the test fix into one push because the two don't logically seem independent of each other. Are we satisfied with the level of test coverage we have here or are we still wanting an automated test for this at some point?

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 11 years ago

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

	Nicolás Del Piano Assignee
	Comment 37 • 11 years ago

Thanks you all, you were very helpful :) !

	Julien Wajsberg [:julienw]
	Comment 38 • 11 years ago

Gene, do you think we'd need some more automated test for this? To ensure sending an empty SMS works?

	Gene Lian (I already quit Mozilla) Reporter
	Comment 39 • 11 years ago

It's fine enough.