Gecko is now working properly with PhoneNumberJS. Scenario (made with Spanish SIM Card, but you can reproduce the same with your SIM Card): - Send a SMS to +34612123123 - Make a request to Gecko with 'getMessages' given a filter for the NON-internationalized phone number 612123123 EXPECTED: We retrieve 1 message, due to +34612123123 & 612123123 it's the same phone number in Span CURRENTLY: We retrieve 0 messages. Gecko it's not working properly with PhoneNumberJS Why does this one should be tef+? Because it's a regression, and picking a contact from SMS App it's completely broken. Scenario: - Create a contact 'Jesper 612123123' - Open SMS App, Send a SMS to +34612123123 - From SMS App: SMS > New > Pick contact > 'Jesper 612123123' EXPECTED: We see the previously sent SMS and we can send a new one CURRENTLY: We retrieve 0 messages, so we loose the threading completely.

You really need to know why we moved PhoneNumberJS into Gecko and integrated it with SMS database. Phone number matching is difficult and no easy way to achieve this goal. All numbers stored in SMS database are in their international form to simplify matching with all possible forms of a specified number. That is, you *MUST* only pass normalized phone numbers to getMessages() call, or, by current design, nothing will ever come out. If you still wish we can match any possible phone number plans in getMessages(), then we shouldn't have bothered saving all numbers as their international form at the first place because this way just doesn't scale up. Instead, we need another 10K lines of code, I guess, to complete it. If you have no further opinion, I'd like to close this issue as WONTFIX.

Hi Vicamo, Im gonna try to explain my point of view about it. Image that I want to create a contact with - Name: Manolo Pérez - PhoneNumber: 612123123 - Carrier: Movistar Why dont we store the object with (taking as assumption a Spanish SIM Card): - Name: Manolo Pérez - InternationalizedPhoneNumber: +34612123123 - PhoneNumber: 612123123 - Carrier: Movistar Using this, when I have to pick a contact, Im gonna see a 'select' HTML input with a list of my contacts (for example 'Manolo 512123123 Movistar'), but the real phone number used by Gecko should be the 'InternationalizedPhoneNumber'. Also this approach we are keeping the user info, but internally all DBs are talking with the same conditions (in this case internationalized version of everything related with a phone number). And in fact, for searching a contact, could be faster. When we were using in SMS App the 'PhoneNumberJS' we had all these issues solved with this approach, and it was working. Wdyt? Thanks!

Let's put it this way. The return SmsMessage object has only one field for sender and receiver each. Explicitly internationalized or the unmodified, which one do you want it to be? Unfortunately, the networks may have different taste than yours, or they just give you both, randomly, unconditionally, because there is no such rule at all in the outside world. So how do we insert a message object into db? How many numbering plans should we take into consideration? We do have multi-key in bug 826977, so we can surely insert as many alias numbers as you want. But how many? Besides, the word "internationalized" isn't very accurate. An international number is actually consisted of an international access code, country calling code, and the national significant number. The first one, international access code, is usually replaced by '+' and as a whole becomes "the international number" now we used in Gecko. So actually, a true "international" contact number in Taiwan could be either "00234612123123", "00534612123123", or "00934612123123", etc. So, are you going to file another bug for the ability to match all these numbers? Can't we just stick with ONE simple way for now?

We don't feel this will be a common user scenario (the issue was only found now), and this doesn't represent data loss, just mild confusion. Not a blocker.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3) > Let's put it this way. The return SmsMessage object has only one field for > sender and receiver each. Explicitly internationalized or the unmodified, > which one do you want it to be? Unfortunately, the networks may have > different taste than yours, or they just give you both, randomly, > unconditionally, because there is no such rule at all in the outside world. > So how do we insert a message object into db? How many numbering plans > should we take into consideration? We do have multi-key in bug 826977, so we > can surely insert as many alias numbers as you want. But how many? > Currently all messages stored in SMS DB are stored with the internationalized version of the phone number using PhoneNumberJS library of Andreas Gal. So actually all messages contains only internationalized phone numbers, so it's out of the scope of this bug. > Besides, the word "internationalized" isn't very accurate. An international > number is actually consisted of an international access code, country > calling code, and the national significant number. The first one, > international access code, is usually replaced by '+' and as a whole becomes > "the international number" now we used in Gecko. So actually, a true > "international" contact number in Taiwan could be either "00234612123123", > "00534612123123", or "00934612123123", etc. So, are you going to file > another bug for the ability to match all these numbers? > > Can't we just stick with ONE simple way for now? PhoneNumberJS takes into account these assumptions, you can try directly http://libphonenumber.googlecode.com/svn/trunk/javascript/i18n/phonenumbers/demo.html . However this is not the issue related with this bug. If you want you could talk with Andreas about it. The issue here is how we are storing Contacts/SMS and the methods for retrieving the info given a number. So the bug is the following: - Send a SMS using Spanish SIM Card to 612123123 - Send a SMS using Spanish SIM Card to +34612123123 - getMessages with a filter using 612123123 EXPECTED: I retrieve 2 messages CURRENTLY: I retrieve only one, neglecting the other to +34612123123 This is a REGRESSION, due to it was working before delegating all phone number normalization to Gecko, so it must be working now as well.

(In reply to Borja Salguero [:borjasalguero] from comment #5) > Currently all messages stored in SMS DB are stored with the > internationalized version of the phone number using > PhoneNumberJS library of Andreas Gal. So actually all messages > contains only internationalized phone numbers, so it's out of > the scope of this bug. That's all that I want to tell you: your expectation is currently out of the scope.

Per offline discuss on skype, PhoneNumberUtil isn't exposed to Gaia yet :( So Gaia just can't always pass an international number as I said in comment #1, and we have to find some way to make it in Gecko.

Thanks a lot for your support Vicamo! ;)

