Optimize L10n AST contains duplicated entities

RESOLVED FIXED

Status

Firefox OS
Gaia::L10n
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

46 bytes, text/x-github-pull-request
gandalf
: review+
Details | Review | Splinter Review
(Assignee)

Description

4 years ago
It's a regression from bug 1018108.

While switching internal AST of resources from Object to Array, we missed that as a result of that, we combine ASTs of all document contexts in the webapp. When the app has more than one document linking to the same l10n resource, we will duplicate its entities in the result JSON.

The offending code is here: https://github.com/mozilla-b2g/gaia/blob/698e6e8a098cc060b26cd6f25171633c4c7e739d/build/webapp-optimize.js#L140-L142
(Assignee)

Updated

4 years ago
Blocks: 1018108
(Assignee)

Updated

4 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
Created attachment 8543193 [details] [review]
pull request

Simple fix.
Attachment #8543193 - Flags: review?(stas)
Comment on attachment 8543193 [details] [review]
pull request

Nice catch, r=me.
Attachment #8543193 - Flags: review?(stas) → review+
Blocks: 1013831
Reverted for breaking the linter (also present in the gaia-try run): https://github.com/mozilla-b2g/gaia/commit/e127d88898a55f236ad3c6fa13e1105ad48b0f16

Log from try run: http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/gaiabld-2b6157ab647a/gaia-try-linux64_gecko/gaia-try_ubuntu64_vm-b2gdt_test-gaia-linter-bm68-tests1-linux64-build3853.txt.gz

As a side-comment, it seems like code like this should probably come with a unit test?
Status: RESOLVED → REOPENED
Flags: needinfo?(gandalf)
Resolution: FIXED → ---
(Assignee)

Comment 5

4 years ago
(In reply to Kevin Grandon :kgrandon from comment #4)
> Reverted for breaking the linter (also present in the gaia-try run):
> https://github.com/mozilla-b2g/gaia/commit/
> e127d88898a55f236ad3c6fa13e1105ad48b0f16

Agh! Thanks Kevin. Fixing.

> As a side-comment, it seems like code like this should probably come with a
> unit test?

The whole code to build ab-CD.json should. And we don't have that yet. Our current plan is to decouple l10n.js from webapp-optimize after 2.2 and introduce a small module that will create the optimized jsons instead. This will come with tests.
Flags: needinfo?(gandalf)
(Assignee)

Comment 6

4 years ago
Created attachment 8543414 [details] [review]
pull request

Carrying over r+ from stas.
Attachment #8543193 - Attachment is obsolete: true
Attachment #8543414 - Flags: review+
(Assignee)

Comment 7

4 years ago
Commit: https://github.com/zbraniecki/gaia/commit/892967a977c8a274396ae7465937ecc9a3f2b3d3
Merge: https://github.com/mozilla-b2g/gaia/commit/c2bf20d23851d5fda9f8f0ef0267db5f49152376
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.