1421837 - Confirm that the 'listitem-checkbox-iconic', 'listcell-checkbox-iconic' , 'listcell-iconic', 'listcell-checkbox', 'listitem-checkbox' bindings are unused and remove them

Status: RESOLVED FIXED

(Core :: XUL, task, P5)

Description

[Brian Grinstead [:bgrins]]

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).

Comment 1

[Brian Grinstead [:bgrins]]

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

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Correction: listcell-iconic is used by listitem-iconic so it cannot be removed.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1421837 - Remove list{item|cell}-checkbox-iconic binding.

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

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8947863 [details]
Bug 1421837 - Remove list{item|cell}-checkbox-iconic binding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217536/diff/1-2/

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8947863 [details]
Bug 1421837 - Remove list{item|cell}-checkbox-iconic binding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217536/diff/2-3/

Comment 9

[Code Review Bot [:reviewbot]]

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

[Code Review Bot [:reviewbot]]

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 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8947863 [details]
Bug 1421837 - Remove list{item|cell}-checkbox-iconic binding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217536/diff/3-4/

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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?

Comment 13

[Brian Grinstead [:bgrins]]

(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.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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).

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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 ?

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8947863 [details]
Bug 1421837 - Remove list{item|cell}-checkbox-iconic binding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217536/diff/4-5/

Comment 18

[Brian Grinstead [:bgrins]]

(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

[:Paolo Amadini]

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 :-)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2dae8d427e39
Remove list{item|cell}-checkbox-iconic binding. r=Paolo

Comment 21

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/2dae8d427e39

Comment 22

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336