Polishing multi-resolution support

RESOLVED FIXED

Status

Firefox OS
Gaia
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: basiclines, Assigned: basiclines)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

345 bytes, text/html
timdream
: review+
daleharvey
: review+
rexboy
: feedback+
Details
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 747923 [details]
Polishing PR

We are waiting to receive updated @2x assets from Sergi. But we can start with the code review
Attachment #747923 - Flags: review?(timdream)
Attachment #747923 - Flags: feedback?(rexboy)
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).
Attachment #747923 - Flags: review?(timdream) → review+
Depends on: 830644
No longer depends on: 83064
Comment on attachment 747923 [details]
Polishing PR

looks good as the issue on github has been resolved. Thank you!
Attachment #747923 - Flags: feedback?(rexboy) → feedback+
(Assignee)

Comment 4

5 years ago
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!
(Assignee)

Updated

5 years ago
Attachment #747923 - Flags: review?(fbsc)
Attachment #747923 - Flags: review?(dale)
I assumed after the last huge multi resolution bug that we would split these into seperate PR's per application?

Updated

5 years ago
Blocks: 870057
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #747923 - Flags: review?(fbsc) → review?
(Assignee)

Updated

5 years ago
Attachment #747923 - Flags: review?
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)
Attachment #747923 - Flags: review?(dale) → review+
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
(Assignee)

Comment 9

5 years ago
Sure Dale i'll work on it!
Thx!
No longer blocks: 870057
Depends on: 870057
(Assignee)

Comment 10

5 years ago
Merged in master: https://github.com/mozilla-b2g/gaia/commit/ca0af90cf372af3dff159e5cd140f5fa4c1620c4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(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
Flags: needinfo?(igonzaleznicolas)
(Assignee)

Comment 12

5 years ago
Checking it right now!
Should i create a new bug and PR for fixing it?
Flags: needinfo?(igonzaleznicolas)
(Assignee)

Comment 13

5 years ago
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
(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.
Flags: needinfo?(igonzaleznicolas)
(Assignee)

Comment 15

5 years ago
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?
Flags: needinfo?(igonzaleznicolas)
Does this need to be included in leo+ ?
blocking-b2g: --- → leo?
(Assignee)

Comment 17

5 years ago
I dont think so as 830644 is not supposed to be nominated to leo
(Assignee)

Updated

5 years ago
Blocks: 876757
(Assignee)

Updated

5 years ago
Blocks: 877095

Updated

5 years ago
Flags: needinfo?(igonzaleznicolas)
(Assignee)

Comment 18

5 years ago
This bug should not be nominated to leo? as it depends on #830644 which has to land only from v1.2
Flags: needinfo?(igonzaleznicolas)
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.
Based on comment 18 , moving leo ? -> koi?
blocking-b2g: leo? → koi?

Updated

5 years ago
blocking-b2g: koi? → ---
You need to log in before you can comment on or make changes to this bug.