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

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.

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.

Wrong WIP? Looks like the patch from bug 621802.

Oops, here's the correct 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 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.

Comment on attachment 500261 [details] [diff] [review] patch r=mfinkle based on comment 7

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.

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

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