We're marking this as tracking but not as blocking since we don't have a strategy for how to fix this yet. We definitely need to look through PhoneNumberJS and SMS interactions and figure out which edge cases we'll try to make work, and which ones we won't. But let's look at all the PhoneNumberJS issues together and come up with a comprehensive plan.

i'm re-requesting blocking tef? to investigate this more urgently. Bug 835645 was found during yesterday's smoketest, and duped to this bug. The contents in that bug describes expected functionality, as in i shoudl be able to view my SMS thread from a contact that's saved in the Contact App.

https://bugzilla.mozilla.org/show_bug.cgi?id=835645#c5 states that this cannot be reproduced on the 1/29 build

(In reply to Lukas Blakk [:lsblakk] from comment #12) > https://bugzilla.mozilla.org/show_bug.cgi?id=835645#c5 states that this > cannot be reproduced on the 1/29 build The point here it's that your contact it's stored with the internationalized version '...My contact has the fully-internationalized number (+1-416-<my #>) ...', so with this scenario you are not reproducing the bug. For reproducing it, you have to store the non-internationalized version of your phone number (follow the steps of the description of the bug https://bugzilla.mozilla.org/show_bug.cgi?id=833291#c0). Thanks!

Daniel - is this a certification blocker? If not, triage still believes that comment 4 stands. We'd track, but not block.

Hi Alex. IMHO this this one it's more than a common user scenario... Probably 80% of Messages that we type come from Contacts App... and without fixing this bug this functionality it's completely broken :S.

OK, so I've seen this in my daily use of the device so I think I can provide more detailed steps about how to reproduce it: 1/ Add a contact to the address book with a phone number without the international prefix (e.g. Borja, 666666666) 2/ Go to the contact details of Borja and click on the SMS icon 3/ A new empty SMS conversation is open 4/ Send a message to Borja in that SMS conversation. 5/ Kill SMS application 6/ Go again to Address Book and Borja contact details 7/ Click on SMS icon Expected result The previous conversation was opened showing the message sent in step 4/ Current result: A new empty conversation is opened (furthermore with a weird transition) At least we should track this and accept a patch unless the risk is high. Let me check with some other people if there is a specific test in TEF certification for this specific scenario and in that case it should be a blocker. I hope to have an answer by Monday.

Daniel, could it be a certification blocker ?

(In reply to David Scravaglieri [:scravag] from comment #18) > Daniel, could it be a certification blocker ? Yes, it is a blocker, thanks for marking it

Gregor, can you investigate a bit more here?

I have a working branch in github[1] and will give first draft implementation tomorrow. [1]: https://github.com/vicamo/b2g_releases-mozilla-central/tree/bugzilla/833291/master

Gregor Vicamo is on Holiday, need your help here, could you investigate more ?

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

(In reply to David Scravaglieri [:scravag] from comment #22) > Gregor Vicamo is on Holiday, need your help here, could you investigate more > ? Currently I can't even open a thread with gaia master and mc master :(

Comment on attachment 714411 [details] [diff] [review] [m-c] Part 2: add thread, participant stores - WIP My idea here is to have another indirect mapping of addresses and participants. Each participant may have multiple addresses but has only one, unique id - participantId. When getMessages() called with a filter with its 'numbers' attribute set, we first look up the corresponding participantId(1) and fetch all related threads(2). 1) Matching participant address is (in my mind) kept simple by matching the address string entirely. It follows we have to try populate the 'addresses' array with as many entries as we can. Both international and local numbers should be included for now. There are still corner cases, but we just can't fix it with such exact matching method. Some kind of fuzzy match must be added to push our reach even further. And, again, 'further' doesn't mean all because the phone numbering plans we have in the world just don't guarantee every number is globally unique. So in some cases, we just can't distinguish one from the other. 2) Thread fetching API we have now isn't very straight forward. GetThreadList() returns an array of thread list items. Each of them has a 'senderOrReceiver' attribute, which is actually the 'sender' field of an incoming message or 'receiver' of an outgoing one. Gaia then gets a list of messages by calling GetMessages() with a filter with its 'numbers' field set with the value of 'senderOrReceiver'. However, this method brings a problem when searching for messages sent to yourself -- all messages, instead of messages in the specified thread, are returned. To solve this, we might need some revise to message thread API. Something like having an additional field 'threadId' in SmsFilter and replacing 'senderOrReceiver' with 'threadId' as well.

The approach looks good to me. Jonas will provide some writeup here. We still have to figure out how the matching function will look like and when we are matching numbers.

The general approach in the patch seems great. Having a separate objectStore which maps between participants and phone numbers is very flexible which will allow us to play around with different heuristics for figuring out which numbers match each other. We talked about creating a function which looks like: function fuzzyMatchNumbers(number1, number2) { if (...) { return true; } if (...) { return true; } return false; } I.e. the function takes two numbers and then applies heuristics to see if the numbers are probably the same number. Whenever a message is sent or received and we want to insert it into the database, we would then take the phonenumber of the message and compare it against the "address" index of the "participant" objectStore. If we find an exact match then we use the participantId of that number. This should be the common case once the participant objectStore is fully populated. If we don't find an exact match, we iterate through the whole "address" index of the "participant" objectStore and call fuzzyMatchNumbers(numberFromIndex, numberFromMessage). If we find a hit then we'll use that participantId. Otherwise we create a new entry in the participant objectStore. This will be slow, but should only happen the first time you send or receive a message to a given number. The nice thing about this approach is that all logic is confined to the fuzzyMatchNumbers function, which means that we can tweak the logic there to improve our behavior. We could even build the function into PhoneNumberJS. To handle existing data, simply going through the full data set and populating the participant table using the above algorithm is likely the best option. This is also something that we'll likely won't have to do very often. (In reply to Vicamo Yang [:vicamo][:vyang] from comment #27) > 2) Thread fetching API we have now isn't very straight forward. > GetThreadList() returns an array of thread list items. Each of them has a > 'senderOrReceiver' attribute, which is actually the 'sender' field of an > incoming message or 'receiver' of an outgoing one. Gaia then gets a list of > messages by calling GetMessages() with a filter with its 'numbers' field set > with the value of 'senderOrReceiver'. However, this method brings a problem > when searching for messages sent to yourself -- all messages, instead of > messages in the specified thread, are returned. To solve this, we might > need some revise to message thread API. Something like having an additional > field 'threadId' in SmsFilter and replacing 'senderOrReceiver' with > 'threadId' as well. I think we can simplify this problem significantly if we define that the API only deals with "sender" numbers on received messages, and "receiver" numbers on sent messages. I.e. never index on the users own phone number (except when sending/receiving to himself). That means that we in the "threads" objectStore only need to store a single participantId. And we can keep the getThreadList API fully compatible with the current API. Simply return the first phone number for that participant. I do understand that this means that we can't support queries like "get all messages sent from me when I used SIM-card X and that are marked as delivered". But I've never seen a phone UI which needs such functionality. The API already doesn't support queries like "get all messages sent from me when I used SIM-card X and that were sent to number Y" since we only have one number field. So I think that we should look at supporting such advanced queries later. Finally, I was wondering if we could use the name "number" rather than "address" for phone numbers in the code. This seems like a much more intuitive name. I've never seen the term "address" being used to refer to phone numbers before. This is totally up to you though.

(In reply to Jonas Sicking (:sicking) from comment #29) > Whenever a message is sent or received and we want to insert > it into the database, we would then take the phonenumber of > the message and compare it against the "address" index of the > "participant" objectStore. If we find an exact match then we > use the participantId of that number. This should be the > common case once the participant objectStore is fully > populated. That's exactly what I want to do now :) > I think we can simplify this problem significantly if we > define that the API only deals with "sender" numbers on > received messages, and "receiver" numbers on sent messages. > I.e. never index on the users own phone number > (except when sending/receiving to himself). > > That means that we in the "threads" objectStore only need to > store a single participantId. And we can keep the > getThreadList API fully compatible with the current API. > Simply return the first phone number for that participant. Exactly what I'm doing. You can see my threadRecord::participants is an array of only one element for SMS. > I do understand that this means that we can't support queries > like "get all messages sent from me when I used SIM-card X and > that are marked as delivered". But I've never seen a phone UI > which needs such functionality. The API already doesn't > support queries like "get all messages sent from me when I > used SIM-card X and that were sent to number Y" since we only > have one number field. So I think that we should look at > supporting such advanced queries later. It's not about that advanced query. I often send SMS to myself in feature phone age to remind me something later. In this case, the actual problem is we borrow getMessages() API for retrieving messages in a specific thread. This API retrieves messages has either the same sender or receiver number as specified in SmsFilter. However, in thread message retrieving, the matching policy is "same sender in incoming messages or same receiver in outgoing ones". It's this very minor semantic difference causes the problem. This problem does exist in every SMS UI design even ours because the problem is triggered by sending a SMS message to yourself, not some special UI flow or query dialog. > Finally, I was wondering if we could use the name "number" > rather than "address" for phone numbers in the code. This > seems like a much more intuitive name. I've never seen the > term "address" being used to refer to phone numbers before. > This is totally up to you though. That's for MMS. In MMS we'll have types other than phone numbers like email addresses, etc. And actually this algorithm was originally planned for MMS, but we just need it now.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #30) > It's not about that advanced query. I often send SMS to myself in feature > phone age to remind me something later. Yeah, I agree that this is the use case that we want to support. > In this case, the actual problem is > we borrow getMessages() API for retrieving messages in a specific thread. > This API retrieves messages has either the same sender or receiver number as > specified in SmsFilter. However, in thread message retrieving, the matching > policy is "same sender in incoming messages or same receiver in outgoing > ones". It's this very minor semantic difference causes the problem. This > problem does exist in every SMS UI design even ours because the problem is > triggered by sending a SMS message to yourself, not some special UI flow or > query dialog. What I propose is that the getMessages works such that if you specify a number using SmsFilter, then we will only return received messages where sender number matches the specified number and sent messages where the receiver number matches the specified number. This can be accomplished by only creating one entry in the appropriate index for each message. I think the old code would always generate two entries in the index, one for the sender number and one for the receiver number. What I propose is that we always just generate one entry in the index, but which number we use depends on if its a sent or received message. Does that sound ok? > > Finally, I was wondering if we could use the name "number" > > rather than "address" for phone numbers in the code. This > > seems like a much more intuitive name. I've never seen the > > term "address" being used to refer to phone numbers before. > > This is totally up to you though. > > That's for MMS. In MMS we'll have types other than phone numbers like email > addresses, etc. And actually this algorithm was originally planned for MMS, > but we just need it now. Ok.

(In reply to comment #31) > What I propose is that we always just generate one entry in the index, but which > number we use depends on if its a sent or received message. Then what happens when it comes to MMS, which features multiple recipients? When we're filtering with only one number, should we match an incoming message with that number being one of its recipients? In SMS, one message has two participants and we can always fill that index key with the other guy. However, in MMS, one message may have more than two participants. If you still want getMessages() to return messages of a specific thread, then we should match messages with the same participants instead. That is, for incoming messages, match their senders and other recipients than myself; or, for outgoing messages, match all recipients. Now you find the semantics of this API is completely different for SMS messages and MMS messages. In SMS, a SmsFilter with two numbers matches a message with either number being its sender or receiver, but in MMS, that filter matches a message with three paticipants. The same API has two different meanings. Does it really what you want?

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #32) > (In reply to comment #31) > > What I propose is that we always just generate one entry in the index, but which > > number we use depends on if its a sent or received message. > > Then what happens when it comes to MMS, which features multiple recipients? For sent MMSs we add one entry for each recipient, but no entry for the sending number. For received MMSs I think we could just add one entry for the sending number. > Now you find the semantics of this API is completely different for SMS > messages and MMS messages. In SMS, a SmsFilter with two numbers matches a > message with either number being its sender or receiver, but in MMS, that > filter matches a message with three paticipants. The same API has two > different meanings. Does it really what you want? No, I don't think that's what we want. I think we should keep the current semantics even for MMS. That means that there is no way to filter all messages which has a certain set of receptients. We can add support for such filters later. I think that its ok if you send a message to 5 of your friends, that that shows up in the thread for each of those friends.

(In reply to Jonas Sicking (:sicking) from comment #33) I had a discuss with Borja last night. I think we can both agree that "a message thread is consist of multiple messages with exactly the same participants", right? So let's based on this rule and find a proper solution for now and the future. Things happen here do not always related to MMS, or multiple recipients, but have the idea in mind and decide a plan that is suitable for both SMS and MMS would be the best. So actually, the following statement breaks the rule and I don't really think that's ok. > For received MMSs I think we could just add one entry for the > sending number. Now, having only SMS stored in database, the implementation of getMessages() returns messages "either sender or receiver matches any of the specified filtering phone numbers". That's etched in the code. We can survive in SMS world and fix this bug without changes to the code because there is only one phone number you can get from the returned items of getThreadList. One side effect: when you click the message thread that you sent to yourself, it will return all messages in database instead. Yes, we can surely fix this by remove yourself from the multi-keys of "numberIndex", and that's good enough for SMS. So that's it. We fix this bug with the participant and thread table and remove yourself from "numberIndex". But we keep the "current" semantics of SmsFilter for now and ever. I propose we do two things to complete message threads API: 1) Cursor-based getThreadList() 2) Return a numeric "thread id" instead of a "senderOrReceiver" string, along with an array of participant addresses, in each item returned by getThreadList. 3) Add a new API getThreadMessages() that takes only one parameter -- thread id. Cursor-based, of course. I really hope we can keep the current behaviour of SmsFilter so that we can implement something described in bug 771463 comment #2, which will also apply to bug 771458 as well.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #35) > (In reply to Jonas Sicking (:sicking) from comment #33) > > I had a discuss with Borja last night. I think we can both agree that "a > message thread is consist of multiple messages with exactly the same > participants", right? I know that this is how many SMS/MMS applications behave in other systems. But I actually dislike this a lot. Often what happens is that if I send the same message to a bunch of friends, this is a single message that has 5 specific recipients and so a new thread is created for this message. But when one of my friends respond, this friend will only send the message to me. And so response appears in the separate thread that is only between me and this friend. The result is that the message that I sent and the response ends up in different threads. This is very annoying. This happens both for MMS and SMS and happens on both iOS and Android. However this is a UX decision and I'm happy to go with whatever solution the UX team decides. Ideally the API wouldn't mandate either behavior though. However I don't see that we have time to deal with these types of issues right now. This bug needs to be fixed for v1.0.1. That means that we have to have it completely fixed for the 28th. That means that if there are any changes to the API, then we have to land significantly before that so that we have time to rewrite things in the SMS app. Additionally we need a few days of margin to fix any issues that we find once the API has landed and once the SMS app has been updated. That means that we have to do the minimal thing that we can do in order to fix the PhoneNumberJS issue. > > For received MMSs I think we could just add one entry for the > > sending number. > > Now, having only SMS stored in database, the implementation of getMessages() > returns messages "either sender or receiver matches any of the specified > filtering phone numbers". That's etched in the code. We can survive in SMS > world and fix this bug without changes to the code because there is only one > phone number you can get from the returned items of getThreadList. > > One side effect: when you click the message thread that you sent to > yourself, it will return all messages in database instead. Yes, we can > surely fix this by remove yourself from the multi-keys of "numberIndex", and > that's good enough for SMS. So that's it. We fix this bug with the > participant and thread table and remove yourself from "numberIndex". > > But we keep the "current" semantics of SmsFilter for now and ever. I propose > we do two things to complete message threads API: > > 1) Cursor-based getThreadList() > 2) Return a numeric "thread id" instead of a "senderOrReceiver" > string, along with an array of participant addresses, in > each item returned by getThreadList. > 3) Add a new API getThreadMessages() that takes only one > parameter -- thread id. Cursor-based, of course. > > I really hope we can keep the current behaviour of SmsFilter so that we can > implement something described in bug 771463 comment #2, which will also > apply to bug 771458 as well. Do we really have time to make all of these changes, as well as followups in the SMS app, as well as fixing any regressions and other issues that we find, in time for the 28th? Wouldn't it be simpler to keep the current API and just change the implementation so that the number matching is fuzzy? I don't think we have enough time to design an API which will be the final standardized API right now anyway, so we don't need to aim for the perfect API. We just need to AIM for doing something which works for the v1.0.1 release. As I have discussed on the SysApps mailing list, I don't think a filter-based API is a good solution as it ends up being either too complicated and slow, or being too specific for a particular UI. That said, you are the module owner so it's your decision.

(In reply to Jonas Sicking (:sicking) from comment #36) > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #35) > > (In reply to Jonas Sicking (:sicking) from comment #33) > > > > I had a discuss with Borja last night. I think we can both agree that "a > > message thread is consist of multiple messages with exactly the same > > participants", right? > > I know that this is how many SMS/MMS applications behave in other systems. > But I actually dislike this a lot. Often what happens is that if I send the > same message to a bunch of friends, this is a single message that has 5 > specific recipients and so a new thread is created for this message. > > But when one of my friends respond, this friend will only send the message > to me. And so response appears in the separate thread that is only between > me and this friend. The result is that the message that I sent and the > response ends up in different threads. This is very annoying. This happens > both for MMS and SMS and happens on both iOS and Android. > > However this is a UX decision and I'm happy to go with whatever solution the > UX team decides. Yes, I share your frustration, the behaviour in all the devices is annoying. That seems to be an indication we can do something better here but also a signal that is not easy to do as none did it before. There was a User Story for sending SMS to multiple recipients for v1.1 (bug 837994) but I think given 1.1 timeframes we should rather focus in other aspects that have less impact on the UX. > > Wouldn't it be simpler to keep the current API and just change the > implementation so that the number matching is fuzzy? > > I don't think we have enough time to design an API which will be the final > standardized API right now anyway, so we don't need to aim for the perfect > API. We just need to AIM for doing something which works for the v1.0.1 > release. As I have discussed on the SysApps mailing list, I don't think a > filter-based API is a good solution as it ends up being either too > complicated and slow, or being too specific for a particular UI. I totally agree with Jonas, can we focus on fixing current issues and look for a perfect solution (doh) later on?

In my previous reply: (In reply to Vicamo Yang [:vicamo][:vyang] from comment #35) > So that's it. We fix this bug with the participant and thread table and remove yourself from > "numberIndex". That's all.

Vicamo: It's unclear to me if you are proposing that we change the current API in this bug or not? I.e. are you proposing that we change getThreadList to return a thread-id and introduce a new getThreadMessages? Like I said, it's your and Borja's decision since you are the owner of the backend and front-end code. So if that is what you guys think is the quickest and safest solution then do go ahead.

(In reply to Jonas Sicking (:sicking) from comment #39) > Vicamo: It's unclear to me if you are proposing that we change the current > API in this bug or not? I.e. are you proposing that we change getThreadList > to return a thread-id and introduce a new getThreadMessages? I suggested changing APIs in future release, just not today. Let's just move discuss about the definition of a message thread to bug 837994.

correct more names

1) Move per method fixes to other patches 2) table definition changes: 2.1) use 'id' as primary key name of threadStore & participantStore instead of 'participantId' and 'threadId' 2.2) add 'participantAddresses' to participantStore 2.3) s/body/subject/ in threadStore 2.4) drop "number" index from messageStore 3) insert both international & national numbers to participant store at upgrading schema 4) try find a matching participant record before creating one at upgrading schema 5) rewrite findParticipantRecordByAddresses(). Hope it's much clearer than before.

