The default bug view has changed. See this FAQ.

Remove spinner from AwesomeBar

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Location Bar
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: limi, Assigned: prip)

Tracking

Trunk
Firefox 11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

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.

Updated

5 years ago
Whiteboard: [good first bug]
(Assignee)

Comment 1

5 years ago
Created attachment 578878 [details] [diff] [review]
My first patch

Comment 2

5 years ago
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
(Assignee)

Comment 3

5 years ago
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
Attachment #578878 - Attachment is obsolete: true

Comment 4

5 years ago
(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!
(Assignee)

Comment 5

5 years ago
Created attachment 579795 [details] [diff] [review]
Remove spinner from AwesomeBar
Attachment #579452 - Attachment is obsolete: true

Updated

5 years ago
Attachment #579795 - Flags: review?(margaret.leibovic)

Updated

5 years ago
Assignee: nobody → thomas
Status: NEW → ASSIGNED

Comment 6

5 years ago
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.
Attachment #579795 - Flags: review?(margaret.leibovic) → feedback+

Updated

5 years ago
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.
(Assignee)

Comment 8

5 years ago
Created attachment 580207 [details] [diff] [review]
Bug 702926 - Remove spinner from AwesomeBar
Attachment #579795 - Attachment is obsolete: true

Comment 9

5 years ago
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+

Comment 10

5 years ago
(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

5 years ago
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.
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11

Updated

5 years ago
Depends on: 713828
You need to log in before you can comment on or make changes to this bug.