Closed Bug 702926 Opened 12 years ago Closed 12 years ago

Remove spinner from AwesomeBar


(Firefox :: Address Bar, defect)

Not set



Firefox 11


(Reporter: limi, Assigned: prip)



(Whiteboard: [good first bug][Snappy:P1])


(1 file, 3 obsolete files)

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.
Whiteboard: [good first bug]
Attached patch My first patch (obsolete) — Splinter Review
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:^[^\0]*%24&hitlimit=&tree=mozilla-central
(In reply to Thomas Prip Vestergaard from comment #3)

> Can we remove searching_16.png?
> 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!
Attached patch Remove spinner from AwesomeBar (obsolete) — Splinter Review
Attachment #579452 - Attachment is obsolete: true
Attachment #579795 - Flags: review?(margaret.leibovic)
Assignee: nobody → thomas
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: 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.
Attachment #579795 - Flags: review?(margaret.leibovic) → feedback+
Whiteboard: [good first bug] → [good first bug][Snappy:P1]
CCing Jared & Dolske, since they're working on removing the favicon from the URL bar in bug 588270, and this is related.
Attachment #579795 - Attachment is obsolete: true
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.
Attachment #580207 - Flags: review+
(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.
Pushed to inbound:

Prip, once this gets merged into mozilla-central it will get marked as RESOLVED FIXED.
Congratulations on your first patch in the tree! Hope to see you on IRC in #developers soon (see 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 :-)
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
Depends on: 713828
You need to log in before you can comment on or make changes to this bug.