[Bug 811539] [SMS] Delete PhoneNumberJS once will be available in Gecko #7319

Merged
merged 1 commit into from Jan 9, 2013

Conversation

Projects
None yet
2 participants
Member

borjasalguero commented Jan 7, 2013

No description provided.

Member

borjasalguero commented Jan 7, 2013

I will squash the changes after the review.

Member

borjasalguero commented Jan 7, 2013

Contributor

julienw commented Jan 7, 2013

I didn't look very close yet but it looks good as an initial look. Will look better tomorrow.

@julienw julienw commented on an outdated diff Jan 9, 2013

apps/sms/js/sms.js
@@ -6,8 +6,7 @@
var MessageManager = {
init: function mm_init() {
this.initialized = true;
- // Init PhoneNumberManager for solving country code issue.
- PhoneNumberManager.init();
+ // Render threads at the beginning
@julienw

julienw Jan 9, 2013

Contributor

nit: unnecessary comment

@julienw julienw commented on an outdated diff Jan 9, 2013

apps/sms/js/sms.js
MessageManager.getThreads(ThreadListUI.renderThreads);
},
onMessageFailed: function mm_onMessageFailed(e) {
- ThreadUI.onMessageFailed(e.message);
+ var message = e.message;
+ ThreadUI.onMessageFailed(message);
@julienw

julienw Jan 9, 2013

Contributor

nit: unnecessary change

@julienw julienw commented on the diff Jan 9, 2013

apps/sms/js/sms.js
@@ -26,12 +25,20 @@ var MessageManager = {
onMessageSending: function mm_onMessageSending(e) {
var message = e.message;
- ThreadUI.appendMessage(message);
+ var num = message.receiver;
+ if (window.location.hash == '#new') {
+ // If we are in 'new' we go to the right thread
+ // 'num' has been internationalized by Gecko
+ window.location.hash = '#num=' + num;
+ } else {
+ ThreadUI.appendMessage(message);
+ }
@julienw

julienw Jan 9, 2013

Contributor

yeah, better to do that here :)

@julienw julienw commented on an outdated diff Jan 9, 2013

apps/sms/js/sms.js
}
+ setTimeout(function(){
+ ThreadListUI.appendThread(thread);
+ appendThreads(threads, callback);
+ });
@julienw

julienw Jan 9, 2013

Contributor

setTimeout needs a delay parameter here

@julienw

julienw Jan 9, 2013

Contributor

forget about it, it seems legal in HTML5

@julienw julienw and 1 other commented on an outdated diff Jan 9, 2013

apps/sms/js/sms.js
- dayHeaderIndex = 0,
- unreadThreads = [];
-
- for (var i = threads.length - 1; i >= 0; i--) {
- var thread = threads[i];
- var time = thread.timestamp.getTime();
- var num = thread.senderOrReceiver;
-
- var numNormalized =
- PhoneNumberManager.getNormalizedInternationalNumber(num);
-
- if (thread.unreadCount) {
- unreadThreads.push(numNormalized);
+ var dayHeaderIndex;
+ var appendThreads = function(threads, callback) {
+ if (threads.length == 0) {
@julienw

julienw Jan 9, 2013

Contributor

nit: either use !threads.length or threads.length === 0, I don't like non-strict equality ;)

@borjasalguero

borjasalguero Jan 9, 2013

Member

+1. Changing

@julienw julienw commented on the diff Jan 9, 2013

apps/sms/js/sms.js
- var threadIds = [],
- dayHeaderIndex = 0,
- unreadThreads = [];
-
- for (var i = threads.length - 1; i >= 0; i--) {
- var thread = threads[i];
- var time = thread.timestamp.getTime();
- var num = thread.senderOrReceiver;
-
- var numNormalized =
- PhoneNumberManager.getNormalizedInternationalNumber(num);
-
- if (thread.unreadCount) {
- unreadThreads.push(numNormalized);
+ var dayHeaderIndex;
+ var appendThreads = function(threads, callback) {
@julienw

julienw Jan 9, 2013

Contributor

mmm I don't see where you're dealing with unread threads ?

@borjasalguero

borjasalguero Jan 9, 2013

Member

Not we are rendering on the fly! The old part of the code it's not needed using getAllThreads from Gecko ;)

@julienw

julienw Jan 9, 2013

Contributor

you will show me where it's done then :)

Contributor

julienw commented Jan 9, 2013

it bothers me that you're doing both the phone number removing stuff and the asynchronous optimizating stuff... could we make two bugs and two patches ? it would be waayyyy easier to review this.

@julienw julienw commented on the diff Jan 9, 2013

apps/sms/js/sms.js
- // TODO Remove with
- // https://bugzilla.mozilla.org/show_bug.cgi?id=811539
- var numNormalized =
- PhoneNumberManager.getNormalizedInternationalNumber(num);
-
- MessageManager.currentNum = numNormalized;
- this.updateHeaderData();
-
- MessageManager.send(numNormalized, text);
-
- if (window.location.hash == '#new') {
- // If we are in 'new' we go to the right thread
- window.location.hash = '#num=' + numNormalized;
- }
+ // Send the SMS
+ MessageManager.send(num, text);
@julienw

julienw Jan 9, 2013

Contributor

I'd keep the lines:

MessageManager.currentNum = num;
this.updateHeaderData();
@borjasalguero

borjasalguero Jan 9, 2013

Member

Currently, as we are sending SMS, and inside onMessageSending there is a change in the hash, if this change happens it goes directly to https://github.com/borjasalguero/gaia/blob/c40a5c7a53919c3dadb74b62fedb0e9d1ebba008/apps/sms/js/sms.js#L135, and there we have MessageManager.currentNum = num; and inside renderMessages, we have this.updateHeaderData(); as well.

Member

borjasalguero commented Jan 9, 2013

The thing is that they are tied, because in the old code we create the thread object with PhoneNumberJS and getMessages without filters, and it comes directly from getAllThreads with the internationalized Phone Number... so I had to modify the way of rendering the threads. But the modification is small, instead of using a for loop Im using setTimeout for not blocking the UI if the list of threads/messages it's large. I told in the bug that the patch will be not only removing the library, because there were some parts tied to this one! :(!

@julienw julienw commented on the diff Jan 9, 2013

apps/sms/js/sms.js
@@ -1276,10 +1257,14 @@ var ThreadUI = {
// Remove 'sending' style and add 'error' style
var aElement = messageDOM.querySelector('a');
+ // Check if it was painted as 'error' before
+ if (!aElement.classList.contains('sending')) {
@julienw

julienw Jan 9, 2013

Contributor

I'm not sure this is adequate.

if you do this, you won't get the resendHandler back (as we remove it when we resend a message), neither the airplane mode notification.

I think this is a "optimization" that we don't need anyway, so please remove it.

Member

borjasalguero commented Jan 9, 2013

In fact the optimization DOM stuff is tracked in other branch and other bugs, I have the patch ready for creating the PR once this one will be fixed!

@julienw julienw commented on an outdated diff Jan 9, 2013

apps/sms/js/utils.js
if (contact) { // we have a contact
- //TODO what if there are more contacts?
+ //What if there are more contacts?
@julienw

julienw Jan 9, 2013

Contributor

why remove "TODO" ?

if something must be done here, please file a bug and put the reference here
if there is nothing to do, just remove the comment :)

@julienw julienw and 1 other commented on an outdated diff Jan 9, 2013

apps/sms/js/utils.js
var name = contact.name,
phone = contact.tel[0],
carrierToShow = phone.carrier;
// Check which of the contacts phone number are we using
for (var i = 0; i < contact.tel.length; i++) {
- if (PhoneNumberManager.getOptionalNumbers(
- contact.tel[i].value).indexOf(number) != -1) {
+ // Based on E.164 (http://en.wikipedia.org/wiki/E.164)
+ var tempPhoneNumber = contact.tel[i].value;
+ if (number.length > 7) {
+ var rootPhoneNumber = number.substr(number.length - 8);
@julienw

julienw Jan 9, 2013

Contributor

you can use number.substr(-8) here

@borjasalguero

borjasalguero Jan 9, 2013

Member

+1. Changed.

@julienw julienw commented on an outdated diff Jan 9, 2013

apps/sms/js/utils.js
var name = contact.name,
phone = contact.tel[0],
carrierToShow = phone.carrier;
// Check which of the contacts phone number are we using
for (var i = 0; i < contact.tel.length; i++) {
- if (PhoneNumberManager.getOptionalNumbers(
- contact.tel[i].value).indexOf(number) != -1) {
+ // Based on E.164 (http://en.wikipedia.org/wiki/E.164)
+ var tempPhoneNumber = contact.tel[i].value;
+ if (number.length > 7) {
+ var rootPhoneNumber = number.substr(number.length - 8);
+ } else {
+ var rootPhoneNumber = number;
+ }
@julienw

julienw Jan 9, 2013

Contributor

I don't like all that, it looks like this should be done in gecko when we get back a contact... or that we must spec that a contact should be normalized

@julienw julienw commented on the diff Jan 9, 2013

apps/sms/js/contacts.js
@@ -9,26 +9,12 @@ var ContactDataManager = {
@julienw

julienw Jan 9, 2013

Contributor

please add 'use strict'; in this file

@borjasalguero

borjasalguero Jan 9, 2013

Member

Good catch! I didnt realize that this was missing. :S

Contributor

julienw commented Jan 9, 2013

please fix gjslint alerts too

Contributor

julienw commented Jan 9, 2013

I don't understand why you can't use the old way of displaying threads when they come from Gecko...

Contributor

julienw commented Jan 9, 2013

but keep it now that I reviewed; just next time you should try very hard to not mix two complex stuff like that, it makes it more difficult to review and therefore to land...

@borjasalguero borjasalguero pushed a commit that referenced this pull request Jan 9, 2013

Francisco Borja Salguero Castellano Merge pull request #7319 from borjasalguero/remove_phonenumberjs_berlin
[Bug 811539] [SMS] Delete PhoneNumberJS once will be available in Gecko
3204365

@borjasalguero borjasalguero merged commit 3204365 into mozilla-b2g:master Jan 9, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment