1197454 - Update Video to be ready for L10n API v3

Status: RESOLVED FIXED

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

Description

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

Mostly remove mozL10n.get and l10n_date to Intl

Comment 1

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

Attachment: pull request

Comment 2

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

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

this patch completes the changes in Media apps (Gallery has been refactored in bug 1170973 and Camera in bug 1197019).

David, can you review it?

Comment 3

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

:djf, of course I only wanted to ask you to review the latter patch. The first in this PR is being reviewed by :stas in bug 1208698.

Comment 4

[David Flanagan [:djf]]

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

Transferring the review to Punam.

Comment 5

[Punam Dahiya [:pdahiya]]

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

Hi Zibi

Patch looks good and has my r+ . Its verified for gallery and video changes. A nit noted in github on comment. 

I observed in video app similar as noted in gallery group headers missing support for 'run time mirrored' pseudo locale (https://bugzilla.mozilla.org/show_bug.cgi?id=1170973#c7)

Also, did you happen to check video app launch performance with the patch. For some reason I can't see raptor results on tree herder.

Thanks for the patch!

Comment 6

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

Sure, I tested it on Flame-kk, runs=31

Master:
| Metric                | Mean     | Median | Min    | Max    | StdDev  | p95      |
| --------------------- | -------- | ------ | ------ | ------ | ------- | -------- |
| navigationLoaded      | 3641.935 | 3684   | 2979   | 3826   | 183.850 | 3778     |
| navigationInteractive | 3658.903 | 3701   | 2996   | 3844   | 183.829 | 3794.950 |
| visuallyLoaded        | 3940.452 | 3984   | 3233   | 4125   | 181.229 | 4083.250 |
| contentInteractive    | 3940.710 | 3984   | 3233   | 4125   | 181.299 | 4084.250 |
| fullyLoaded           | 4077.871 | 4108   | 3321   | 4248   | 186.748 | 4226.300 |
| rss                   | 39.703   | 39.625 | 39.543 | 42.109 | 0.440   | 39.695   |
| pss                   | 22.531   | 22.471 | 22.386 | 24.475 | 0.357   | 22.538   |
| uss                   | 17.693   | 17.645 | 17.559 | 19.305 | 0.296   | 17.711   |

patch:
| Metric                | Mean     | Median | Min    | Max    | StdDev  | p95      |
| --------------------- | -------- | ------ | ------ | ------ | ------- | -------- |
| navigationLoaded      | 3558.290 | 3586   | 2829   | 3667   | 141.218 | 3630.650 |
| navigationInteractive | 3575.194 | 3603   | 2846   | 3684   | 141.229 | 3646.750 |
| visuallyLoaded        | 3919.355 | 3951   | 3293   | 4014   | 127.037 | 4010.750 |
| contentInteractive    | 3919.613 | 3951   | 3293   | 4014   | 127.084 | 4010.750 |
| fullyLoaded           | 4063.548 | 4089   | 3415   | 4166   | 132.326 | 4160.500 |
| uss                   | 17.906   | 17.898 | 17.801 | 17.992 | 0.050   | 17.973   |
| rss                   | 40.452   | 40.445 | 40.352 | 40.535 | 0.050   | 40.523   |
| pss                   | 23.007   | 23     | 22.903 | 23.093 | 0.050   | 23.075   |

I think the results look good. I'm mostly happy about lower stdev.

Comment 7

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

> I observed in video app similar as noted in gallery group headers missing support for 'run time mirrored' pseudo locale (https://bugzilla.mozilla.org/show_bug.cgi?id=1170973#c7)

Yeah, date/time in pseudo will use en-US locale for now. We plan to tackle that in the next release.

Comment 8

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

Thanks for the review!

Commit: https://github.com/mozilla-b2g/gaia/commit/e4b0d234a8aec8fd58dd836f68db0323b503bd92