still working on filtering with multiple numbers on my github branch: https://github.com/vicamo/b2g_releases-mozilla-central/tree/bugzilla/833291/master

done, refactoring some code to avoid copy arrays.

[m-c] try https://tbpl.mozilla.org/?tree=Try&rev=172d8829ed1c

Comment on attachment 719652 [details] [diff] [review] [m-c] Part 1/7: fix namings Review of attachment 719652 [details] [diff] [review]: ----------------------------------------------------------------- rs=me (rs stands for 'rubberstamp'). I didn't really look at the changes here since it's just a search'n'replace. So the rs=me only applies if there *only* search'n'replace changes and whitespace fixes here, and nothing else.

Vicamo: I'm sorry, this change is too big for me to be comfortable with reviewing. We really need someone who knows this code better than me to review it given how important it is that it'll work.

(In reply to Jonas Sicking (:sicking) from comment #55) > [m-c] Part 1/7: fix namings > Review of attachment 719652 [details] [diff] [review]: > ----------------------------------------------------------------- > I didn't really look at the changes here since it's just a search'n'replace. > So the rs=me only applies if there *only* search'n'replace changes and > whitespace fixes here, and nothing else. There are also two trivial changes in around line 178. Original code: ensureDB: function ensureDB(callback) { ... let objectStore = event.target.transaction.objectStore(STORE_NAME); self.upgradeSchema(objectStore); ... }, upgradeSchema: function upgradeSchema(objectStore) { ... }, becomes: ensureDB: function ensureDB(callback) { ... self.upgradeSchema(event.target.transaction); ... }, upgradeSchema: function upgradeSchema(transaction) { let messageStore = transaction.objectStore(MESSAGE_STORE_NAME); ... }, to avoid redefine |objectStore| in ensureDB().

(In reply to Jonas Sicking (:sicking) from comment #56) > Vicamo: I'm sorry, this change is too big for me to be comfortable with > reviewing. We really need someone who knows this code better than me to > review it given how important it is that it'll work. I know, that's why I want to let you know my plans in previous comments. Finding other appropriate reviewers.

Comment on attachment 719653 [details] [diff] [review] [m-c] Part 2/7: add thread, participant stores Review of attachment 719653 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +502,5 @@ > + self.lastThreadId += 1; > + let threadRecord = { > + id: self.lastThreadId, > + participantIds: [participantRecord.id], > + patticipantAddresses: [number], part not patt. @@ +519,5 @@ > + if (!messageCursor) { > + // No more message records, check next most recent record. > + mostRecentCursor.continue(); > + return; > + } nit, add an empty line @@ +524,5 @@ > + let messageRecord = messageCursor.value; > + // Check whether does the message really belong to this thread. > + let matchSenderOrReceiver = false; > + if (messageRecord.delivery == DELIVERY_RECEIVED) { > + if (messageRecord.sender == number) { nit, use if (.. && ..) @@ +534,5 @@ > + if (!matchSenderOrReceiver) { > + // Check next message record. > + messageCursor.continue(); > + return; > + } nit, add an empty line @@ +792,5 @@ > > + getAliasPhoneNumbers: function getAliasPhoneNumbers(aAddresses) { > + let aliases = aAddresses; > + for each (let address in aAddresses) { > + if (aliases.indexOf(address) < 0) { When will this happen? Since aliases = aAddresses in line 794 @@ +802,5 @@ > + continue; > + } > + let nationalNumber = parsedNumber.nationalNumber; > + if (nationalNumber && (nationalNumber != address) > + && (aliases.indexOf(nationalNumber) < 0)) { nit, align '&&' Also could you explain this if check? @@ +825,5 @@ > + }; > + if (DEBUG) { > + debug("createParticipantRecord: creating " > + + JSON.stringify(participantRecord)); > + } nit, add an extra line @@ +834,5 @@ > + } > + }, > + > + findParticipantRecordByAddresses: function findParticipantRecordByAddresses( > + aParticipantStore, aPossibleAddresses, aCreate, aCallback) { why not just replace aPossibleAddresses to aAddresses? @@ +851,5 @@ > + (function find(index) { > + if (index >= aPossibleAddresses.length) { > + if (aCreate) { > + self.createParticipantRecord(aParticipantStore, aPossibleAddresses, > + aCallback); I would suggest you move 'create' outside this function, and make this function does 'find' only.

Comment on attachment 719653 [details] [diff] [review] [m-c] Part 2/7: add thread, participant stores Review of attachment 719653 [details] [diff] [review]: ----------------------------------------------------------------- I'm a bit concerned that the changes here seem bigger than needed to fix this bug. In particular moving away from the mostRecentRequest objectStore to the thread objectStore. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +84,5 @@ > + let cursor = event.target.result; > + if (!cursor) { > + if (DEBUG) { > + debug("Could not get the last key for '" + storeName + "' store. " + > + "Probably empty database."); This doesn't seem like it's an error. Having an empty message database seems like a normal situation when the user turns on the phone for the first time, no? Simply setting the idAttributeName to 0 in that case seems like the right solution. @@ +88,5 @@ > + "Probably empty database."); > + } > + return; > + } > + that[idAttributeName] = cursor.key || 0; Since you .bind to 'this' you can simply use this[idAttributeName] here. @@ +102,5 @@ > + }; > + > + initLastId.call(this, MESSAGE_STORE_NAME, "lastMessageId"); > + initLastId.call(this, THREAD_STORE_NAME, "lastThreadId"); > + initLastId.call(this, PARTICIPANT_STORE_NAME, "lastParticipantId"); In general, you should be able to use the autoIncrement feature of IndexedDB rather than manually tracking lastXId. @@ +789,5 @@ > } > return false; > }, > > + getAliasPhoneNumbers: function getAliasPhoneNumbers(aAddresses) { In general, I'm not sure if the approach in this function will work. As I understand it, it tries to generate all aliases for a phone number. However this seems hard if we start with a local phone number since we don't know which international prefixes it might belong to. That's why I was proposing doing a fuzzyMatch(number1, number2) function instead. However I'm still looking over the patch to try to understand everything so it may very well be that the current approach works. @@ +793,5 @@ > + getAliasPhoneNumbers: function getAliasPhoneNumbers(aAddresses) { > + let aliases = aAddresses; > + for each (let address in aAddresses) { > + if (aliases.indexOf(address) < 0) { > + aliases.push(address); Note that aliases and aAddresses start out pointing to the *same* array. I.e. not two arrays which contains the same numbers, but literally the same array. That doesn't seem like your intent? @@ +805,5 @@ > + if (nationalNumber && (nationalNumber != address) > + && (aliases.indexOf(nationalNumber) < 0)) { > + if (aliases.length == aAddresses.length) { > + // Make a copy here. > + aliases = aAddresses.slice(); Couldn't this end up losing all the aliases that you have accumulated so far? @@ +829,5 @@ > + } > + let request = aParticipantStore.add(participantRecord); > + if (aCallback) { > + request.onsuccess = aCallback.bind(null, participantRecord); > + request.onerror = aCallback.bind(null, null); You don't need to wait for the success value of the .add call. If the add fails the transaction will automatically get aborted. So just call aCallback(participanRecord) manually and directly here.

Comment on attachment 719655 [details] [diff] [review] [m-c] Part 3/7: fix saveRecord() Review of attachment 719655 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +869,5 @@ > })(0); > }, > > + findParticipantIdsByAddresses: function findParticipantIdsByAddresses( > + aParticipantStore, aParticipantAddresses, aCreate, aSkipNonexistent, aCallback) { I'd prefer move create outside this function, also could you add comments for aSkipNonexistent? @@ +875,5 @@ > + debug("findParticipantIdsByAddresses(" > + + JSON.stringify(aParticipantAddresses) + ", " > + + aCreate + ", " + aSkipNonexistent + ")"); > + } > + (function findParticipantId(addresses, result) { Why don't you move result outside this local function? @@ +919,5 @@ > + if (DEBUG) debug("findThreadRecordByParticipants: " + participantIds); > + if (!participantIds) { > + aCallback(null, null); > + return; > + } nit , put an extra line.

Comment on attachment 719653 [details] [diff] [review] [m-c] Part 2/7: add thread, participant stores Review of attachment 719653 [details] [diff] [review]: ----------------------------------------------------------------- So if I understand the algorithms here correctly, we basically consider two messages to be from the same number of they are either the same exact number, or if the local version of either number matches the other number, or matches the local version of the other number. I.e. two numbers are considered to be the same if number1 == number2 || localVersion(number1) == number2 || number1 == localVersion(number2) || localVersion(number1) == localVersion(number2) Though the code ends up performing that calculation more elegantly. Is that an accurate description of the algorithm? ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +860,5 @@ > + } > + let address = aPossibleAddresses[index++]; > + let range = IDBKeyRange.only(address); > + let request = aParticipantStore.index("addresses") > + .openCursor(range, NEXT); You could simply use a .get(address) here rather than a cursor with a keyrange

1) Remove getAliasPhoneNumbers() and rewrite matching algorithm in findParticipantRecordByAddress() 2) Use get() instead of openCursor() when possible.

Comment on attachment 719658 [details] [diff] [review] [m-c] Part 7/7: fix createMessageList() removing these per comment 58.

Comment on attachment 719896 [details] [diff] [review] [m-c] Part 2/7: add thread, participant stores : v2 Review of attachment 719896 [details] [diff] [review]: ----------------------------------------------------------------- Please renominate if I misunderstand something. Implementation algorithms aside, it'd be good to document what general matching rules the patch is implementing. See comment 62 ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +863,5 @@ > + let participantRecord = cursor.value; > + for each (let storedAddress in participantRecord.addresses) { > + let match = false; > + if (parsedAddress) { > + if (storedAddress.endsWith(parsedAddress.nationalNumber)) { Doing substring matches generally won't work will it? I thought that often the national number was something like: 08123456 with the international number being +468123456 I.e. often the national number inserts an extra digit to indicate that there's an area code, but this digit is dropped in the international number.

(In reply to Jonas Sicking (:sicking) from comment #65) > [m-c] Part 2/7: add thread, participant stores : v2 > Review of attachment 719896 [details] [diff] [review]: > ----------------------------------------------------------------- > Doing substring matches generally won't work will it? I.e. often the national number inserts > an extra digit to indicate that there's an area code, but this digit is dropped in the > international number. My comments have already addressed this. International number "+886987654321" has phonenumberutil parsed national number "987654321" only.

1) rs=sicking 2) re-gen patch with hg.

1) Use autoIncrement in thread & participan store. Applying autoIncrement in message store have to re-create the store and is opened as another bug 833291 2) Remove createParticipantRecord() because it's only called in findParticipantRecordByAddress()

1) Rebase to changes made in comment #63 and comment #68. 2) Don't slice() whenever possible.

fix runtime exception

1) Rebase 2) Call onNextMessageInListGotCb() and multiFiltersGotCb() instead because getting empty results from findParticipantIdsByAddresses() doesn't mean it's an error.

How are the reviews coming along here?

Comment on attachment 720719 [details] [diff] [review] [m-c] Part 2/7: add thread, participant stores : v3 Review of attachment 720719 [details] [diff] [review]: ----------------------------------------------------------------- This part introduces the participant and thread tables and the algorithm for internationalized phone numbers. Looks good to me. Thank you! ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +418,5 @@ > cursor.continue(); > }; > }, > > + upgradeSchema7: function upgradeSchema7(db, transaction) { Please give a general description. @@ +420,5 @@ > }, > > + upgradeSchema7: function upgradeSchema7(db, transaction) { > + /** > + * This "participant" object store keep mappings of multiple phone numbers nit: s/keep/keeps @@ +477,5 @@ > + } > + > + let mostRecentRecord = mostRecentCursor.value; > + > + // Each entry in mostRecentStore is supposed to be an unique thread, so nit: s/an unique/a unique @@ +508,5 @@ > + return; > + } > + > + let messageRecord = messageCursor.value; > + // Check whether does the message really belong to this thread. nit: should be "Check whether the message really belongs to ..." @@ +857,5 @@ > + } > + } > + if (!match) { > + continue; > + } Add an empty line here to improve readability.

Comment on attachment 720720 [details] [diff] [review] [m-c] Part 3/7: fix saveRecord() : v2 Review of attachment 720720 [details] [diff] [review]: ----------------------------------------------------------------- Searching for the right thread for a message, nice fix! ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1072,2 @@ > aMessage.readIndex = [FILTER_READ_UNREAD, timestamp]; > + // threadIdIndex & participantIdsIndex are filled in saveRecord(). Move this line to line#1070. @@ +1097,2 @@ > aMessage.readIndex = [FILTER_READ_READ, timestamp]; > + // threadIdIndex & participantIdsIndex are filled in saveRecord(). Move this line to line#1095.

Comment on attachment 720727 [details] [diff] [review] [m-c] Part 7/7: fix createMessageList() : v2 Review of attachment 720727 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1474,5 @@ > + self.findParticipantIdsByAddresses(participantStore, filter.numbers, > + false, true, > + function (participantIds) { > + if (!participantIds || !participantIds.length) { > + // Oops! No such participant at all. An additional empty line here should help improve readability.

Address review comments.

Address review comments

No change, re-gen from hg, r=hsinyi

No change, re-gen from hg, r=hsinyi

No change, r=hsinyi

Address review comments. Try https://tbpl.mozilla.org/?tree=Try&rev=95f49f466163

https://hg.mozilla.org/integration/mozilla-inbound/rev/b59dda6608ab https://hg.mozilla.org/integration/mozilla-inbound/rev/4b11fa628d03 https://hg.mozilla.org/integration/mozilla-inbound/rev/e13b9f6eddee https://hg.mozilla.org/integration/mozilla-inbound/rev/23d5f8aff7c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/28bd1c831210 https://hg.mozilla.org/integration/mozilla-inbound/rev/917a2c1cad20 https://hg.mozilla.org/integration/mozilla-inbound/rev/ed4682d0df21

b2g18_v1_0_1 rebase working branch: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/833291/b2g18_v1_0_1

https://hg.mozilla.org/mozilla-central/rev/b59dda6608ab https://hg.mozilla.org/mozilla-central/rev/4b11fa628d03 https://hg.mozilla.org/mozilla-central/rev/e13b9f6eddee https://hg.mozilla.org/mozilla-central/rev/23d5f8aff7c5 https://hg.mozilla.org/mozilla-central/rev/28bd1c831210 https://hg.mozilla.org/mozilla-central/rev/917a2c1cad20 https://hg.mozilla.org/mozilla-central/rev/ed4682d0df21

Vicamo, it would be great if you could document what matching algorithm between phone numbers these patches ended up implementing. It would be really great if QA could test that the behavior is matching those algorithms, and also get feedback from Telefonica to verify that they match their expectations. What I'm trying to avoid is that we realize that there is some edge case that doesn't work and that we'll fail in certification.

try https://tbpl.mozilla.org/?tree=Try&rev=ace95c4194ad

(In reply to Jonas Sicking (:sicking) from comment #85) > Vicamo, it would be great if you could document what matching algorithm between phone > numbers these patches ended up implementing. Already available in inline comments? > It would be really great if QA could test that the behavior is matching those > algorithms, Already included in marionette test cases in part 7. > and also get feedback from Telefonica to verify that they match their expectations. Mark need info from Borja. > What I'm trying to avoid is that we realize that there is some edge case that doesn't work > and that we'll fail in certification. Yes, I did tried on emulator with message app and: 1) Messages sent to +886987654321 and 0987654321 are stitch up in the same thread. 2) Searching for +886987654321 gets all messages to and from either +886987654321 or 0987654321.

Im gonna try with a BUILD with this patch and I will come back with feedback soon.

(In reply to Borja Salguero [:borjasalguero] from comment #94) > Im gonna try with a BUILD with this patch and I will come back with feedback soon. I've verified again with b2g18_v1_0_1 tip[1] and will push later. [1]: https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a0eb57759a92

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6cbee21ebc00 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6c87a6dda120 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8eca0ed4b601 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/902c7e30650e https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/447f34657ca2 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c93130995ea0 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6a00a4d8f017

You didn't land this on b2g18...

Vicamo, this needs to land on b2g18 as well (as per the tree rules). Please finish your uplift or I will back this out of b2g18_v1_0_1. https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_tef.2B.2Fshira.2B_bugs_for_v1.0.1_.28updated_2.2F13.29

(In reply to Ryan VanderMeulen [:RyanVM] from comment #98) > Vicamo, this needs to land on b2g18 as well (as per the tree rules). Please > finish your uplift or I will back this out of b2g18_v1_0_1. > https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_tef.2B. > 2Fshira.2B_bugs_for_v1.0.1_.28updated_2.2F13.29 Let me have some time mark dependency and re-nominate leo? flag of prerequisite bugs, ok?

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #99) > Let me have some time mark dependency and re-nominate leo? flag of > prerequisite bugs, ok? In the future, please don't land until you're fully ready to do so.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #99) > Let me have some time mark dependency and re-nominate leo? flag of prerequisite bugs, ok? Have to rebase patches for b2g18. Will take some time. Working on it.

Let's uplift bug 844429 (also leo+) first.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/99dbe0b16152 https://hg.mozilla.org/releases/mozilla-b2g18/rev/e2af7d45e6b1 https://hg.mozilla.org/releases/mozilla-b2g18/rev/dbaa78c74a75 https://hg.mozilla.org/releases/mozilla-b2g18/rev/bbe37d0a9d72 https://hg.mozilla.org/releases/mozilla-b2g18/rev/3ce5eb4ba38c https://hg.mozilla.org/releases/mozilla-b2g18/rev/25a6eaebfec7 https://hg.mozilla.org/releases/mozilla-b2g18/rev/d6b0665bc52d

Added [NO_UPLIFT] per recent commercial RIL compatibility issue. Waiting on further decision to keep the patch in b2g18 or to back it out. ------------------------------ If we really want to back them out, backing the following MMS bugs should be enough to make the commercial RIL compatible: Bug 854422 - B2G MMS: should call .NotifyResponseTransaction() with MMS_PDU_STATUS_RETRIEVED after an MMS is retrieved under the RETRIEVAL_MODE_AUTOMATIC mode (a follow-up for bug 845643) Bug 850680 - B2G MMS: broadcast "sms-received" and "sms-sent" system messages Bug 850530 - B2G MMS: Use the same attribute name for delivery (s/state/delivery) like SMS Bug 852911 - B2G MMS: fail to expose correct nsIDOMMozMmsMessage.attachments. Bug 853725 - B2G MMS: fail to read nsIDOMMozMmsMessage.receivers for a received MMS (a follow-up of bug 849741). Bug 853329 - B2G MMS: other Android phones cannot read attachments sent from FFOS Bug 852471 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) (follow-up fix) Bug 852460 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event (follow-up fix) Bug 849741 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event Bug 847756 - B2G MMS: provide nsIDOMMobileMessageManager.markMessageRead(). Bug 847736 - B2G MMS: provide nsIDOMMobileMessageManager.delete(). Bug 847738 - B2G MMS: provide nsIDOMMobileMessageManager.getMessage(). Bug 844431 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) Bug 845643 - B2G MMS: Save retrieved MMS into database. Bug 792321 - Check max values of MMS parameters in sendRequest. Bug 833291 - B2G SMS & MMS: getMessages it's not working with PhoneNumberJS Bug 844429 - B2G SMS & MMS: move SMS codes into dom/mobilemessage to make it generic for MMS Bug 839436 - B2G MMS: make DB be able to save MMS messages.

Confirming with Michael to see we should back out to Bug 839436 or just Bug 844431 (if we eventually decide to back out). Please see Bug 857632, comment #17.

Per off-line discussion with Michael, we decided not to back out the MMS bugs that have already been in mozilla-b2g18. Removing [NO_UPLIFT] to make the check-in status sync'ed.

That image doesn't seem relevant and this bug is resolved.