1091113 - Don't merge obsolete entity fields

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect, P2)

Description

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

Our fairly dummy l10n-merge in FirefoxOS is currently merging whole entities irrelevant of their fields.

That causes problems when the obsolete entity in ab-CD contains different fields than new en-US.

A solution would be to look into entity and copy only the fields (value/attributes) that are in source locale.

Comment 1

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

Attachment: pull request

So, what this patch does is that it changes how we do our naive l10n-merge.

With it, we will look into the top-level structure of each entity and verify if it has the same fields. If the structure of the entity is different, we'll pick the source entity.

That also matches closer to how it will work in l20n where we will have the concept of malformed entity (entity with public fields not matching source entity).

For now, it will fix the problem of obsolete entities with values leaking into result locales and causing UI regressions.

Comment 2

[Staś Małolepszy :stas]

Comment on attachment 8513942 [details] [review]
pull request

I commented in the PR, although pretty please attach textual diffs in Bugzilla in the future.  r=me with comments addressed.

Comment 3

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

Comment on attachment 8513942 [details] [review]
pull request

Thanks Stas for the review!

Comment 4

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

Commit: https://github.com/zbraniecki/gaia/commit/d1984db195e4fdd681819ea033c4a57d5afcd25b
Merge: https://github.com/mozilla-b2g/gaia/commit/3b3179b06cd85b3548688b67dde050fe20b2753a

Comment 5

[Staś Małolepszy :stas]

I landed this in the l20n.js repo as well:

https://github.com/l20n/l20n.js/commit/c81d1981d74708e5d8e56c825fc996c6b777304b

Zibi, I thought you were going to land the version with keys1.every() ?

Comment 6

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

(In reply to Staś Małolepszy :stas from comment #5)
> I landed this in the l20n.js repo as well:
> 
> https://github.com/l20n/l20n.js/commit/
> c81d1981d74708e5d8e56c825fc996c6b777304b

Oh, thanks!

> Zibi, I thought you were going to land the version with keys1.every() ?

No, since we decided not to go for two loops, but rather length match plus one loop I don't think that an external function makes sense and inline loop does not inflate the stack.

Comment 7

[Staś Małolepszy :stas]

I agreed with you not to do (keys1.every && keys2.every).  However, I thought we said keys1.every was okay.  It's one loop, a much cleaner functional approach and is only one line rather than 3 which leak the implementation of for-loop.

Comment 8

[Staś Małolepszy :stas]

Anyways, this had my r+, so it's okay, I just wanted to clarify my position and my preference on the style.  I do think keys1.every is easier to read and we're not even remotely close to care about the call stack here.

Comment 9

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

Yeah, I'm afraid that it's a difference in our preferences - imperative vs. declarative.

Yours seems to be driven by code readability, mine on CPU cycles, performance [1], and memory.

[1] http://jsperf.com/every-vs-loop222