Last Comment Bug 702926 - Remove spinner from AwesomeBar
: Remove spinner from AwesomeBar
Status: RESOLVED FIXED
[good first bug][Snappy:P1]
:
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Thomas Prip Vestergaard [:prip]
:
Mentors:
Depends on: 713828
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-16 03:41 PST by Alex Limi (:limi) — Firefox UX Team
Modified: 2012-02-01 13:58 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
My first patch (622 bytes, patch)
2011-12-03 17:45 PST, Thomas Prip Vestergaard [:prip]
no flags Details | Diff | Splinter Review
Remove spinner from AwesomeBar (6.64 KB, patch)
2011-12-06 14:11 PST, Thomas Prip Vestergaard [:prip]
no flags Details | Diff | Splinter Review
Remove spinner from AwesomeBar (13.54 KB, patch)
2011-12-07 12:47 PST, Thomas Prip Vestergaard [:prip]
margaret.leibovic: feedback+
Details | Diff | Splinter Review
Bug 702926 - Remove spinner from AwesomeBar (15.00 KB, patch)
2011-12-08 15:08 PST, Thomas Prip Vestergaard [:prip]
margaret.leibovic: review+
Details | Diff | Splinter Review

Description Alex Limi (:limi) — Firefox UX Team 2011-11-16 03:41:03 PST
When considering responsiveness and "perceived performance", one of the things that makes Firefox seem slow at times is the spinner we show when a Places lookup is still happening. 

Somewhat anecdotally, it seems to that our lookup performance is now so good that it rarely shows up — and when it does, it's for a very very short time, and it adds a feeling of slowness instead of doing something useful. Most of the time it spins for under a second, and when it does, the results don't change after it's done spinning anyway.

Going back to the principle of not showing more states than necessary (time perception = number of change events), it seems like this makes us seem slower than we actually are.

I suggest we remove the spinner from the AwesomeBar searches at this point, since it seems a meaningless now that there aren't many (any?) pathological cases left.
Comment 1 Thomas Prip Vestergaard [:prip] 2011-12-03 17:45:10 PST
Created attachment 578878 [details] [diff] [review]
My first patch
Comment 2 :Margaret Leibovic 2011-12-04 22:35:17 PST
Comment on attachment 578878 [details] [diff] [review]
My first patch

Awesome! Thanks, Thomas!

A quick drive-by comment: It looks like this is the only place where we're using this throbber, so you can probably go ahead and remove the XUL element itself, as well as the CSS that styles it. I used a MXR search to find these references:

http://mxr.mozilla.org/mozilla-central/search?string=urlbar-throbber&find=%2Fbrowser%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Comment 3 Thomas Prip Vestergaard [:prip] 2011-12-06 14:11:53 PST
Created attachment 579452 [details] [diff] [review]
Remove spinner from AwesomeBar

Thanks Margaret!

Can we remove searching_16.png?

http://mxr.mozilla.org/mozilla-central/search?string=searching_16.png&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Comment 4 :Margaret Leibovic 2011-12-07 10:33:42 PST
(In reply to Thomas Prip Vestergaard from comment #3)

> Can we remove searching_16.png?
> 
> http://mxr.mozilla.org/mozilla-central/search?string=searching_16.
> png&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

It looks like that image is only used for this spinner, so yes, rip it out!
Comment 5 Thomas Prip Vestergaard [:prip] 2011-12-07 12:47:48 PST
Created attachment 579795 [details] [diff] [review]
Remove spinner from AwesomeBar
Comment 6 :Margaret Leibovic 2011-12-07 19:58:37 PST
Comment on attachment 579795 [details] [diff] [review]
Remove spinner from AwesomeBar

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> var LocationBarHelpers = {
>   _timeoutID: null,
> 
>   _searchBegin: function LocBar_searchBegin() {
>     function delayedBegin(self) {
>       self._timeoutID = null;
>-      document.getElementById("urlbar-throbber").setAttribute("busy", "true");
>-    }
>-
>+    }
>     this._timeoutID = setTimeout(delayedBegin, 500, this);
>   },
> 
>   _searchComplete: function LocBar_searchComplete() {
>     // Did we finish the search before delayedBegin was invoked?
>     if (this._timeoutID) {
>       clearTimeout(this._timeoutID);
>       this._timeoutID = null;
>     }
>-    document.getElementById("urlbar-throbber").removeAttribute("busy");
>   }
> };

Looking at this more closely, we can remove this entire object, since it's only purpose is to show/hide this throbber. Then you should also remove the lines of code that reference this object, which are just two lines in browser.xul: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#519. So much cleanup! :)

I'm just giving this f+ for now, but I'll give it an r+ if you upload a patch with those changes.
Comment 7 Alex Limi (:limi) — Firefox UX Team 2011-12-08 14:16:52 PST
CCing Jared & Dolske, since they're working on removing the favicon from the URL bar in bug 588270, and this is related.
Comment 8 Thomas Prip Vestergaard [:prip] 2011-12-08 15:08:04 PST
Created attachment 580207 [details] [diff] [review]
Bug 702926 - Remove spinner from AwesomeBar
Comment 9 :Margaret Leibovic 2011-12-08 15:21:38 PST
Comment on attachment 580207 [details] [diff] [review]
Bug 702926 - Remove spinner from AwesomeBar

Looks good to me!

I guess the page-proxy-stack doesn't need to exist as a stack anymore, but I don't think it's worth it to remove it, since that may introduce different bugs. Also, like Limi said, it's probably going away in bug 588270 anyway.
Comment 10 :Margaret Leibovic 2011-12-08 15:26:08 PST
(In reply to Margaret Leibovic [:margaret] from comment #9)
> I guess the page-proxy-stack doesn't need to exist as a stack anymore, but I
> don't think it's worth it to remove it, since that may introduce different
> bugs.

To clarify, I just think it may take non-trivial effort to keep things looking the same if we re-structure the XUL here, but I could be wrong.
Comment 11 :Margaret Leibovic 2011-12-08 15:47:45 PST
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd233b47a343

Prip, once this gets merged into mozilla-central it will get marked as RESOLVED FIXED.
Comment 12 Ed Morley [:emorley] 2011-12-09 06:47:53 PST
Congratulations on your first patch in the tree! Hope to see you on IRC in #developers soon (see https://wiki.mozilla.org/IRC#Getting_Started for details). 

If you'd like to fix another bug (it would be awesome if you did!) but need some inspiration, pop on & say hi - and we'll find something for you :-)

https://hg.mozilla.org/mozilla-central/rev/bd233b47a343

Note You need to log in before you can comment on or make changes to this bug.