1019970 - Clean up connstate l10n

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

Description

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

In anticipation for bug 992473 I'd like to try to get this in, since it's a separate cleanup patch.

The logic of lockscreen connstate code relies on textContent being empty instead of data-l10n-id being set.

There are also three states which are not localizable.

Lastly, the mock_l10n in unit tests does not set data-l10n-id at all.

Comment 1

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

Attachment: pull request

Comment 2

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

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

Arthur, can you review this?

According to blame, you're the last person who touched this code, which makes you an owner of it in my eyes ;)

I'd like to land it separately from MutationObservers, but the code will work without any alternations after we land that.

Comment 3

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

f=me. But I would like the system app owner aware of the change.

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

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

I'd rather to have l10n guy aware.

Comment 5

[Axel Hecht [:Pike]]

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

Not sure what localizers would do with operator or cell-broadcast?

Also, given that it's unclear, they should have a comment.

Frankly, I'd just drop both, not sure we're solving a problem there.

Comment 6

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

Hi Axel, thanks for your feedback. I don't know how I missed that I named the entity the same as a variable.

I agree we should not introduce non-localizable entities. I reworked my patch to cover directly injected textContent in order to avoid unnecessary l10n entities.

I'm re-requesting f and r.

Comment 7

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

by the way. Alive, while working on that, I noticed that LockScreenConnInfoManagerPrototype.updateConnStates is fired a lot.

 - it seems to be fired 6 times at phone boot before the screen shows up
 - it is fired 4 times when turning on airplane mode
 - it is fired 3 times when turning off airplane mode

Is that a bug or per design?

Comment 8

[Axel Hecht [:Pike]]

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

f=me on the changes in the l10n file, and the absence of l10n wrappers that are no-ops in all locales.

I didn't really dive into the rest of the code.

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Zibi Braniecki [:gandalf] from comment #7)
> by the way. Alive, while working on that, I noticed that
> LockScreenConnInfoManagerPrototype.updateConnStates is fired a lot.
> 
>  - it seems to be fired 6 times at phone boot before the screen shows up
>  - it is fired 4 times when turning on airplane mode
>  - it is fired 3 times when turning off airplane mode
> 
> Is that a bug or per design?

Transfer needinfo, arthur could confirm.

Comment 10

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

Commit: https://github.com/mozilla-b2g/gaia/commit/bf988c38bf6d9316a1b95859ca88e30fb66005d3
Merge: https://github.com/mozilla-b2g/gaia/commit/536b3da91de6d864d162415433ffea316ed7618c

Thanks guys!

Leaving this bug open for ni=arthur

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

The connection states are determined by many attributes provided by gecko. The states should be updated when the events corresponding the attributes occur. When booting up or toggling airplane mode, gecko send those events, so that the state get updated frequently. This is neither a bug nor by design. However, we could improve it by making the requests of refresh queued for a short period of time to reduce the updating frequency.

Comment 12

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

thanks!