Closed Bug 618715 Opened 9 years ago Closed 9 years ago

bookmark toggle is blocking the UI

Categories

(Firefox for Android Graveyard :: Bookmarks, defect, major)

All
Android
defect
Not set
major

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: davida, Assigned: mbrubeck)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Using Nexus One, 4.0b3pre (damn, typing in about:support is painful, is there a better way?) Gecko/20101207)

for some reason, there are multi-second delays between showing right sidebar and clicking on the star and the star turning yellow (5-10 seconds).  My places history is probably pretty large.

STRs:

go to about:support, slide right, 
click on star: 
 wait 10 seconds until background dark grey disappear
 wait another 3-4 seconds until star turns yellow

once it's yellow, click on it, dialog shows up fast
 click on remove, wait 4 seconds for the star to turn grey again.
I agree. I see this too - it should be much faster and not block the UI
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Do we have any updated metric after the Places branch merge? did it improve or nothing changed?
How much time is spent in insertBookmark and how much in getMostRecentBookmarkForURI calls (fwiw, this is probably not the fastest way to check for bookmarks). Can somebody take some empirical timing of the time spent by the various calls in cmd_star?
Here there could be backend and frontend code improvements, I'd like to have numbers on what does hurt more and how much.
I've seen how painful bookmarking was on a phone (dietrich's one), but on another phone (ddahl's one) it was practically instant, there must be something making a difference there too.
Attached patch WIP (obsolete) — Splinter Review
This uses the new async bookmark code, so the popup appears instantly.  The star icon still takes a while to update (this could be improved), but it no longer blocks the UI during this time.

This breaks our browser-chrome tests; they will need to be updated.
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Wrong WIP? Looks like the patch from bug 621802.
Attached patch WIP (obsolete) — Splinter Review
Oops, here's the correct patch.
Attachment #500253 - Attachment is obsolete: true
Attached patch patchSplinter Review
This patch toggles the icon and popup immediately, and then inserts the bookmark.  Browser-chrome tests are now green.

One problem is that the user can't tell if there is an error adding the bookmark.  However, this is not really worse than our current code.  (Currently if there is an error, the "Page Bookmarked" popup appears but the star does not change color.)  I'm don't know whether there will ever be an error here in normal operation.
Attachment #500256 - Attachment is obsolete: true
Attachment #500261 - Flags: review?(mark.finkle)
Comment on attachment 500261 [details] [diff] [review]
patch

>diff -r 5efd73e3566d chrome/content/browser-ui.js

>+        PlacesUtils.asyncGetBookmarkIds(bookmarkURI, function (aItemIds) {
>+          if (!aItemIds.length) {
>+            let bookmarkTitle = browser.contentTitle || bookmarkURI.spec;
>+            let bookmarkService = PlacesUtils.bookmarks;
>+            let bookmarkId = bookmarkService.insertBookmark(BookmarkList.panel.mobileRoot, bookmarkURI,
>+                                                            bookmarkService.DEFAULT_INDEX,
>+                                                            bookmarkTitle);
>+

I assume that if something goes wrong in insertBookmark, it will throw. We could catch that and reset the star state.  

>+            // Used for browser-chrome tests
>+            let event = document.createEvent("Events");
>+            event.initEvent("BookmarkCreated", true, false);

I'm not a big fan of this, especially since the bookmark system has an observer system for listening for bookmark "events"

I see we already have "BookmarkRemove" and "BookmarkOpen", but they don't seem to be used.

Can you file a followup bug to remove these "Bookmark*" events? We can switch to bookmark observers where needed or figure out a better way to handle these events.


r+ on this patch, definitely a positive improvement
Pushed, with an added try/catch around insertBookmark:
http://hg.mozilla.org/mobile-browser/rev/7e3b5ef18e90

Filed followup bug 622130 to remove Bookmark* events.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 500261 [details] [diff] [review]
patch

r=mfinkle based on comment 7
Attachment #500261 - Flags: review?(mark.finkle) → review+
I'm still seeing it take about 4-5 seconds before the bookmark star is highlighted yellow. Re-opening since the bug is titled, "bookmark toggle is very slow" and that hasn't been fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
It won't be fixed. This bug is about blocking the UI, which is should be better now.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Summary: bookmark toggle is very slow → bookmark toggle is blocking the UI
verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20100105 Namoroka/4.0b9pre Fennec/4.0b4pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.