Closed
Bug 1421837
Opened 7 years ago
Closed 7 years ago
Confirm that the 'listitem-checkbox-iconic', 'listcell-checkbox-iconic' , 'listcell-iconic', 'listcell-checkbox', 'listitem-checkbox' bindings are unused and remove them
Categories
(Core :: XUL, task, P5)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: bgrins, Assigned: timdream)
References
Details
(Whiteboard: [xbl-available])
Attachments
(1 file)
This is a binding attached to 'listitem[type="checkbox"].listitem-iconic' - https://dxr.mozilla.org/mozilla-central/search?q=listitem-checkbox-iconic&redirect=true The only elements that have the listitem-iconic class seem to be direct results of the 'appendItem' call (which doesn't set type="checkbox"): - https://dxr.mozilla.org/mozilla-central/search?q=listitem-iconic - https://dxr.mozilla.org/mozilla-central/rev/c2248f85346939d3e0b01f57276c440ccb2d16a1/toolkit/content/widgets/listbox.xml#717 I believe this is also the only consumer of listcell[type="checkbox"].listcell-iconic which is bound to "listcell-checkbox-iconic" bindings and associated CSS in listbox.css (.listcell-icon).
Reporter | ||
Comment 1•7 years ago
|
||
I'm pretty sure these aren't used.. I did a try push where I print every call to `nsXBLBinding::SetBoundElement` and neither appear in the output: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaccf2c1ff6b66b52bcfa1390b78bb781b86f672&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=148874008
Updated•7 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → wontfix
status-firefox59:
--- → fix-optional
Priority: -- → P5
Reporter | ||
Updated•7 years ago
|
Summary: Confirm that the 'listitem-checkbox-iconic' and 'listcell-checkbox-iconic' bindings are unused and remove them → Confirm that the 'listitem-checkbox-iconic', 'listcell-checkbox-iconic' , 'listcell-iconic', 'listcell-checkbox', 'listitem-checkbox' bindings are unused and remove them
Assignee | ||
Comment 3•7 years ago
|
||
I can see that 'listitem-iconic' is being used, but not the ones on the summary: * listitem-checkbox-iconic (inc. listitem-checkbox) * listcell-checkbox * listcell-checkbox-iconic (inc. listcell-iconic) I can produce a patch to remove the code, if I am not being pull away from other priorities.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
Correction: listcell-iconic is used by listitem-iconic so it cannot be removed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8947863 [details] Bug 1421837 - Remove list{item|cell}-checkbox-iconic binding. https://reviewboard.mozilla.org/r/217536/#review223314 ::: toolkit/themes/osx/global/jar.mn:58 (Diff revision 2) > skin/classic/global/arrow/arrow-up-dis.gif (arrow/arrow-up-dis.gif) > skin/classic/global/arrow/arrow-up-sharp.gif (arrow/arrow-up-sharp.gif) > skin/classic/global/arrow/arrow-up.gif (arrow/arrow-up.gif) > skin/classic/global/arrow/panelarrow-horizontal.svg (arrow/panelarrow-horizontal.svg) > skin/classic/global/arrow/panelarrow-vertical.svg (arrow/panelarrow-vertical.svg) > skin/classic/global/checkbox/cbox-check.gif (checkbox/cbox-check.gif) This file is not removed because it still used by a test. https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/accessible/tests/mochitest/treeview.css#2 I am not sure about if it's safe to remove.
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8947863 [details] Bug 1421837 - Remove list{item|cell}-checkbox-iconic binding. https://reviewboard.mozilla.org/r/217536/#review223316 Static analysis found 1 defect in this patch. - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint check path/to/file` (Python/Javascript/wpt) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/profile/content/profileSelection.js:41 (Diff revision 1) > var profile = profileList.getNext().QueryInterface(I.nsIToolkitProfile); > > var listitem = profilesElement.appendItem(profile.name, ""); > > var tooltiptext = > - gProfileManagerBundle.getFormattedString("profileTooltip", [profile.name, profile.rootDir.path]); > + gProfileManagerBundle.getFormattedString("profileTooltip", ['XXXXX' + profile.name, profile.rootDir.path]); Error: Strings must use doublequote. [eslint: quotes]
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8947863 [details] Bug 1421837 - Remove list{item|cell}-checkbox-iconic binding. https://reviewboard.mozilla.org/r/217536/#review223320 Static analysis found 1 defect in this patch. - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint check path/to/file` (Python/Javascript/wpt) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/profile/content/profileSelection.js:41 (Diff revision 2) > var profile = profileList.getNext().QueryInterface(I.nsIToolkitProfile); > > var listitem = profilesElement.appendItem(profile.name, ""); > > var tooltiptext = > - gProfileManagerBundle.getFormattedString("profileTooltip", [profile.name, profile.rootDir.path]); > + gProfileManagerBundle.getFormattedString("profileTooltip", ['XXXXX' + profile.name, profile.rootDir.path]); Error: Strings must use doublequote. [eslint: quotes]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8947863 [details] Bug 1421837 - Remove list{item|cell}-checkbox-iconic binding. https://reviewboard.mozilla.org/r/217536/#review223344 ::: toolkit/content/widgets/listbox.xml:1075 (Diff revision 4) > > <binding id="listitem-iconic" > extends="chrome://global/content/bindings/listbox.xml#listitem"> > <content> > <children> > - <xul:listcell class="listcell-iconic" xbl:inherits="label,image,crop,disabled,flexlabel"/> > + <xul:listcell xbl:inherits="disabled"> The `disabled` property is needed by `general.xml#basecontrol` binding (extended by `listcell` binding, see below). ::: toolkit/content/widgets/listbox.xml:1083 (Diff revision 4) > - <xul:listcell type="checkbox" class="listcell-iconic" xbl:inherits="label,image,crop,checked,disabled,flexlabel"/> > </children> > </content> > </binding> > > <binding id="listcell" role="xul:listcell" With this patch, the only consumers of this binding are the `listitem` and the `listitem-iconic` bindings. I don't know if that make this binding removable in this bug? Also, is it possible to inherit `basecontrol` without this binding?
Reporter | ||
Updated•7 years ago
|
Attachment #8947863 -
Flags: review?(bgrinstead) → review?(paolo.mozmail)
Reporter | ||
Comment 13•7 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #7) > Comment on attachment 8947863 [details] > Bug 1421837 - Remove listitem-checkbox{-iconic}, listcell-checkbox{-iconic} > binding. > > https://reviewboard.mozilla.org/r/217536/#review223314 > > ::: toolkit/themes/osx/global/jar.mn:58 > (Diff revision 2) > > skin/classic/global/arrow/arrow-up-dis.gif (arrow/arrow-up-dis.gif) > > skin/classic/global/arrow/arrow-up-sharp.gif (arrow/arrow-up-sharp.gif) > > skin/classic/global/arrow/arrow-up.gif (arrow/arrow-up.gif) > > skin/classic/global/arrow/panelarrow-horizontal.svg (arrow/panelarrow-horizontal.svg) > > skin/classic/global/arrow/panelarrow-vertical.svg (arrow/panelarrow-vertical.svg) > > skin/classic/global/checkbox/cbox-check.gif (checkbox/cbox-check.gif) > > This file is not removed because it still used by a test. > > https://searchfox.org/mozilla-central/rev/ > eeb7190f9ad6f1a846cd6df09986325b3f2c3117/accessible/tests/mochitest/treeview. > css#2 > > I am not sure about if it's safe to remove. I assume the CSS rule in the test could also be removed (or the image changed to something else) but at the least we should remove cbox-check.gif from the jar.mn and into support-files for the test so we aren't shipping it with the application.
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #13) > (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment > > > skin/classic/global/checkbox/cbox-check.gif (checkbox/cbox-check.gif) > > > > This file is not removed because it still used by a test. > > > > https://searchfox.org/mozilla-central/rev/ > > eeb7190f9ad6f1a846cd6df09986325b3f2c3117/accessible/tests/mochitest/treeview. > > css#2 > > > > I am not sure about if it's safe to remove. > > I assume the CSS rule in the test could also be removed (or the image > changed to something else) but at the least we should remove cbox-check.gif > from the jar.mn and into support-files for the test so we aren't shipping it > with the application. Would that break the mapping from chrome://global/skin/checkbox/cbox-check.gif from the actual file in the package? Also, moving to support-files means it will apply to all platforms, not just osx. I would prefer keeping the chrome url (assuming we can still unship it).
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #14) > Would that break the mapping from > chrome://global/skin/checkbox/cbox-check.gif from the actual file in the > package? The answer to this question is yes. > Also, moving to support-files means it will apply to all platforms, not just > osx. I would prefer keeping the chrome url (assuming we can still unship it). %ifdef XP_MACOSX ?
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #15) I will remove treeview.css altogether because none of the files referenced exists (except the one I am removing). (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #3) > * listitem-checkbox-iconic (inc. listitem-checkbox) I was wrong about listitem-checkbox; it is being used at least Places edit tags UI and clear recent history, given the Try errors. :'(
Comment hidden (mozreview-request) |
Reporter | ||
Comment 18•7 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #15) > (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment > #14) > > Would that break the mapping from > > chrome://global/skin/checkbox/cbox-check.gif from the actual file in the > > package? > > The answer to this question is yes. > > > Also, moving to support-files means it will apply to all platforms, not just > > osx. I would prefer keeping the chrome url (assuming we can still unship it). > > %ifdef XP_MACOSX ? OK -- originally I thought the only remaining consumer of the file was a test. But if other places are accessing the image as a chrome URI then we should keep it.
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8947863 [details] Bug 1421837 - Remove list{item|cell}-checkbox-iconic binding. https://reviewboard.mozilla.org/r/217536/#review224216 Thanks! Looks like a simpler patch than when I first took a look :-)
Attachment #8947863 -
Flags: review?(paolo.mozmail) → review+
Comment 20•7 years ago
|
||
Pushed by timdream@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2dae8d427e39 Remove list{item|cell}-checkbox-iconic binding. r=Paolo
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2dae8d427e39
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 22•6 years ago
|
||
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
Updated•6 years ago
|
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•