Closed Bug 1019970 Opened 7 years ago Closed 7 years ago

Clean up connstate l10n

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Attached file pull request
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.
Attachment #8433668 - Flags: review?(arthur.chen)
Comment on attachment 8433668 [details] [review]
pull request

f=me. But I would like the system app owner aware of the change.
Attachment #8433668 - Flags: review?(arthur.chen)
Attachment #8433668 - Flags: review?(alive)
Attachment #8433668 - Flags: feedback+
Blocks: 992473
Comment on attachment 8433668 [details] [review]
pull request

I'd rather to have l10n guy aware.
Attachment #8433668 - Flags: review?(alive)
Attachment #8433668 - Flags: review+
Attachment #8433668 - Flags: feedback?(l10n)
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.
Attachment #8433668 - Flags: feedback?(l10n) → feedback-
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.
Attachment #8433668 - Flags: review?(alive)
Attachment #8433668 - Flags: review+
Attachment #8433668 - Flags: feedback?(l10n)
Attachment #8433668 - Flags: feedback-
Attachment #8433668 - Flags: feedback+
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?
Flags: needinfo?(alive)
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.
Attachment #8433668 - Flags: feedback?(l10n) → feedback+
(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.
Flags: needinfo?(alive) → needinfo?(arthur.chen)
Attachment #8433668 - Flags: review?(alive) → review+
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.
Flags: needinfo?(arthur.chen)
thanks!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.