870739 - Polishing multi-resolution support

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Ismael Gonzalez [:basiclines]]

As we've landed the patches from https://bugzilla.mozilla.org/show_bug.cgi?id=83064
There is a few issues related with missing images, and some visual tweaks that should be fixed in order to close the work done to support multi-resolution in Gaia.

Comment 1

[Ismael Gonzalez [:basiclines]]

Attachment: Polishing PR

We are waiting to receive updated @2x assets from Sergi. But we can start with the code review

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 747923 [details]
Polishing PR

Looks good. But we shouldn't be touching Message app this week I think (given the fact they are working hard on MMS work).

Comment 3

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 747923 [details]
Polishing PR

looks good as the issue on github has been resolved. Thank you!

Comment 4

[Ismael Gonzalez [:basiclines]]

The changes about sms should not be related with the work done with MMS. Anyway i will wait to talk with Borja to confirm it. Thx!

Comment 5

[Dale Harvey (:daleharvey)]

I assumed after the last huge multi resolution bug that we would split these into seperate PR's per application?

Comment 6

[Ismael Gonzalez [:basiclines]]

Hey Dale, this is going to be the only patch that we will land for this bug (then close). Will not be like the multi resolution one. But yes is way more easy to handle it in one PR as all changes are small fixes and missing images.

Comment 7

[Dale Harvey (:daleharvey)]

Comment on attachment 747923 [details]
Polishing PR

The scrubber in the video player still has an issue with a background gradient that doesnt reach the full height on high resolution devices.

I am happy to r+ this as it isnt a regression and we can fix in a follow up.

(needs rebased)

Comment 8

[Dale Harvey (:daleharvey)]

Actually testing this on the unagi, it is a regression, the scrubber should be fixed before this is merged, but seeing the size of the PR happy for you to merge it once you have rebasd / fixed / tested. Cheers

Comment 9

[Ismael Gonzalez [:basiclines]]

Sure Dale i'll work on it!
Thx!

Comment 10

[Ismael Gonzalez [:basiclines]]

Merged in master: https://github.com/mozilla-b2g/gaia/commit/ca0af90cf372af3dff159e5cd140f5fa4c1620c4

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Ismael from comment #10)
> Merged in master:
> https://github.com/mozilla-b2g/gaia/commit/
> ca0af90cf372af3dff159e5cd140f5fa4c1620c4

This has created a regression in clock performance on . Can you back-out the clock part (or I will backout all the PR ;)). 

See https://datazilla.mozilla.org/b2g/?branch=master&device=unagi&range=7&test=cold_load_time&app_list=clock&app=clock&gaia_rev=66f3098da13467b4&gecko_rev=52341e43539a0e8b

Comment 12

[Ismael Gonzalez [:basiclines]]

Checking it right now!
Should i create a new bug and PR for fixing it?

Comment 13

[Ismael Gonzalez [:basiclines]]

The Gaia revision that you are providing (66f3098da13467b4) is not the same as the generated for this PR (ca0af90cf372af3dff159e5cd140f5fa4c1620c4).
In fact this patch reduces load time by 30ms: https://datazilla.mozilla.org/b2g/?branch=master&device=unagi&range=7&test=cold_load_time&app_list=clock&app=clock&gaia_rev=ca0af90cf372af3d&gecko_rev=39c9d5a5c09dfb94

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Ismael from comment #13)
> The Gaia revision that you are providing (66f3098da13467b4) is not the same
> as the generated for this PR (ca0af90cf372af3dff159e5cd140f5fa4c1620c4).
> In fact this patch reduces load time by 30ms:
> https://datazilla.mozilla.org/b2g/
> ?branch=master&device=unagi&range=7&test=cold_load_time&app_list=clock&app=cl
> ock&gaia_rev=ca0af90cf372af3d&gecko_rev=39c9d5a5c09dfb94

I copy the wrong revision number but looking at:

https://datazilla.mozilla.org/b2g/?branch=master&device=unagi&range=7&test=cold_load_time&app_list=clock&app=clock

The last commit in this app since days (weeks?) is this one so it seems deeply related to me :)


(In reply to Ismael from comment #12)
> Checking it right now!
> Should i create a new bug and PR for fixing it?

please.

Comment 15

[Ismael Gonzalez [:basiclines]]

I still don't see the problem on that graphs (i'm not too much familiar with perf-o-matic).
What i see is a variety of commits that introduced slower cold-load-times:

66f3098da13467b4 = 2013-05-24 - 861.87ms - not related with clock app ?
7c52767685ab0fe7 = 2013-05-27 - 1000.33 - not related with clock app ?

Then i see my commit that actually speed up from 880.67ms to 851.83ms:
ca0af90cf372af3d = 2013-05-27 - 851.83 - Polish bug (clock app changes)

Am i missing something?

Comment 16

[Corey Frang [:gnarf]]

Does this need to be included in leo+ ?

Comment 17

[Ismael Gonzalez [:basiclines]]

I dont think so as 830644 is not supposed to be nominated to leo

Comment 18

[Ismael Gonzalez [:basiclines]]

This bug should not be nominated to leo? as it depends on #830644 which has to land only from v1.2

Comment 19

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Please be informed that we did a JS-based responsive design in keyboard app, so it is not required to include shared/style/responsive.css for keyboard app.

Including it may cause incorrect layout on qHD/WVGA resolution, like Bug 874778.

Comment 20

[bhavana bajaj [:bajaj]]

Based on comment 18 , moving leo ? -> koi?