702926 - Remove spinner from AwesomeBar

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

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

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]]

Attachment: My first patch

Comment 2

[:Margaret Leibovic]

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]]

Attachment: 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]

(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]]

Attachment: Remove spinner from AwesomeBar

Comment 6

[:Margaret Leibovic]

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]

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]]

Attachment: Bug 702926 - Remove spinner from AwesomeBar

Comment 9

[:Margaret Leibovic]

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]

(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]

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]]

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