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)

Product:
Core
Component:
XUL
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: bgrins, Assigned: timdream)

References

Details

(Whiteboard: [xbl-available])

Attachments

(1 file)

Bug 1421837 - Remove list{item|cell}-checkbox-iconic binding.
59 bytes, text/x-review-board-request
Paolo
: review+
Details 
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).
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
status-firefox57: --- → wontfix
status-firefox58: --- → wontfix
status-firefox59: --- → fix-optional
Priority: -- → P5
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
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
Correction: listcell-iconic is used by listitem-iconic so it cannot be removed.
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 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 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 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?
Attachment #8947863 - Flags: review?(bgrinstead) → review?(paolo.mozmail)
(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.
(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).
(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 ?
(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. :'(
(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 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+
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2dae8d427e39
Remove list{item|cell}-checkbox-iconic binding. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/2dae8d427e39
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: