Closed Bug 968666 Opened 6 years ago Closed 6 years ago

multilocale.js should be a standalone build script which can be executed by macro |run-js-command|


(Firefox OS Graveyard :: Gaia::Build, defect)

Not set



2.0 S1 (9may)
feature-b2g 2.0


(Reporter: yurenju, Assigned: yurenju)



(Whiteboard: [p=5])


(1 file)

after bug 897352 land, we can refactoring webapp-zip.js and migrate l10n feature from webapp-zip to standalone multilocale.js to get clearer build process.

after refactoring, webapp-zip.js should remove all l10n features, and multilocale.js should be a standalone build script which can be executed by macro |run-js-command|
Component: Gaia → Gaia::Build
Yuren, did you get a change to work on this bug last week? Please give out and update here since this bug was listed on your weekly report.
Flags: needinfo?(yurenju.mozilla)
no we are trying to land bug 897352 since sheriff said it broke windows build, I'm negotiating with sheriffs for this issue.
Flags: needinfo?(yurenju.mozilla)
Assignee: nobody → yurenju.mozilla
I'm still working on bug 897352
WIP here, this patch still has some problem for communications app, others looks good.
Target Milestone: --- → 1.4 S6 (25apr)
Alex, could you review this pr?

and Pike, could you give some feedback for this pr?

BTW, this pr cannot be merged to master because commit of bug 968661 was backout, but I think we still can review it first since commit of bug 968661 shouldn't be modified a lot.
Attachment #8410751 - Flags: review?(poirot.alex)
Attachment #8410751 - Flags: feedback?(l10n)
bug 968661 has been resolved.
Depends on: 968661
Comment on attachment 8410751 [details] [review]
github PR:

Looks good to me, but I have some questions in the PR.
Multilocale works fine here but I would like to hear from Pike or any l10n expert to ensure everything is still in place.

From what I understand of current implementation of httpd,
it shouldn't change much to DEBUG=1 support as it seems to still fetch files from source directories.
Attachment #8410751 - Flags: review?(poirot.alex) → review+
Whiteboard: [p=8]
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Whiteboard: [p=8] → [p=5]
I'm utterly slow to review this, sorry. I have a really hard time to wrap my head around what's actually supposed to happen in which stage. Could we add comments to that extent?

Then I could just check if things do what they should, instead of reverse engineering what things are doing, and then seeing if they're doing it right :-/ . Comments in both the Makefile and the js run scripts would help greatly.
There is a document for gaia build process

multilocale should be execute between app-makefile and webapp-optimize, and our goal for this issue is move all build process for multilocale to multilocale.js instead of using l10nManager everywhere.

and find me on IRC if you have any question!
l10n for DEBUG=1 deosn't work since bug 929172 landed.
and I feel a little bit sad because it took a lot of time to implement and no one found this issue in 3 months...
filed bug 1006950 for DEBUG=1 test case
Hi Pike,

today I fix the multilocale issue for DEBUG=1 mode and I also verified it with/without environment variable GAIA_INLINE_LOCALES=0 GAIA_CONCAT_LOCALES=0

so please let me know if you have any question, I really hope this commit can be landed in this week

Flags: needinfo?(l10n)
Comment on attachment 8410751 [details] [review]
github PR:

I ended up doing a full code review, it seems to be almost a rewrite. So some of this might not be regressions or changes.

As in the PR, it'd be nice if the order of dependencies were consistent in Makefile.

In multilocale.js, it looks like localizeManifest would overwrite the title and description for en-US with the value in en-US, if that was locally present? Doesn't sound right. Can you check what happens if a localizer has a partial translation of a Like, description is there, but name isn't.

In webapp-zip.js, I think you can remove localesFile, and the check for it?

That's the stuff I had, I confess I stopped looking in-depth at the point where the PR reached httpd.js :_/

I think this is a feedback+ with nits, I don't think I need another pass over this.
Attachment #8410751 - Flags: feedback?(l10n) → feedback+
Flags: needinfo?(l10n)
Pike, I didn't change that part of code in this pull request so I think that is another bug, I filed bug 1007485 and let's fix on there.
merged, thanks!
Closed: 6 years ago
Resolution: --- → FIXED
feature-b2g: --- → 2.0
You need to log in before you can comment on or make changes to this bug.