618715 - bookmark toggle is blocking the UI

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Bookmarks, defect)

Description

[David Ascher (:davida)]

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.

Comment 1

[Thomas Arend [:tarend]]

I agree. I see this too - it should be much faster and not block the UI

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

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.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Wrong WIP? Looks like the patch from bug 621802.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

Oops, here's the correct patch.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: patch

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.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 8

[Matt Brubeck (:mbrubeck)]

Pushed, with an added try/catch around insertBookmark:
http://hg.mozilla.org/mobile-browser/rev/7e3b5ef18e90

Filed followup bug 622130 to remove Bookmark* events.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 500261 [details] [diff] [review]
patch

r=mfinkle based on comment 7

Comment 10

[Aakash Desai [:aakashd]]

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.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

It won't be fixed. This bug is about blocking the UI, which is should be better now.

Comment 12

[Aakash Desai [:aakashd]]

verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20100105 Namoroka/4.0b9pre Fennec/4.0b4pre