1321556 - Remove unused bindings from urlbarbindings.xml

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

After bug 1310942, I was having a look at what's left in urlbarbindings.xml.
Most of the stuff looks urlbar related finally, but 3 things:

1. <binding id="splitmenu">
http://searchfox.org/mozilla-central/search?q=splitmenu&path=
I cannot find a rule associating this to anything... was this the old splitmenu binding we used in the old Firefox menu? If so, I assume we can safely remove it.

2. <binding id="menuitem-tooltip"
http://searchfox.org/mozilla-central/search?q=menuitem-tooltip&path=
It looks associated to a menuitem-tooltip class that I don't see used anywhere.

3. <binding id="menuitem-iconic-tooltip"
http://searchfox.org/mozilla-central/search?q=menuitem-iconic-tooltip&path=
again associated with menuitem-iconic-tooltip and menuitem-tooltip that look unused.

Dao, am I missing something, or can I safely make a patch to remove these?

Comment 1

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [::mak] from comment #0)
> After bug 1310942, I was having a look at what's left in urlbarbindings.xml.
> Most of the stuff looks urlbar related finally, but 3 things:
> 
> 1. <binding id="splitmenu">
> http://searchfox.org/mozilla-central/search?q=splitmenu&path=
> I cannot find a rule associating this to anything... was this the old
> splitmenu binding we used in the old Firefox menu? If so, I assume we can
> safely remove it.

We kept this because add-ons were using it. It's possible that the number of affected add-ons is now small enough that we can remove this.

> 2. <binding id="menuitem-tooltip"
> http://searchfox.org/mozilla-central/search?q=menuitem-tooltip&path=
> It looks associated to a menuitem-tooltip class that I don't see used
> anywhere.
> 
> 3. <binding id="menuitem-iconic-tooltip"
> http://searchfox.org/mozilla-central/search?q=menuitem-iconic-tooltip&path=
> again associated with menuitem-iconic-tooltip and menuitem-tooltip that look
> unused.

Probably the same as 1.

Comment 2

[Marco Bonardo [:mak]]

OK, I will check add-ons then. thank you.

Comment 3

[Marco Bonardo [:mak]]

> (In reply to Marco Bonardo [::mak] from comment #0)
> > 1. <binding id="splitmenu">
> > http://searchfox.org/mozilla-central/search?q=splitmenu&path=
> > I cannot find a rule associating this to anything... was this the old
> > splitmenu binding we used in the old Firefox menu? If so, I assume we can
> > safely remove it.

I could find only 2 add-ons that are not completely outdated or removed:
https://addons.mozilla.org/firefox/addon/best-search-ultimate/ (62 users)
https://addons.mozilla.org/firefox/addon/menu-icons-plus/ (8500 users)

For the latter the only reason to use the binding is to add icons to the no-more existing menu. That means it would be trivial to fix it, just by removing the binding.
Unfortunately the add-on looks "abandoned" since the last update is older than 2 years ago, that means if we break it, it's likely to stay broken :(

> > 2. <binding id="menuitem-tooltip"
> > http://searchfox.org/mozilla-central/search?q=menuitem-tooltip&path=
> > It looks associated to a menuitem-tooltip class that I don't see used
> > anywhere.

Same as above (menu-icons-plus), plus:
https://addons.mozilla.org/it/firefox/addon/private-tab/ (64k users)
This wouldn't break (further) though, it uses those 2 classes to somehow detect tooltips and print hotkeys... but since those classes are unused in current Firefox, it's a no-op. It won't break and we can ignore this usage.
It looks safe to remove.

> > 3. <binding id="menuitem-iconic-tooltip"
> > http://searchfox.org/mozilla-central/search?q=menuitem-iconic-tooltip&path=
> > again associated with menuitem-iconic-tooltip and menuitem-tooltip that look
> > unused.

This one is used more broadly by a bunch of add-ons, so we probably can't remove it yet. But we can add a comment explaining why it's there.

To sum up:
1. splitmenu: decide what to do with menu icons plus. This add-on is abandoned and even if it sort-of works, it's likely quite outdated
2. menuitem-tooltip: can be removed
3. menuitem-iconic-tooltip: leave it with a comment

Comment 4

[Marco Bonardo [:mak]]

Looks like some change in the past removed this:
+splitmenu {
+  -moz-binding: url("chrome://browser/content/urlbarBindings.xml#splitmenu");
+}
from browser.css, that means splitmenu is just broken for a bunch of add-ons that are using it without redefining the binding.

We can either complete the removal (also remove it from nsGkAtomList.h, nsMenuPopupFrame.cpp and toolkit/themes), or move it to toolkit/content/widgets/menu.xml and reinsert the appropriate binding rule.
It is strange that this widget is spread across toolkit and browser like that.

What do you think?

Comment 5

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [::mak] from comment #4)
> Looks like some change in the past removed this:
> +splitmenu {
> +  -moz-binding:
> url("chrome://browser/content/urlbarBindings.xml#splitmenu");
> +}
> from browser.css, that means splitmenu is just broken for a bunch of add-ons
> that are using it without redefining the binding.

I did some research on this. It looks like bug 863753 attempted to remove the binding regardless of add-on compat, and landed on the UX branch before being merged to mozilla-central months later. My best guess is that the merge had conflicts that weren't resolved properly.

> We can either complete the removal (also remove it from nsGkAtomList.h,
> nsMenuPopupFrame.cpp and toolkit/themes), or move it to
> toolkit/content/widgets/menu.xml and reinsert the appropriate binding rule.

No, we shouldn't do that.

> It is strange that this widget is spread across toolkit and browser like
> that.

That's because we didn't want to duplicate the menu.css rules. Extending the selectors was just way easier.

Comment 6

[Marco Bonardo [:mak]]

Attachment: Bug 1321556 - Remove unused bindings from urlbarbindings.xml.

Review commit: https://reviewboard.mozilla.org/r/98652/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98652/

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8818666 [details]
Bug 1321556 - Remove unused bindings from urlbarbindings.xml.

https://reviewboard.mozilla.org/r/98652/#review99034

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c490f8faff5b
Remove unused bindings from urlbarbindings.xml. r=dao

Comment 9

[Phil Ringnalda (:philor)]

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