Closed
Bug 1321556
Opened 8 years ago
Closed 8 years ago
Remove unused bindings from urlbarbindings.xml
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: mak, Assigned: mak)
Details
(Keywords: addon-compat)
Attachments
(1 file)
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?
Flags: needinfo?(dao+bmo)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment 1•8 years ago
|
||
(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.
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 2•8 years ago
|
||
OK, I will check add-ons then. thank you.
Assignee | ||
Comment 3•8 years ago
|
||
> (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
Assignee | ||
Updated•8 years ago
|
Keywords: addon-compat
Assignee | ||
Comment 4•8 years ago
|
||
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?
Flags: needinfo?(dao+bmo)
Comment 5•8 years ago
|
||
(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.
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request) |
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8818666 [details] Bug 1321556 - Remove unused bindings from urlbarbindings.xml. https://reviewboard.mozilla.org/r/98652/#review99034
Attachment #8818666 -
Flags: review?(dao+bmo) → review+
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/c490f8faff5b Remove unused bindings from urlbarbindings.xml. r=dao
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c490f8faff5b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in
before you can comment on or make changes to this bug.
Description
•