Closed
Bug 822055
Opened 12 years ago
Closed 12 years ago
[SMS] Messages sent are duplicated in thread and throbber is visible on each message
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect, P1)
Tracking
(blocking-basecamp:+)
People
(Reporter: dscravaglieri, Assigned: julienw)
References
Details
(Keywords: dogfood, smoketest)
Attachments
(5 files, 6 obsolete files)
22.45 KB,
image/png
|
Details | |
3.54 KB,
patch
|
Details | Diff | Splinter Review | |
4.25 KB,
patch
|
Details | Diff | Splinter Review | |
2.76 KB,
text/plain
|
Details | |
48.70 KB,
patch
|
borjasalguero
:
review+
|
Details | Diff | Splinter Review |
Changeset
gecko-b2g18: 47823629795e
gaia: ff7472af0fe27f4cda0e42f301026b167ebaafe1
Steps to reproduce:
1- Open SMS
2- tap message
3- Send message
Expected:
Message appears in message thread with throbber, then throbber disappear when message is sent.
Actual:
2 Messages appears in message thread, on stays with throbber, even if you close the App or reboot the phone
Reporter | ||
Comment 1•12 years ago
|
||
There is no issue with
gecko-beta: bacea371020a
gaia: ff7472af0fe27f4cda0e42f301026b167ebaafe1
Reporter | ||
Comment 2•12 years ago
|
||
Comment 3•12 years ago
|
||
Here is some more information as I helped David narrowing down this issue.
It is a gecko regression as it works with the current gaia master (ff7472af0fe27f4cda0e42f301026b167ebaafe1) and last working smoketest gecko version (bacea371020a).
I seems to be due to bug 774621. As it fails when I cherry pick this bug patches on top of the changesets I just described. (note that you have to execute `make-reset-gaia` after updating gecko)
Vicamo, I have no idea what is going on there, it may just be matter of updating gaia code due to new platform behavior?? Here is the gaia code that interact with SMS API:
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/sms.js#L1247-L1291
Especially the code that handle pending messages, as it seems to do what you just implemented on platform side, isn't?
Comment 4•12 years ago
|
||
With the new change in Gecko, current Gaia code should work as usual. In the 'performance' bug, one of the task it's remove 'IndexedDB' for pending messages and rely on the platform only, due to 'error' and 'sending' messages are handle by Gecko now. Otherwise, we could ask Vicamo if there is any other change that should be implemented in Gaia. Vicamo, Do you know what it's happening? Thanks!
Flags: needinfo?(vyang)
Reporter | ||
Updated•12 years ago
|
Component: Gaia::SMS → General
QA Contact: mbarone976
Reporter | ||
Updated•12 years ago
|
blocking-basecamp: ? → +
Assignee | ||
Updated•12 years ago
|
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Updated•12 years ago
|
Target Milestone: --- → B2G C3 (12dec-1jan)
Comment 6•12 years ago
|
||
This test case runs correctly on gecko trunk. No duplicated message found. Still verifying b2g18 branch.
Flags: needinfo?(vyang)
Comment 7•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #6)
> This test case runs correctly on gecko trunk. No duplicated message found.
> Still verifying b2g18 branch.
Passed! Building GUI emulator locally ....
Comment 10•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #6)
> > This test case runs correctly on gecko trunk. No duplicated message found.
> > Still verifying b2g18 branch.
>
> Passed! Building GUI emulator locally ....
Can't reproduce with current revisions:
Gecko: mozillaorg/gecko-18 (Bug 817730 - Abort connect if rilproxy is not ready.)
Gaia: ff7472af0fe27f4cda0e42f301026b167ebaafe1
Comment 11•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> Can't reproduce with current revisions:
> Gecko: mozillaorg/gecko-18 (Bug 817730 - Abort connect if
Sorry, it's https://github.com/mozilla/mozilla-central/tree/b2g18
> rilproxy is not
> ready.)
> Gaia: ff7472af0fe27f4cda0e42f301026b167ebaafe1
Assignee | ||
Comment 12•12 years ago
|
||
I reproduce Bug 822255 with same gecko and gaia 74f6107ad865102abdecd7d130be891298dec42a
(so more recent).
Updated•12 years ago
|
Assignee: nobody → jonas
If this is a platform problem, we need to more information about which platform call is returning unexpected results.
Borja: Can you detail which function in the SMS API isn't returning the results that the gaia code is expecting.
Assignee: jonas → fbsc
Comment 14•12 years ago
|
||
I cant reproduce it with Today's build & Latest Gaia... Tomorrow I will try with new Build from Manifest and I will come back with more feedback
Assignee: fbsc → nobody
Assignee | ||
Comment 15•12 years ago
|
||
borja> this is very simple to reproduce. For example, you can try to send a message to a name that you don't have as a contact.
I could also reproduce it sending a message to a working number, waiting that the message is sent, wait a few seconds more, and then going back to the message list, and going to the conversation page again.
Both are reproducing the "throbber not disappearing" problem.
The duplicated message is very easily reproducible without a SIM card. Just send a SMS to whatever name (even a name that is not in your contact list), there is an error (of course), then go back to the conversation list, then go again to the conversation page.
current gaia master, current b2g18.
Comment 16•12 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #15)
> borja> this is very simple to reproduce.
I've tried to reproduce this with the gecko revisions David provided within emulator, just works as normal.
Comment 17•12 years ago
|
||
I can take a stab at this.
(In reply to Julien Wajsberg [:julienw] from comment #15)
> borja> this is very simple to reproduce. For example, you can try to send a
> message to a name that you don't have as a contact.
What exactly do you mean? Precise STRs please!
> I could also reproduce it sending a message to a working number, waiting
> that the message is sent, wait a few seconds more, and then going back to
> the message list, and going to the conversation page again.
>
> Both are reproducing the "throbber not disappearing" problem.
I can't reproduce this problem at all.
> The duplicated message is very easily reproducible without a SIM card. Just
> send a SMS to whatever name (even a name that is not in your contact list),
> there is an error (of course), then go back to the conversation list, then
> go again to the conversation page.
I can definitely reproduce this.
Assignee: nobody → philipp
Assignee | ||
Comment 18•12 years ago
|
||
The first two testcases need a SIM card.
1st testcase's STR:
* open SMS app
* tap the "+" button to create a new SMS
* enter "jouiofguofigu" as destination
* enter "pdsidispdiposdi" as text
* press the send button
=> IIRC an error is displayed (Sorry I don't have a phone here) which is correct but there is a throbber that doesn't stop which is not correct.
2nd testcase's STR: see Bug 822255, comment 0
I'm pretty sure
Comment 19•12 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #18)
> The first two testcases need a SIM card.
Yep.
> 1st testcase's STR:
> * open SMS app
> * tap the "+" button to create a new SMS
> * enter "jouiofguofigu" as destination
> * enter "pdsidispdiposdi" as text
> * press the send button
> => IIRC an error is displayed (Sorry I don't have a phone here) which is
> correct but there is a throbber that doesn't stop which is not correct.
Yes, the error is displayed, but the throbber disappears as expected.
> 2nd testcase's STR: see Bug 822255, comment 0
I can't reproduce this either.
So far I can only see the duplicate message appearing when you try to send a text without a SIM card.
Assignee | ||
Comment 20•12 years ago
|
||
Ok, I will try to produce more precise STR today then.
Reporter | ||
Comment 21•12 years ago
|
||
This issue is reproducible with today build:
gecko-b2g18: 6f2f48519b62
gaia: 7c95cecd8bf049c103e626c51de5714b7aefb7dd
You need a SIM card and you must send a message to another phone, if you send a message to yourself, it works but it's not the real usecase.
Reporter | ||
Comment 22•12 years ago
|
||
In addition, to see the throbber, from the thread, go back to the message list then reopen the thread.
Comment 23•12 years ago
|
||
I've tested with:
gaia:"7c95cecd8bf049c103e626c51de5714b7aefb7dd" (same Gaia as you)
gecko:"5bad6f1d595945a4b4acc1125447ec0669bf6474"
And with this version it's impossible to reproduce. Im gonna try to test with your version (Manifest Build it's building yet...), but it seems that it's something related with Gecko (because same Gaia it's working for me).
I've tried the following scenarios:
-Send a SMS with SIM Card to a valid phone number.
-Send a SMS with SIM Card to a valid phone number in 'Flight mode'.
-Send a SMS without SIM Card to a valid phone number.
I will come back with more feedback once I have the build with your version.
Assignee | ||
Comment 24•12 years ago
|
||
borja> don't forget you have to go back to the "message list" page, and then come back at the "navigation" page, to see this behaviour.
And also, I saw that if you wait a few seconds after the SMS was sent, and THEN you go back to the message list and then come back to the navigation page, it is happening more consistentely.
I'll try again with a new fresh build as soon as I arrive at the office.
Assignee | ||
Comment 25•12 years ago
|
||
new fresh build.
gecko b2g18, |hg summary| result :
parent: 122365:a7a4704fd1ef tip
Bug 816452 - Don't fire mozbrowsererror when loading page is stopped. r=jlebar a=blocking-basecamp
gaia master, |git log -n 1 --oneline| result:
7c95cec Merge pull request #7057 from asutherland/email-bcc-fix
Can reproduce the "throbber stays here" bug with this STR :
* have a SIM card insered, with the PIN entered
* open sms app
* tap "+"
* put your own number as the recipient
* add any message
* tap "send"
=> the message appears in the thread view, with a throbber
=> when the message is sent, the throbber disappear
=> we get a notification that a new message appeared (BTW, it doesn't show up automatically in the opened conversation which is another bug imho)
* wait a minute
* tap "<" to go back
* tap on the number you sent the mail (which should be your number)
=> the throbber is back beside your sent message
I can reproduce this every time. So this is very strange that you don't and we might have something stranger here.
I have no helpful log.
Comment 26•12 years ago
|
||
[:julienw] It's only happening when you send a SMS to yourself? or it's happening in every case?
Assignee | ||
Comment 27•12 years ago
|
||
So I added some logs in the pile of spaghetti that is the Sms app, and I found that the mozSms API returns messages in the "sending" state, which is wrong. This happens for both sending to myself and sending to another number.
Assignee | ||
Comment 28•12 years ago
|
||
Digging deeper, I'm wondering if the problem doesn't come from the delivery message. That would explain why I have to wait a few seconds/minutes before doing the final actions to make the bug appear. And that would also explain maybe why we don't all see this, maybe some networks don't send the delivery message.
Since I'm not familiar at all with this, could someone have a look at [1] and check if this seems fine ? Who could check this ?
[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1450
Comment 29•12 years ago
|
||
Hi all. After reviewing deeply I've found some problems of sync. between Gaia & Gecko code, and it's the root of this problem and others related with SMS, so we should move fast on it.
After landing https://bugzilla.mozilla.org/show_bug.cgi?id=774621 'sending' messages are handled by Gecko, but also (due to indexedDB for 'sending/error' messages is still in the app), this SMS is shown twice! (one from Gecko, one from Gaia).
¿Could we fix it in Gaia side?
I have a patch ready, but I've discovered that Gecko is not handling 'failed' messages properly, so messages sent when 'flight mode' & 'No SIM Card' scenarios are not stored in Gecko! So removing the local indexedDB will produce a regression in the functionality :(. And furthermore I cant remove `partially` this indexedDB due to it's tied to PhoneNumberJS, and as you know this library should be out of Gaia code asap.
¿How could we fix this issue?
There are two ways for doing it (AFAIK):
- One should be creating a 'followup' bug where [:vicamo] should add a patch for the 'failed' messages, and I would add the patch for Gaia. ¿Problem? Until these patches will be ready and landed SMS App is not going to work properly!
- On the other hand we could backout this changes in Gecko, add the 'failed' messages management and Gaia patch, and merge all this code once it will be ready and reviewed. With this approach SMS App would be working while we create the patch.
Wdyt?
Flags: needinfo?(vyang)
Updated•12 years ago
|
Flags: needinfo?(philipp)
Assignee | ||
Comment 30•12 years ago
|
||
My view is that Sms is working fine enough currently and you should just move forward in creating the new patches.
Could you please upload your wip patch so that the work is not done twice (I'm thinking to :philikon here :) ) ?
Comment 31•12 years ago
|
||
I'm in charge of the Gaia part, and I would need the patch in Gecko side for the 'failed' sms for testing it and check that everything works as expected (currently there are scenarios that it's impossible to test :( ). That's why I would need who is gonna be in charge of this in Gecko side for sync. in this work and deliver it asap.
Comment 32•12 years ago
|
||
More info yet :(. Change to 'sending' status should dispatch an event, but this event it's never dispatched. So it's not only adding 'failed' in the scenarions missing, I would need to fix this bug as well.
Comment 33•12 years ago
|
||
(In reply to David Scravaglieri [:scravag] from comment #21)
> This issue is reproducible with today build:
Please be specific. There are three separate issues being discussed in this bug. (In the future, I suggest filing individual bugs for these.)
(In reply to Julien Wajsberg [:julienw] from comment #27)
> So I added some logs in the pile of spaghetti that is the Sms app, and I
> found that the mozSms API returns messages in the "sending" state, which is
> wrong. This happens for both sending to myself and sending to another number.
Can you please upload this logging patch?
(In reply to Borja Salguero [:borjasalguero] from comment #29)
> After landing https://bugzilla.mozilla.org/show_bug.cgi?id=774621 'sending'
> messages are handled by Gecko, but also (due to indexedDB for
> 'sending/error' messages is still in the app), this SMS is shown twice! (one
> from Gecko, one from Gaia).
This would explain the duplicate message when there's no SIM card.
> ¿Could we fix it in Gaia side?
> I have a patch ready, but I've discovered that Gecko is not handling
> 'failed' messages properly,
Please be more specific. How is Gecko not handling failed messages "properly"? What is the "proper" behaviour and what is the current behaviour? If there's a Gecko bug, let's file it and refer to it here.
> so messages sent when 'flight mode' & 'No SIM
> Card' scenarios are not stored in Gecko! So removing the local indexedDB
> will produce a regression in the functionality :(.
It already isn't working. See bug 822468.
> ¿How could we fix this issue?
> There are two ways for doing it (AFAIK):
> - One should be creating a 'followup' bug where [:vicamo] should add a patch
> for the 'failed' messages, and I would add the patch for Gaia. ¿Problem?
> Until these patches will be ready and landed SMS App is not going to work
> properly!
It already isn't working properly. Can we please outline what the Gecko bug is?
Flags: needinfo?(philipp)
Comment 34•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #33)
> > so messages sent when 'flight mode' & 'No SIM
> > Card' scenarios are not stored in Gecko! So removing the local indexedDB
> > will produce a regression in the functionality :(.
>
> It already isn't working. See bug 822468.
Never mind, I can't reproduce this bug with today's build.
Comment 35•12 years ago
|
||
> > ¿Could we fix it in Gaia side?
> > I have a patch ready, but I've discovered that Gecko is not handling
> > 'failed' messages properly,
>
> Please be more specific. How is Gecko not handling failed messages
> "properly"? What is the "proper" behaviour and what is the current
> behaviour? If there's a Gecko bug, let's file it and refer to it here.
>
Currently Gecko is not handling 'failed' messages in following scenarios:
- Send a SMS without SIM Card
- Send a SMS in flight mode
Also is not handling some 'status' changes:
- There is no event when 'sending'
> > so messages sent when 'flight mode' & 'No SIM
> > Card' scenarios are not stored in Gecko! So removing the local indexedDB
> > will produce a regression in the functionality :(.
>
> It already isn't working. See bug 822468.
>
It's working due to Gaia has it own IndexedDB where store these messages. However, it should be stored by Gecko, due to having this management in Gaia level is tied to PhoneNumberJS, and it's well known that one of the goals it's remove this library in Gaia.
Assignee | ||
Comment 36•12 years ago
|
||
per :philikon request
Comment 37•12 years ago
|
||
(In reply to Borja Salguero [:borjasalguero] from comment #35)
> Currently Gecko is not handling 'failed' messages in following scenarios:
> - Send a SMS without SIM Card
> - Send a SMS in flight mode
Please define "handling". Are you saying it's not notifying a 'failed' event when trying to send an SMS in these scenarios?
> Also is not handling some 'status' changes:
> - There is no event when 'sending'
That's expected. 'sending' is the initial status when you're sending a message. You *know* it's 'sending' because you just asked the system to send the message. So there's no need to dispatch an event. It's like mozTelephony doesn't dispatch a "dialing" event when you dial a new phone call. Because that's the initial status.
Comment 38•12 years ago
|
||
Here's a patch with a bit more debugging. I see all events and database records working as expected.
With PhoneNumberJS already integrated in Gecko's SmsDatabaseService, why can't we just rip out the pending SMS database in Gaia?
Attachment #693826 -
Attachment is obsolete: true
Flags: needinfo?(vyang)
Comment 39•12 years ago
|
||
We are going to remove this indexedDB in Gaia (in fact this is the patch that I've almost ready), but for doing it I need the issues explained before. Furthermore, in Gecko SMS states are not working properly! If after send some sms to some destination you call 'getAllMessages', al messages are marked as 'sending'.... Im testing with production build of today.
Assignee | ||
Comment 40•12 years ago
|
||
borja, could you please attach your work in progress ?
We'd like to try and fix this during the following week and it would be nice to not begin from scratch.
Thanks !
About the fact that all messages are marked 'sending', I'm quite sure of this too, and it happens in [1], maybe because the envelope is not updated in handleSmsSent above.
[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1468
Comment 41•12 years ago
|
||
Comment 42•12 years ago
|
||
Scenario easy to reproduce:
- Send a SMS to phone number A.
- Come back to 'Thead List'
- Click again in 'Phone number A' in order to see all sms from/to this phone number.
Log:
E/GeckoConsole( 458): Content JS LOG at app://sms.gaiamobile.org/js/sms.js:513 in thlui_renderThreads:
Threads: Message ID 1 status sending
Expected:
Message ID 1 should be 'SENT' due to it was sent properly
Currently:
All messages are marked as 'sending', so SMS App is completely broken.
Comment 43•12 years ago
|
||
In the patch I've removed the messages from local indexedDB, so all messages retrieved come directly from Gecko. I've added some logs as well in order to check what it's happening.
Assignee | ||
Comment 45•12 years ago
|
||
Patch in Bug 824465 fixes the "throbber is still there" bug for me.
For the "duplicate message" bug I think that getting rid of the IndexedDB database will just work. I'll try this now.
Assignee: philipp → felash
Assignee | ||
Comment 46•12 years ago
|
||
Remove the pending IndexedDB and minor refactoring
+ lots of logs
I know I have still some bugs left:
- deleting multiple threads
- we don't keep the error mark when going back on a thread
But it seems to work better than ever !
I hope to have some more time later today, would be cool to have this bug fixed app for Christmas ;-)
Comment 48•12 years ago
|
||
Julien - I haven't had time to go over your patch yet, but the work I did for #824552 fixes the deletion, and loading marks when revisiting a thread. Not sure if it's helpful at all, but here is the pull request: https://github.com/mozilla-b2g/gaia/pull/7186
Assignee | ||
Comment 49•12 years ago
|
||
(In reply to Kevin Grandon from comment #48)
> Julien - I haven't had time to go over your patch yet, but the work I did
> for #824552 fixes the deletion, and loading marks when revisiting a thread.
> Not sure if it's helpful at all, but here is the pull request:
> https://github.com/mozilla-b2g/gaia/pull/7186
Thanks Kevin !
The loading marks problem is a gecko bug (patch in bug 824465 that landed on inbound today so it should come to other branches later).
Your patch could work (I didn't try it) but it adds up a hack on top of another hack, and I'm not comfortable with this, especially for the Sms app.
I think that I'll finish my patch today which removes a lot of now-unnecessary things from the sms app.
Would you like to review this patch once it's ready ?
Comment 50•12 years ago
|
||
Hi Julien! Probably we are working in parallel on the same thing:S... I was working in https://bugzilla.mozilla.org/show_bug.cgi?id=811539 , that basically it's based on removing IndexedDB for Pending (due to is tied to PhoneNumberJS, and the bug is about removing this phone number code) and removing the 'hacks' in 'Contacts' request. As I've seen that we are working on the same, we should make it step by step and try to not 'overlap' our code. In this case (if you agree) your patch would be removing this 'indexedDB' and 'sending' management, and I will be in charge of removing the other parts of code where 'PhoneNumberJS' is used and the 'optimization' code, that it's ready in one of my branches. Wdyt? Please put me as reviewer of your patch and I will test&review it asap! Thanks ;)
Assignee | ||
Comment 51•12 years ago
|
||
Borja, I'm ok with this, I should have a working patch later today. Thanks !
Assignee | ||
Comment 52•12 years ago
|
||
Remove the pending IndexedDB and minor refactoring in sending, resending, and
deletion of messages and threads.
This fixes the duplicated message in thread.
Fixed some other bugs in the process :
- duplicated message when we were resending a failed message
- the received SMS was not displayed when I sent it to myself
- when I was sending the first SMS to a contact, and when I was hiding the Sms
app (using the home button) and then going back to the Sms app, then the
thread was empty. This happened only when the phone number was not normalized
yet in the hash.
- now we try to send the SMS even if we don't have mozSettings
- fix notification was appearing when we were already in the good thread, when
the num was not normalized yet
Attachment #695501 -
Attachment is obsolete: true
Attachment #695834 -
Flags: review?(fbsc)
Assignee | ||
Comment 53•12 years ago
|
||
I forgot: now we store in gecko a SMS that the user tried to send when airplane mode was set.
Assignee | ||
Comment 54•12 years ago
|
||
changes: reset MessageManager.currentNum when necessary + fixed most gjslint and jshint warnings.
Attachment #695834 -
Attachment is obsolete: true
Attachment #695834 -
Flags: review?(fbsc)
Attachment #695945 -
Flags: review?(fbsc)
Comment 55•12 years ago
|
||
Julien, i was reviewing your code and I have some concerns about 'currentNum' due to we should not work with 'phone number' because we are going to lost this capability once PhoneNumberJS will be out of SMS App (I mean, currentNum could be not internationalized, so probably we are gonna have a lot of issues about it). Otherwise I would like to test a simple approach (Im trying to develop it and I would like to share with you and comment this idea) because I think that it could be more simpler! I will come back soon with this info and the comments with a deep review of your code. Thanks for all your support!
Assignee | ||
Comment 56•12 years ago
|
||
Borja, let's take care of this in Bug 809213 if this turns out to be a problem.
I tried to take care to always have a internationalized number in currentNum but I may have failed. If you find a place in my code where this is not the case, I can fix that.
I don't want a simple approach, I want a approach that works and fixes all the bugs I found in the process, and removing the IndexedDB that we don't want anymore. I'm not sure you could find a simpler approach doing this. I took great care to not change how the whole application worked, I kept all the functions, so I think this should be ok for you.
I'm looking forward to your review so that we can have a working Sms app ! :)
Assignee | ||
Comment 57•12 years ago
|
||
Just to be clear, my process has been :
* first, remove the Pending DB everywhere it was used
* then, fix any bug that I found that was related to the DB being removed.
* tested a lot, with or without a SIM card, with or without airplane mode, sending SMS to myself, to other person, receiving SMS when I was in the app, receiving SMS when I was in the good thread, receiving SMS when I was elsewhere, resending a failed SMS, canceling the resend and then trying to resend again, a failed resend, trying to resend when going back to a thread...
Comment 58•12 years ago
|
||
For sure! I have the same goal with SMS App ;). Im gonna try to upload the approach for sharing with you and analyze if it's feasible and know your opinion and I will review your code as well. Thanks!
Comment 59•12 years ago
|
||
Julien! Here you have the proposal. https://github.com/mozilla-b2g/gaia/pull/7242 Please take a look and let me know what do you think! Im reviewing your code now, I will come back with feedback. Thanks!
Assignee | ||
Comment 60•12 years ago
|
||
I understand that you want to put the handlers directly on navigator.mozSMS. I agree that this is interesting and will lead to cleaner code.
However I'm concerned because this is a very big change and it may take some time to work correctly (your code doesn't work at all for now)... and we must ship in 2 weeks.
I'd suggest that we keep this for post-v1 and we merge my code that I tested a lot for v1.
Let's file a new bug for this and let's move forward for v1 ? And then let's fix other bugs ;)
What do you think ?
Assignee | ||
Comment 61•12 years ago
|
||
Just FYI I spent nearly 3 full days on this, with half of it only for testing.
Comment 62•12 years ago
|
||
(Reviewing...!)
Comment 63•12 years ago
|
||
Julien, could you create the patch as a PR in order to add the comments easily? Thanks!
Assignee | ||
Comment 64•12 years ago
|
||
Here you are : https://github.com/mozilla-b2g/gaia/pull/7244
However, you know that you can add comments in the bugzilla "splinter review" view by double-clicking on one line ?
Comment 65•12 years ago
|
||
Assignee | ||
Comment 66•12 years ago
|
||
new patch implementing Borja's proposition. The code is a lot cleaner now. There is a lot of things that work differently now, both in terms of code or of behaviour.
Also see PR at https://github.com/mozilla-b2g/gaia/pull/7244
Attachment #695945 -
Attachment is obsolete: true
Attachment #695945 -
Flags: review?(fbsc)
Attachment #696555 -
Flags: review?(fbsc)
Comment 67•12 years ago
|
||
Awesome! This code looks so nice! Tomorrow I will review it in more detail and I will test it in my Unagi. Otherwise thanks a lot for your effort! SMS App is gonna work better than ever with these changes ;). If I find something to change I will add a comment and I will try to make you a PR to your branch (only if it's necessary) in order to save time and merge it asap. Merçi! Gracias!
Assignee | ||
Comment 68•12 years ago
|
||
I forgot to remove the last bit of now-useless "pendingAction" :)
already pushed to the PR
Attachment #696555 -
Attachment is obsolete: true
Attachment #696555 -
Flags: review?(fbsc)
Attachment #696614 -
Flags: review?(fbsc)
Assignee | ||
Comment 69•12 years ago
|
||
Borja, take care because I rewrote the commit so if you checked out yesterday, you should do it again :-)
Comment 70•12 years ago
|
||
Hehehe, dont worry! Im checking the code again and testing it in my Unagi, I've added to the PR only a small comment. The rest of the code looks nice, Im going to use it during today as my personal one trying to generate all scenarios and test it again and I will come back with the final review asap. By the way I think that this code will be merge on time! :D
Assignee | ||
Comment 71•12 years ago
|
||
I've fixed the error you reported, thanks !
Attachment #696614 -
Attachment is obsolete: true
Attachment #696614 -
Flags: review?(fbsc)
Attachment #696658 -
Flags: review?(fbsc)
Assignee | ||
Comment 72•12 years ago
|
||
I've seen one or two unrelated bugs, I'll file new bugs as soon as this one is landed.
Updated•12 years ago
|
Attachment #696658 -
Flags: review?(fbsc) → review+
Comment 73•12 years ago
|
||
R+ from my side! I've tested this code a lot and it's working as expected. Please squash your changes and that's all!. Thanks a lot for your support and your effort during these days! This code will be merged on time ;). Last review of the year, so ... je te souhaite une bonne année 2013! Feliz año!
Assignee | ||
Comment 74•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Component: General → Gaia::SMS
QA Contact: mbarone976
You need to log in
before you can comment on or make changes to this bug.
Description
•