613477 - Make the primary Star UI (bookmarked indicator) asynchronous

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

The Star is updated at each page load, synchronously. The queries we do are expensive too since we look for bookmarks and livemarks in fancy ways. We read far more than what we need.

Since all reads and writes are serialized, it is also possible this causes thread locking on page load.

We can do some things:
- reduce the times the star has to update, by better internal tracking of bookmarks.
- make the isBookmarked check use a specialized async query while we wait for a official bookmarks service API. We can put a util method in PlacesUtils and in future it will just forward to the API.
- add a third "disabled" status to the star, that will activate during its updates, the star won't be clickable when it is updating. So we end up with "bookmarked", "not bookmarked", "updating/disabled" (need feedback from UX team). Indeed we don't want the user to click on a not bookmarked star that will shortly become bookmarked.

Comment 1

[Marco Bonardo [:mak]]

Note: this is only about the star, not the panel that is opened by the star.

Comment 2

[Marco Bonardo [:mak]]

Asking blocking for Places branch, for reads reduction and main-thread locking.

Comment 3

[Marco Bonardo [:mak]]

this seems to be our Txul killer, Talos numbers are starting to appear, see measure compared to Places branch tip http://tinyurl.com/2ujv9ek and compared to central (last merge) http://tinyurl.com/348v5vb
Notice the patch I pushed was just completely making the star a zombie.

Comment 4

[Marco Bonardo [:mak]]

note: browser_bug432599.js and browser_410196_paste_into_tags.js will have to be updated.

Comment 5

[Marco Bonardo [:mak]]

Attachment: wip v0.1

just saving work, this won't do anything special.

Comment 6

[Shawn Wilsher :sdwilsh]

Uh, yeah, this blocks.  Wow.

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

>- add a third "disabled" status to the star, that will activate during its
>updates, the star won't be clickable when it is updating. So we end up with
>"bookmarked", "not bookmarked", "updating/disabled" (need feedback from UX
>team). Indeed we don't want the user to click on a not bookmarked star that
>will shortly become bookmarked.

Is the disabled state only an issue on page load when we are checking, or would we pass through the disabled state when the user is creating a new bookmark as well?

I'm thinking that simply having the star not present might work ok since we already have a decent amount of visual activity over there (hovered hyperlink, stop button).  But if we encounter the disabled state at other times, then having it disappear obviously wouldn't work.

Comment 8

[Alex Limi (:limi) — Firefox UX Team]

OK, so assuming I understand this correctly:

* In general, we are in the "unknown" state sometime between clicking a link and usually in good time before the page load completes.
* There will be some worst-case scenarios if you have disk thrashing and I/O issues where this might be pretty late, but they shouldn't be very common.

We have two options here, I think:

1) Making the "unknown" state not show the star (in active or inactive state) until it's ready. The new star design from Stephen's mockups is much more discreet, so it shouldn't add a lot of visual noise to put it back.

2) Keep the previous state until we know better, and compensate if they click it before we know for sure — ie. we just leave the star in its default state, and if you click it before we have the correct state, we make sure that it doesn't toggle, but just does a no-op if it's already in the opposite state from when it was clicked.

I prefer #2 to reduce visual noise and flicker, but if the first implementation is easier done as #1, we can handle that in a follow-up.

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

>2) Keep the previous state until we know better, and compensate if they click
>it before we know for sure

only downside is that a double click is going to break entirely, since we can't show the panel yet.  However, this might be worth cutting down on visual noise otherwise.  What kind of lag are we expecting to be normal?

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

Also, the windows and linux stars are pretty light right now, so it would be easy to confuse with a disabled representation (and it sounds like the OS X star is getting lighter as well), so I think everyone agrees that a disabled version of hte star wouldn't really work.

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> only downside is that a double click is going to break entirely, since we can't
> show the panel yet.  However, this might be worth cutting down on visual noise
> otherwise.  What kind of lag are we expecting to be normal?
Normally, it should be pretty quick.  If the disk is being thrashed though, it might be more noticeable to the user.

Comment 12

[Marco Bonardo [:mak]]

so, yeah, when a new page is loaded (so a new focused tab opens, or you type and go on a address, or a new window open) the status is unknown, in the common state this unknow status could last for some millisecond, in some rare case could take more, but I'd say not more than 200ms looking at other async UIs (I'm mostly guessing from experience).
This unknown status could also last when the bookmarked status changes, but I'm still looking at the code since I think once the initial status is setup I can rely on notification to always update without querying... Thus the unknown would end up being limited to the first load of the page.

Comment 13

[Marco Bonardo [:mak]]

I could have a testing patch for monday, and I could give you a build to check how much it's noticeable (maybe I could hide the star since as Alex said make it disabled could be confused with "unstarred")

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This should do it, still missing test changes.

I think hiding the star works pretty well, I've done so that we need to async query only if a new page loads, or when customization ends. In all other cases the change is immediate and tracked internally. I feel like I'm not focused on the star when I load a new page, I don't even notice it missing and appearing, but could be me :)

I'll make a try build when I have fixed tests, if you can't directly apply this patch to test it.

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Builds will appear here as soon as they are ready (max a couple hours from now): http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-a17aabc424b9

Notes for UI-review:
- DO NOT use these builds with your profile, in any case. We have a database change here that could break it when you come back to central. A new profile will be fine.
- I've set the flag to faaborg, but feel free to change/forward to limi or any other ux-team member that wants to look at this.

Notes for code review:
* the tagging service change is just removing a warning I saw while fixing tests, since tag.id is a getter overwriting it is useless and wrong.
* I fixed PlacesUtils shutdown observer since it was throwing errors in those tests, due to the fact transactionManager was wrongly adding 2 observers (looks like some sort of merge error). It is much better and less error-prone now, allowing to register shutdown functions. I use it to finalize the statement.
* I had to reorder a bit the functions in browser_410196_paste_into_tags.js because that test was impossible to follow. It still sucks, but it's a bit better.

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 492318 [details] [diff] [review]
patch v1.1

>+++ b/browser/base/content/browser-places.js
>+    PlacesUtils.asyncGetBookmarkIds(this._uri, function (aItemIds) {
nit: lose the space between function and (

>+++ b/browser/base/content/test/browser_bug432599.js
>+function waitForStarChange(aValue, aCallback) {
>+  let starButton = document.getElementById("star-button");
>+  if (starButton.hidden || starButton.hasAttribute("starred") != aValue) {
>+    info("Waiting for star button change.");
>+    setTimeout(arguments.callee, 50, aValue, aCallback);
>+    return;
>+  }
>+  aCallback();
>+}
I think it'd be better to use a mutation event listener here instead of polling.  (this applies to every file you added this method to)

>+++ b/browser/components/places/tests/browser/browser_410196_paste_into_tags.js
Since you are changing all this anyway, lets get rid of makeURI and just use NetUtil.newURI please.

>@@ -15,212 +15,204 @@
>  * The Original Code is Places test code.
>  *
>  * The Initial Developer of the Original Code is Mozilla Corp.
Fix this please to be properly formatted and use foundation.  Could even change it to PD since mozilla owns the copyright on it anyway (and is fine with making tests public domain).

>+++ b/toolkit/components/places/src/PlacesUtils.jsm
>+  /**
>+   * Given a uri returns list of itemIds associated to it.
>+   *
>+   * @param aURI
>+   *        nsIURI or spec of the page.
>+   * @param aCallback
>+   *        function to be called when done.
>+   * @param aScope
>+   *        Scope for the callback.
>+   * @return array of itemIds associated to aURI.
nit: we aren't really returning here.  We pass it as the first argument to aCallback, and the documentation should reflect that.

>+  asyncGetBookmarkIds: function PU_asyncGetBookmarkIds(aURI, aCallback, aScope)
>+  {
>+    let url = aURI instanceof Ci.nsIURI ? aURI.spec : aURI;
nit: move to first use

>+    let db = this.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;
nit: move inside if

r=sdwilsh

Comment 17

[Shawn Wilsher :sdwilsh]

This patch is updating exists tests, so we'll have coverage when it lands :)

Comment 18

[Marco Bonardo [:mak]]

(In reply to comment #16)
> >+++ b/browser/base/content/browser-places.js
> >+    PlacesUtils.asyncGetBookmarkIds(this._uri, function (aItemIds) {
> nit: lose the space between function and (

this is the common js style for anonymous functions

> >+++ b/browser/base/content/test/browser_bug432599.js
> >+function waitForStarChange(aValue, aCallback) {
> I think it'd be better to use a mutation event listener here instead of
> polling.  (this applies to every file you added this method to)

I prefer not using mutation events in the browser window, even if this is a test, since they are never completely cleaned up, and that will slowdown all next tests.

fixed all the rest

Comment 19

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 492318 [details] [diff] [review]
patch v1.1

This seems to work well enough to land, IMO — I can't really tell a significant difference compared the existing behavior, especially since it isn't shown and then filled in, but seems to check before being shown at all. This is similar to how it already works in the current (and less efficient) approach.

Comment 20

[Marco Bonardo [:mak]]

Attachment: patch v1.2

addresses comments, fixes a random orange, added back try/catch to init/uninit based on past review requests from Seamonkey Places merge.
should be ready to land.

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v1.2

the real 1.2

Comment 22

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/ac0b81efdb00

Comment 23

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ac0b81efdb00