Closed Bug 988128 Opened 11 years ago Closed 11 years ago

[CardView][V1.3T] The icons of card view were broken

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3T+, b2g-v1.3 unaffected, b2g-v1.3T verified, b2g-v1.4 unaffected)

VERIFIED FIXED
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3 --- unaffected
b2g-v1.3T --- verified
b2g-v1.4 --- unaffected

People

(Reporter: whsu, Assigned: fabrice)

References

Details

(Whiteboard: 1.3tarakorun1 [systemsfe])

Attachments

(3 files)

Attached image The_icons_of_CV.png (deleted) —
* Description: This bug happened on v1.3T build, and cannot reproduce it on other branches. You will see broken icons after long pressing the home button to show card view. It happened on following app. - Dialer app - Contacts app - Attach the screenshot.(The_icons_of_CV.png) I think it is a side effect of following patch. - https://github.com/mozilla-b2g/gaia/commit/7ffb9e1ccf0af4d944f8562a0267896cf1c75806 * Reproduction steps: 1. Launch the dialer app or contacts apps 2. Long press the home button 3. Check the card view * Expected result: 1. Firefox OS displays the card view 2. A icon displays on the center of card view * Actual result: The icon was broken * Reproduction build:(Tarako Build) - Gaia f6a7afd0d716f7a38dfc1808ee0aab3e06ba2f1d - Gecko fb7149d9b320d939d716011f865ec34661ebc353 - BuildID 20140326061134 - Version 28.1 Thanks!
blocking-b2g: --- → 1.3T?
Whiteboard: 1.3tarakorun1
Blocks: 972216
Whiteboard: 1.3tarakorun1 → 1.3tarakorun1 [systemsfe]
William, can you please confirm whether you have included this Bug 972216? it should only show icons now. Minus for now, if you still see problems after including the fix for bug 972216, please renom. thanks
blocking-b2g: 1.3T? → -
Flags: needinfo?(whsu)
Hi, Joe, As I mentioned on description, this is a side effect of the following commit. - https://github.com/mozilla-b2g/gaia/commit/7ffb9e1ccf0af4d944f8562a0267896cf1c75806 (Bug 972216) Thanks!
Flags: needinfo?(whsu)
(In reply to Joe Cheng [:jcheng] from comment #1) > William, can you please confirm whether you have included this Bug 972216? > it should only show icons now. > Minus for now, if you still see problems after including the fix for bug > 972216, please renom. thanks I think that's the entire point of why this bug got filed - the first build that would have it would be the build generated in Taipei's timezone on 3/26 in the morning, so I think he's got the patch. We also didn't anything like what's seen in the screenshot until after bug 972216 landed, so he's definitely got the patch here.
blocking-b2g: - → 1.3T?
ni? sfoster
Flags: needinfo?(sfoster)
I've seen that happening when the app is killed when going to the card view.
We build the cards view from WindowManager.getRunningApps() and get the icon URL from either app.manifest.icons, or - if there's no manifest - app.icon. And, if neither returns a truthy value we should at least fallback to the default rocket icon which is given in the stylesheet. So the broken icon is odd. FWIW I'm not able to reproduce this here - but I'm testing on an Open-C.
Flags: needinfo?(sfoster)
Ok, so the bug is at https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/system/js/cards_view.js#L82 'origin' is not always a real origin because of apps with multiple entry points, so for the contacts app we concatenate app://communications.gaiamobile.org/contacts/index.html + /contacts/style/icons/Contacts_60.png which doesn't lead to a proper uri. The fix is to do a real url resolution, using 'origin' as a base url.
Attached patch card-icon.patch (deleted) — Splinter Review
Sam, that fixes the issue. Do you know on which branch you landed initially and who reviewed?
Flags: needinfo?(sfoster)
Fabrice, this was bug 972216, it was reviewed by :alive and landed on 1.3t: https://github.com/mozilla-b2g/gaia/commit/f6a7afd0d716f7a38dfc1808ee0aab3e06ba2f1d Thanks for spotting that, my bad luck that I didn't test with an app with multiple entry points I guess. I'm currently working on the follow-up bug 987237 for master/1.5 (and possible uplift to 1.4) which does this "properly" by toggling the use or screenshot or icon via a setting. (Actually, in that work I had the same origin problem and solution and was going to find out more about it today.)
Flags: needinfo?(sfoster)
Attachment #8397315 - Flags: review?(alive)
Comment on attachment 8397315 [details] [diff] [review] card-icon.patch Review of attachment 8397315 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/system/js/cards_view.js @@ +58,4 @@ > */ > function getIconURI(origin) { > var app = runningApps[origin]; > + if (!app) { /me slaps head, shoulda caught this before. @@ +83,5 @@ > > if (iconPath.indexOf('data:') !== 0) { > + // We need to resolve iconPath as a relative url to origin, since > + // origin can be a full url in some apps. > + var base = document.createElement("a"); There is already a getOriginObject in this file which does exactly this, so we could remove duplication by make this var base = getOriginObject(origin); @@ +86,5 @@ > + // origin can be a full url in some apps. > + var base = document.createElement("a"); > + base.href = origin; > + var port = base.port ? (":" + port) : ""; > + iconPath = base.protocol + "//" + base.host + port + iconPath; I think base.host should be base.hostname here?
Attachment #8397315 - Flags: feedback+
blocking-b2g: 1.3T? → 1.3T+
Assignee: nobody → fabrice
Attachment #8397315 - Flags: review?(alive) → review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Thanks for the prompt help! Verified it. * Build Information: (V1.3T) - Gaia 02b97c89dec7a10a955c85b921b2a66181eebb0e - Gecko f8bca24057937f5b09d73512ba1e771011ab4203 - BuildID 20140331124355 - Version 28.1
Status: RESOLVED → VERIFIED
Attached image CardView.png (deleted) —
attach the screenshot (CardView.png)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: