Closed Bug 613477 Opened 14 years ago Closed 13 years ago

Make the primary Star UI (bookmarked indicator) asynchronous

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fixed-in-places])

Attachments

(1 file, 4 obsolete files)

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.
Note: this is only about the star, not the panel that is opened by the star.
Asking blocking for Places branch, for reads reduction and main-thread locking.
blocking2.0: --- → ?
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Keywords: uiwanted
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.
note: browser_bug432599.js and browser_410196_paste_into_tags.js will have to be updated.
Attached patch wip v0.1 (obsolete) — Splinter Review
just saving work, this won't do anything special.
Uh, yeah, this blocks.  Wow.
blocking2.0: ? → betaN+
>- 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.
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.
>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?
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.
(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.
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.
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")
Blocks: livemarksIO
Attached patch patch v1.0 (obsolete) — Splinter Review
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.
Attachment #491855 - Attachment is obsolete: true
Attached patch patch v1.1 (obsolete) — Splinter Review
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/mak77@bonardo.net-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.
Attachment #492058 - Attachment is obsolete: true
Attachment #492318 - Flags: ui-review?(faaborg)
Attachment #492318 - Flags: review?(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
Attachment #492318 - Flags: review?(sdwilsh) → review+
This patch is updating exists tests, so we'll have coverage when it lands :)
Flags: in-testsuite?
(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 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.
Attachment #492318 - Flags: ui-review?(faaborg) → ui-review+
Attached patch patch v1.2 (obsolete) — Splinter Review
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.
Attachment #492318 - Attachment is obsolete: true
Attached patch patch v1.2Splinter Review
the real 1.2
Attachment #492385 - Attachment is obsolete: true
http://hg.mozilla.org/projects/places/rev/ac0b81efdb00
Flags: in-testsuite? → in-testsuite+
Whiteboard: [fixed-in-places]
Depends on: 614083
Blocks: 614940
Depends on: 615419
Blocks: 585704
Summary: Make the primary Star UI async. → Make the primary Star UI (bookmarked indicator) asynchronous
Blocks: FF2SM
http://hg.mozilla.org/mozilla-central/rev/ac0b81efdb00
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b9
No longer blocks: FF2SM
Depends on: 620683
Blocks: 621802
Depends on: 624734
You need to log in before you can comment on or make changes to this bug.