Closed
Bug 1089538
Opened 10 years ago
Closed 6 years ago
[Contacts] The thumbnail is slowly appeared when scrolls contact list
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect, P1)
Tracking
(tracking-b2g:backlog)
RESOLVED
WONTFIX
tracking-b2g | backlog |
People
(Reporter: edchen, Assigned: arcturus)
References
()
Details
(Keywords: perf, Whiteboard: [2.1-bug-bash][TPE][p=3][perf-wanted])
Attachments
(3 files)
73 bytes,
text/plain
|
cawang
:
feedback+
fang
:
feedback+
|
Details |
2.68 KB,
patch
|
Details | Diff | Splinter Review | |
7.53 KB,
patch
|
jlorenzo
:
qa-approval-
|
Details | Diff | Splinter Review |
*** Build Information Gaia-Rev 1e48e3e40e0780c0cd07a3457e5fe2efeeb542d1 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/09fb60a37850 Build-ID 20141023001201 Version 34.0 Device-Name flame Base image: ***Please update this field to say if you're running the v180 or the v188 base image*** *** Description The thumbnail is slowly appeared when scrolls contact list. *** Steps to Reproduce 1. Flash the gaia 2. Make reference-workload-light 3. Launch contact app 4. Scroll the contact list *** Expected Results The thumbnail should be normal displayed. *** Actual Results The thumbnail is slowly appeared when scrolls contact list. *** Other Notes *** Reproduction Frequency: 100%
Comment 1•10 years ago
|
||
Do we have any numbers about the objectives in terms of performance for this feature?
Flags: needinfo?(francisco)
Whiteboard: [2.1-FC-bug-bash][TPE] → [2.1-bug-bash][TPE]
Assignee | ||
Comment 2•10 years ago
|
||
Yes, contacts list will never try to go under 60fps when scrolling. Right now we are around 58-60.
Flags: needinfo?(francisco)
Comment 3•10 years ago
|
||
Edward, can you get a video of the perceived perception of this issue? while francisco comments that scrolling is inline with 60 fps expectations, i'd like to see how bad the user behavior is.
Updated•10 years ago
|
Flags: needinfo?(edchen)
Reporter | ||
Comment 4•10 years ago
|
||
Tony - Uploaded video. http://youtu.be/CUq1aqbN3I8
Flags: needinfo?(edchen)
Assignee | ||
Comment 5•10 years ago
|
||
I think we should take a look to this bug in next sprint. At least see how much effort we need to put here.
Severity: normal → major
blocking-b2g: --- → backlog
Target Milestone: --- → 2.2 S9 (3apr)
Comment 6•10 years ago
|
||
Agree with backlog. not showing the thumbnails while scrolling normally isnt a good user experience. but i woudlnt block as it eventually does load.
Assignee | ||
Updated•10 years ago
|
Target Milestone: 2.2 S9 (3apr) → 2.1 S9 (21Nov)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → francisco
Whiteboard: [2.1-bug-bash][TPE] → [2.1-bug-bash][TPE][p=4]
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → NEW
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
Hei Carrie, Fang could you take a look to how it looks the new way of loading images. On the right you can see the current behaviour, on the left, default images appear instantly and then we preload any other image. Do you agree with this behavior? Any suggestion? I still need to properly measure memory and fps but to me it's looking promising
Attachment #8522314 -
Flags: feedback?(fshih)
Attachment #8522314 -
Flags: feedback?(cawang)
Comment 8•10 years ago
|
||
Comment on attachment 8522314 [details]
PoC for default thumbnail appearing fast
Hi Francisco,
Of course the left one is much better. Actually, a similar issue happens in Email as well - the texts will slowly appear while scrolling. Hence, they display grey lines to replace the texts lines and then the real texts appear. I think our approach here is based on the same logic. It works fine to me. Thanks!
Attachment #8522314 -
Flags: feedback?(cawang) → feedback+
Comment 9•10 years ago
|
||
Comment on attachment 8522314 [details]
PoC for default thumbnail appearing fast
For the visual part looks good to me as well! Thanks!
Attachment #8522314 -
Flags: feedback?(fshih) → feedback+
Comment 12•10 years ago
|
||
Dear Francisco, Could you do me a faver to attach the patch in commet 7 to this bugzilla or email me, I can‘t reach location of www.youtube.com in my network. Thanks a lot.
Flags: needinfo?(francisco)
Assignee | ||
Comment 13•10 years ago
|
||
Flags: needinfo?(francisco)
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Shiwei Zhang from comment #12) > Dear Francisco, > Could you do me a faver to attach the patch in commet 7 to this bugzilla > or email me, I can‘t reach location of www.youtube.com in my network. > Thanks a lot. Hi Shiwei, just attached the current patch (not ready to land): https://bug1089538.bugzilla.mozilla.org/attachment.cgi?id=8525958
Updated•10 years ago
|
Target Milestone: 2.1 S9 (21Nov) → 2.2 S1 (5dec)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [2.1-bug-bash][TPE][p=4] → [2.1-bug-bash][TPE][p=3]
Comment 15•10 years ago
|
||
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #14) > > Hi Shiwei, > > just attached the current patch (not ready to land): > > https://bug1089538.bugzilla.mozilla.org/attachment.cgi?id=8525958 Dear Francisco, I havd a test on this patch, default-photo(initial photo) display synchronously, but thumbnails(pictures I selected from gallery) still display slowly. However, there is no similar issue on v1.4. Is there any thing we can do to improve this patch?
Comment 18•10 years ago
|
||
Dear Francisco, Could you help me to review this patch(bug367054_contact_photo_display_slow.patch). I think and test patch_1089538 didn't realize the effect that item display default "group" background before displaying the real photo, but this patch realize it. In this patch, I make some changes, some of them are same with patch_1089538, and the other changes are mainly used to optimize the display speed and solve some problems. One problem is when we scroll contact list, we can find that default background appear faster than the photos, so there is a "blank" time for some items. To avoid the blank, I add and use interface displayDefaultPhoto() before finishing load the photo. Thanks a lot.
Attachment #8541991 -
Attachment description: Display default background before load the thumbnail. → bug367054_contact_photo_display_slow.patch
Updated•9 years ago
|
Target Milestone: 2.2 S1 (5dec) → ---
Comment 20•9 years ago
|
||
A dupe of this bug has been filed recently. It seems the activity on this bug has stopped a couple of weeks ago. Francisco, do you think the PoC is good enough to be landed? Do you want some checks by QA before landing it?
Flags: needinfo?(francisco)
Assignee | ||
Comment 21•9 years ago
|
||
Hi Johan, we have even a modification of the PoC from a contributor. But we didn't have much time to work with it. Do you think is possible to have a QA test on that?
Flags: needinfo?(francisco)
Comment 22•9 years ago
|
||
Comment on attachment 8541991 [details] [diff] [review] bug367054_contact_photo_display_slow.patch I think I can help out here ;)
Attachment #8541991 -
Flags: qa-approval?(jlorenzo)
Comment 23•9 years ago
|
||
Comment on attachment 8541991 [details] [diff] [review] bug367054_contact_photo_display_slow.patch Review of attachment 8541991 [details] [diff] [review]: ----------------------------------------------------------------- This patch helps to reduce the time to load the default image but increase notably the time of the cold start of the app (see this video as a comparison: http://mzl.la/1ErcZ2s). It wasn't the case in the video called "PoC for default thumbnail appearing fast". So I would prefer to get the implementation demonstrated in this video rather than this one. Also, the patch changes the color of the circle and leaves some ambiguous documentation. Francisco, can we go with patch_1089538.diff? ::: apps/communications/contacts/js/views/list.js @@ +1018,5 @@ > + if (defaultImage) { > + renderDefaultPhoto(img, link, group); > + if (link.dataset.visited !== 'true') { > + // To avoid display blank, first display default picture. > + // modify for bug367054. Please do not mention a bug which is not on bugzilla.mozilla.org. Bug 367054 exists and is not related to this change. @@ +1035,5 @@ > return; > } > > if (img) { > + //delete img.dataset.group; Commented code. @@ +1043,5 @@ > } > > + //var figure = photoTemplate.cloneNode(true); > + //img = figure.children[0]; > + //setImageURL(img, photo); Commented code. @@ +1048,2 @@ > > + //link.insertBefore(figure, link.children[0]); Commented code. @@ +1119,5 @@ > setImageURL(img, null); > }; > > + // Display default group photo. > + // add for bug367054 Please do not mention a bug which is not on bugzilla.mozilla.org. Bug 367054 exists and is not related to this change. ::: apps/communications/contacts/style/contacts.css @@ +765,4 @@ > left: 0; > width: 100%; > height: 100%; > + color: #00f; Why do we change the color of the text and the background? ::: shared/js/contacts/utilities/image_loader.js @@ +113,5 @@ > --imgsLoading; > image.style.backgroundImage = 'url(' + src + ')'; > if (tmp.complete) { > + // Delete group background when item will display real photo. > + // modify for bug367054. Please do not mention a bug which is not on bugzilla.mozilla.org. Bug 367054 exists and is not related to this change. @@ +129,5 @@ > } > > /** > + * Display default background. > + * add for bug367054. Please do not mention a bug which is not on bugzilla.mozilla.org. Bug 367054 exists and is not related to this change. @@ +134,5 @@ > + */ > + function displayDefaultBackground(item) { > + var image = item.querySelector('span[data-type=img][data-group]'); > + if (image) { > + // var src = '/contacts/style/images/Imagery.png'; Commented code. @@ +135,5 @@ > + function displayDefaultBackground(item) { > + var image = item.querySelector('span[data-type=img][data-group]'); > + if (image) { > + // var src = '/contacts/style/images/Imagery.png'; > + // image.style.backgroundImage = 'url(' + src + ')'; Commented code. @@ +199,5 @@ > > + // If theItem has not display any picture, or > + // if theItem needs to display real photo but has still not display, > + // it need to call loadImage to display. > + // modify for bug367054. Please do not mention a bug which is not on bugzilla.mozilla.org. Bug 367054 exists and is not related to this change.
Attachment #8541991 -
Flags: qa-approval?(jlorenzo) → qa-approval-
Comment 25•9 years ago
|
||
I couldn't test patch_1089538.diff, it doesn't apply anymore on the current master.
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
Updated•9 years ago
|
Priority: -- → P1
Updated•9 years ago
|
Whiteboard: [2.1-bug-bash][TPE][p=3] → [2.1-bug-bash][TPE][p=3][perf-wanted]
Comment 27•6 years ago
|
||
Firefox OS is not being worked on
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•