400809 - remove unneeded/unused parts from the urlbar binding

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 285856 [details] [diff] [review]
patch

>Index: toolkit/content/widgets/autocomplete.xml

>   <binding id="autocomplete"
>            extends="chrome://global/content/bindings/textbox.xml#textbox">

>     <content sizetopopup="pref">
>       <xul:hbox class="autocomplete-textbox-container" flex="1">
>-        <children includes="image|deck|stack">
>+        <children includes="image|deck|stack|box">

What's this change about? Unintentional?

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> (From update of attachment 285856 [details] [diff] [review])
> >Index: toolkit/content/widgets/autocomplete.xml
> 
> >   <binding id="autocomplete"
> >            extends="chrome://global/content/bindings/textbox.xml#textbox">
> 
> >     <content sizetopopup="pref">
> >       <xul:hbox class="autocomplete-textbox-container" flex="1">
> >-        <children includes="image|deck|stack">
> >+        <children includes="image|deck|stack|box">
> 
> What's this change about? Unintentional?

That's for the identity box from bug 383183.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Oh, right... That's suboptimal, I was rather wanting to avoid having to change a toolkit binding for the identity handler specifically, but I guess there isn't much of an alternative. Does duplicating the autocomplete binding's <content> and making that single change cause a perf hit?

Comment 4

[Dão Gottwald [:dao]]

I wouldn't expect any perf hit, the simple fact that we would be duplicating the entire thing just to change one value seemed suboptimal to me. I don't really like adding "box" there either, but then most of the remaining "includes" rules aren't very intuitive.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> I wouldn't expect any perf hit, the simple fact that we would be duplicating
> the entire thing just to change one value seemed suboptimal to me.

Yeah, but the alternative is a change to a global binding that might affect other people that use it and put <box>es in it. Maybe I'm worrying too much.

Comment 6

[AndrewM]

To this inexperienced user, from looking at the patch, it looks like it removes the browser.urlbar.hideProtocols and browser.urlbar.animateBlend prefs. I currently have hideProtocols empty and animateBlend set to false because I don't find much benefit from hiding the protocol and the location bar flickering was getting annoying (bug 396084).

I suspect I am not entirely alone in being happy (especially among more advanced Firefox users) with the way the location bar behaves without the fading and the hiding of the http and https bits, although many users I'm sure will find that useful.

Is it possible to retain a way for those who don't find these features useful to disable them?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

It removes the prefs because it's removing the feature.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 285856 [details] [diff] [review]
patch

a=endgame drivers for M9 checkin.  Due to the likely perf impact of this patch, please land this on a quiet tree.

Comment 9

[Asaf Romano (gone)]

[03:25]	<Mano> gavin: btw, why is xmlns:html still there? :p

Comment 10

[Reed Loden [:reed]]

Attachment: unbitrotten patch with nit addressed

Comment 11

[Reed Loden [:reed]]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.214; previous revision: 1.213
done
Checking in browser/base/content/urlbarBindings.xml;
/cvsroot/mozilla/browser/base/content/urlbarBindings.xml,v  <--  urlbarBindings.xml
new revision: 1.38; previous revision: 1.37
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.89; previous revision: 1.88
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.109; previous revision: 1.108
done
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.85; previous revision: 1.84
done

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified via a combination of both Bonsai and UI/functionality inspection; we no longer do neither the protocol hiding nor the fade/hover effects on the following platforms:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008020604 Minefield/3.0b4pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008020604 Minefield/3.0b4pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020604 Minefield/3.0b4pre