Bug 828226 - [Dialer] Call logs doesn't show nothing (r=albertopq) #7443

Merged
merged 1 commit into from Jan 9, 2013

3 participants

@gtorodelvalle

There was a problem with the lazy loading of the localization library which cause the usage of the navigator.mozL10n object before it was really created.

The PR also fixes another issue found regarding the filtering of all and missed calls in the Call Log.

@etiennesegonzac etiennesegonzac and 2 others commented on an outdated diff Jan 9, 2013
apps/communications/dialer/js/lazy_l10n.js
},
_waitForLoad: function ll10n_waitForLoad(callback) {
- var self = this;
- window.addEventListener('localized', function onLocalized() {
- window.removeEventListener('localized', onLocalized);
+ window.addEventListener('localized', this._onLocalized.bind(

not that because of the bind you're never removing the listener

@albertopq
Mozilla-B2G member

Why? Is removing the listener from window, isn't it?

I hope you do not freak out with the solution I am going to suggest... ;-)

.bind returns a new (anonymous) function which is != from this._onLocalized

@albertopq
Mozilla-B2G member

Oh sorry! I was thinking on the subject (window), I didn't notice the method to be unregistered. You are totally right

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

@etiennesegonzac , I have included a this._onLocalizedListeners object to host the possible localized listeners to be able to remove them when fired and notified. This is a very rare case which could happen anyhow. Basically, it would require several almost simultaneous invocations of LazyL10n.get . The first one would load the L10n libraries and in case that during the loading new invocations of LazyL10n.get are made, these ones would provoke the execution of the _waitForLoad function since in these LazyL10n.get invocations this._loaded would be false but this._inDOM would be true. We need to store the generated listener to remove them one the event fire and the listener is executed. If you need further explanation, please do not hesitate to contact me.

@gtorodelvalle

I did not want to just host the generated listener directly in a property of LazyL10n to cover the case that more than 1 additional LazyL10n.get invocation is made during the loading of the libraries... ;-999

@etiennesegonzac

Mmmh... wait, why don't we just inline the content of the _onLocalized method in the addEventListener

var finalize = this._finalize.bind(this);
window.addEventListener('localized', function onLocalized() {
    window.removeEventListener('localized', onLocalizedListeners[key]);
    finalize(callback);
});
@gtorodelvalle

@etiennesegonzac I guess you meant this:
var finalize = this._finalize.bind(this);
window.addEventListener('localized', function onLocalized() {
window.removeEventListener('localized', >>>onLocalized<<<);
finalize(callback);
});

Right?

Please check the new update ;)

Thanks!

@etiennesegonzac etiennesegonzac commented on the diff Jan 9, 2013
apps/communications/dialer/js/lazy_l10n.js
});
+ },
+
+ _finalize: function ll10n_finalize(callback) {
+ document.documentElement.lang = navigator.mozL10n.language.code;

last thing, we should check for if (this._loaded) and do an early return.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@etiennesegonzac etiennesegonzac commented on the diff Jan 9, 2013
apps/communications/dialer/js/recents.js
@@ -847,6 +849,7 @@ var Recents = {
groupCalls: function re_groupCalls(olderCallEl, newerCallEl, count, inc) {
olderCallEl.classList.add('hide');
+ olderCallEl.classList.add('collapsed');
count += inc;

are the changes in recents.js from another patch?

Yeah, it fixes something we broke regarding the filtering of the missed calls... :-(

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

Sorry @albertopq I kinda stole the review :/

@albertopq
Mozilla-B2G member

No problem @etiennesegonzac ! 4 eyes better than 2 :)

@gtorodelvalle

Absolutely! ;)

@gtorodelvalle

Just to confirm, @etiennesegonzac , do you mean something like this?

_finalize: function ll10n_finalize(callback) {
if (!this._loaded) {
document.documentElement.lang = navigator.mozL10n.language.code;
document.documentElement.dir = navigator.mozL10n.language.direction;
this._loaded = true;
}
callback(navigator.mozL10n.get);
}

@etiennesegonzac

@gtorodelvalle no I want to avoid calling the callback twice (on the localized and on the onload)

But maybe that was part of the fix... :/

@gtorodelvalle

No, the callback is never called twice or more times... It is invoked once per Lazy10n.get invocation... ;-) In fact, I am removing the this._loaded control in the _finalize function ;-)

@gtorodelvalle

The current updates would be my candidate for merging... ;-)

@etiennesegonzac

Oh yeah, my bad.
Probably a sign that I should let @albertopq bring 2 more eyes to this latest version :)

But looking good.

@gtorodelvalle

;) @albertopq , once you are OK with it and since this is bb+, please be my guest and click the green merge button... :-p Thanks to both of you guys! ;-)

@albertopq
Mozilla-B2G member

Looking good r=me

@gtorodelvalle gtorodelvalle merged commit 22bbbde 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