1369544 - Make Services.intl references in toolkit/components/passwordmgr conditional

Status: RESOLVED WONTFIX

(Firefox Build System :: General, enhancement)

Description

[Nick Alexander :nalexander [he/him]]

I witnessed this first in an artifact build, but I now see it in a full build, and think there are multiple things at play.

I am seeing toolkit/ code that assumes Services.intl is present, in particular at http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/toolkit/components/passwordmgr/LoginManagerContent.jsm#1445.  And this is probably reasonable: all of our release configurations have ICU enabled, and thus Services.intl should be available.  But I for reasons I don't understand -- and can't trace, 'cuz the ICU build system is complicated -- my local Android artifact builds and even my local Android compile-environment builds don't expose Services.intl.

Comment 1

[Nick Alexander :nalexander [he/him]]

ted: I think you did the ICU build system work.  Can you confirm that ICU is enabled everywhere?  Is there a reason we can turn it off?  Is it wrong to depend on it?

gandalf: you are knowledgeable about the client-side l10n code, and I think you did a lot of the work to enable and use ICU.  Can you tell me if the toolkit/ code can assume Services.intl is defined?

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Unfortunately no. At the moment we do not have ICU in Android builds and thus Services.intl cannot work.

So, if you're working on toolkit code that will be used by Android you should not use Servics.intl :(

I was working on a project that was supposed to enable us to land ICU in Android and stop inducing the engineering cost of handling ICU/non-ICU duality, but lately there has been no update from the platform PM on the plan.
 Ni'ing :jet and :rnewman for the update on if we could enable ICU now

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> Unfortunately no. At the moment we do not have ICU in Android builds and
> thus Services.intl cannot work.

OK, so it sounds like toolkit/ uses of Services.intl should be forbidden.  MattN, I witnessed about:logins/LoginManager issues with Services.intl on Android; that's how I got here.  Were you aware that Services.intl needs to be guarded?  Are you okay with working around these issues in LoginManager?  (I see uses in DownloadManager as well, but I'll figure out who to bug later, after we have consensus about Services.intl in toolkit/.)

Comment 4

[Richard Newman [:rnewman]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)

>  Ni'ing :jet and :rnewman for the update on if we could enable ICU now

It's sadly out of my hands; I've already opined that we should ship it. Bug 1343725 is the last I saw.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Personally, I'm not convinced that it should. Services.intl provides an important value that we can't provide otherwise and making Firefox Desktop lose it due to a limitation of Fennec seems unwise.

I'd rather see an if-else use of Services.intl and something that Fennec can handle.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> Personally, I'm not convinced that it should. Services.intl provides an
> important value that we can't provide otherwise and making Firefox Desktop
> lose it due to a limitation of Fennec seems unwise.

Just to be clear: you're not convinced that Services.intl should be forbidden in toolkit/.  Correct?

> I'd rather see an if-else use of Services.intl and something that Fennec can
> handle.

I think we may have different notions of "forbidden".  Let me change "Services.intl is forbidden in toolkit/" to  "cannot assume Services.intl exists in toolkit/".  Then we agree, correct?

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Let me change "Services.intl is forbidden in toolkit/" to  "cannot assume Services.intl exists in toolkit/".  Then we agree, correct?

unfortunately, yes :(

Comment 8

[Nick Alexander :nalexander [he/him]]

This doesn't even really seem like a bug; it's just a non-obvious configuration: Bug 1349884 disables Intl on x86 (in order -- I think -- to ensure that we have a tier 1 platform running tests without Intl) and Bug 1343725 disables Intl on non-Nightly builds.

Between the two, those mean that we're shipping busted code in toolkit/ right now that we should address.  Updating the description.

I think all that's needed is to add fallbacks in (some) of the Password Manager toolkit component, following the approach of http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/toolkit/crashreporter/content/crashes.js#83.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Agree with :nalexander

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Services.intl is available everywhere now!