Closed Bug 1029891 Opened 7 years ago Closed 7 years ago

Deleting smartCollections and bookmarks is very slow

Categories

(Firefox OS Graveyard :: Gaia::Homescreen, defect, P1)

All
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: ladamski, Assigned: jlal)

References

Details

(Keywords: perf, uiwanted, Whiteboard: [c=progress p=3 s= u=2.0][systemsfe])

Attachments

(2 files)

When tapping on the X icon to delete smartCollections and bookmarks, the deletion dialog often takes a few seconds to be displayed.  X to delete for normal apps is fast.
blocking-b2g: --- → 2.0?
QA Wanted for a video.
Blocks: 1015336
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?]
Keywords: perf, qawanted
It is not ideal but it is normal due to the implementation. To delete apps the confirm message is displayed in the homescreen app but smartCollections and bookmarks are controlled by other apps via a MozActivity. Nothing to do here from homescreen point of view
I think its problematic for the UI to be unresponsive for several seconds, so we need to find some UX trick to provide feedback to the user while that loads.
Keywords: uiwanted
Severity: normal → blocker
Priority: -- → P1
Whiteboard: [c=progress p=3 s= u=]
blocking-b2g: 2.0? → 2.0+
Whiteboard: [c=progress p=3 s= u=] → [c=progress p=3 s= u=2.0]
According to our goals for meeting responsiveness [1], we want to show this interaction within 140ms. If we cannot, we need to display some kind of indicator that there is some action in progress within 1000ms, e.g. with a spinner, etc.

[1] https://developer.mozilla.org/en-US/Apps/Build/Performance/Firefox_OS_app_responsiveness_guidelines#User_stories
IMO this can be very simple... we just delete the icon after the confirm (without really deleting it) and delete the thing asynchronously.
The problem is that currently we surface the confirm() inside of the collection or bookmark app, and surface that using a MozActivity. Should we move this confirm into the homescreen app instead?
Gregor, do you have anyone that can take ownership of this issue? (FxOS Performance Triage)
Flags: needinfo?(anygregor)
(In reply to Eli Perelman, :Eli from comment #7)
> Gregor, do you have anyone that can take ownership of this issue? (FxOS
> Performance Triage)

Gregor is on PTO, so I'm redirecting this to Candice.
Flags: needinfo?(anygregor) → needinfo?(cserran)
A reasonable approach to me seems to:

  - Move the dialogs to the homescreen (or gaia_grid that is another issue)
  - Use datastore to remove the collections, bookmarks asynchronously
  - Live with the very-very-very edge case of unsuccessful downloads
I can provide a patch today to fix this.
Flags: needinfo?(cserran)
QA Contact: jlal
For best performance we need to move strings around... there is no way to workaround this and get the performance we need (with the same strings).... The ids will move between apps but the strings themselves should remain the same.
Keywords: late-l10n
Hrm- on second thought we can do this without l10n changes if we simply reuse the same string for apps/collections which should not be the end of the world for near-instant downloads. 

<name> and all of its data will be deleted.
Keywords: late-l10n
Needs tests for bookmark/collection removal (we already have some for app installs).
Attachment #8446240 - Flags: feedback?(kgrandon)
Comment on attachment 8446240 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21000

Makes sense to me, thanks.
Attachment #8446240 - Flags: feedback?(kgrandon) → feedback+
Assignee: nobody → jlal
Status: NEW → ASSIGNED
QA Contact: jlal
Comment on attachment 8446240 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21000

Ready for review... I fixed the test failures (in one case removing the unit test in favor of the better integration test) and adding tests for bookmark removal / collection removal.
Attachment #8446240 - Flags: feedback+ → review?(kgrandon)
Please be careful because the message is different when you are removing an app or deleting a bookmark and you are doing the same thing in your pull request. See http://i.imgur.com/itHxCCe.png

Bookmarks:

Remove <name>
Remove <name> form homescreen?

Cancel | Remove

With your implementation here you could close this bug 1029477 because I think that you are fixing it too seeing your code in github
I chatted with UX today about this (the bookmark case at least) changing the strings is less then ideal at this point but we can try something.

@jsavory Are you okay with the above screenshot ? (Thanks Cristian)
Flags: needinfo?(jsavory)
Comment on attachment 8446240 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21000

Hey, you basically have an R+ here, but I do think there is a legit failing test for pinning items to smart collections. I haven't looked into why yet, but can help dig in tomorrow. Nice job.
Attachment #8446240 - Flags: review?(kgrandon) → review+
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking+]
Whiteboard: [c=progress p=3 s= u=2.0] → [c=progress p=3 s= u=2.0][systemsfe]
Ugh - should not attempt to read images at night ... the image above is the current images not what this patch does.
:D I will fix this right now since I have an hour... So happy we have those tests ++
Should be fixed now... There was a typo in collection additions and a missing name change in the collections app.
Assignee: jlal → kgrandon
Target Milestone: --- → 2.0 S5 (4july)
Assignee: kgrandon → jlal
Landed: https://github.com/mozilla-b2g/gaia/commit/a62db8f1a3c52546d30fbd08b5e539676c5fc6c0

There is some UX discussion happening in bug 1029477, but because it appears there are no l10n changes here I think this is a win and we should take it for 2.0. I will also close bug 1029477, but UX can follow-up here or there. Thanks!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(jsavory)
Resolution: --- → FIXED
Keywords: qawanted
Duplicate of this bug: 1030205
Attached video VIDEO0117_Compress.MP4
This issue has been successfully verified on Flame 2.0:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141202000201
Version         32.0
Device-Name     flame
FW-Release      4.4.2

This issue has been successfully verified on Flame 2.1:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141202001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.