Closed
Bug 957084
Opened 11 years ago
Closed 11 years ago
Unable to send empty SMS' (Gecko side)
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
1.3 C3/1.4 S3(31jan)
People
(Reporter: airpingu, Assigned: ndel314)
References
Details
(Whiteboard: [mentor=gene])
Attachments
(2 files, 4 obsolete files)
2.33 KB,
patch
|
airpingu
:
review+
|
Details | Diff | Splinter Review |
1.32 KB,
patch
|
Details | Diff | Splinter Review |
+++ 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? :)
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.
Reporter | ||
Comment 2•11 years ago
|
||
Nice! Please let me know if you encounter any issues. I'd be glad to guide.
Assignee: nobody → ndel314
Reporter | ||
Updated•11 years ago
|
Whiteboard: [mentor=gene]
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!
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).
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
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!
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.
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!
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.
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8359753 -
Attachment is obsolete: true
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!
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 !
Attachment #8359754 -
Flags: review?(gene.lian)
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.
Attachment #8359754 -
Flags: review?(gene.lian)
Assignee | ||
Comment 15•11 years ago
|
||
Hey Gene,
I fixed the issues :).
Regards!
Attachment #8360384 -
Flags: review?(gene.lian)
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #8359754 -
Attachment is obsolete: true
Attachment #8360384 -
Attachment is obsolete: true
Attachment #8360384 -
Flags: review?(gene.lian)
Attachment #8360385 -
Flags: review?(gene.lian)
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.
Attachment #8360385 -
Flags: review?(gene.lian) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
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.
Flags: in-testsuite?
Keywords: checkin-needed
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 :).
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 :)
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 :).
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).
Flags: needinfo?(gene.lian)
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
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.
Flags: needinfo?(gene.lian)
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/
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!
Flags: needinfo?(gene.lian)
Flags: needinfo?(felash)
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!
Flags: needinfo?(felash)
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
Flags: needinfo?(gene.lian)
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!
Attachment #8360385 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8362104 -
Flags: review?(gene.lian)
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.
Attachment #8362104 -
Flags: review?(gene.lian) → review+
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
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).
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 ;) )
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 :)
Keywords: checkin-needed
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?
Keywords: checkin-needed
Comment 36•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Assignee | ||
Comment 37•11 years ago
|
||
Thanks you all, you were very helpful :) !
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?
Flags: needinfo?(gene.lian)
Reporter | ||
Comment 39•11 years ago
|
||
It's fine enough.
Reporter | ||
Updated•11 years ago
|
Flags: needinfo?(gene.lian)
You need to log in
before you can comment on or make changes to this bug.
Description
•