1398520 - Remove nsGfxCheckboxControlFrame and nsGfxRadioControlFrame

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, enhancement, P3)

Description

[Mats Palmgren (inactive)]

Now that the Android-specific stuff has been removed from these
classes (bug 1352238) the only remaining method is AccessibleType().
However, bug 1349835 changed that on the a11y side to not use
the frame, so those methods are dead code.

So I think we should remove these two frame classes and use
the (badly named) nsFormControlFrame base class directly instead,
(and rename it to nsCheckboxRadioFrame or something since it's
not used for anything else).

I have patches, but I'll wait until we branch v57 on Sept 21.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=672928eda6c046f67d490f69aea34be55762169b

Comment 1

[Marco Zehe (:MarcoZ)]

No objections from the a11y side. Test passed, and the one failure I saw in that try run was a known intermittent. Also bug 1349835 has been on the trains for a while and didn't show any regressions.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - Add missing #include for NS_THEME_* constants.

Removing a couple of files will change the build chunking,
so I'm fixing this problem upfront.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2 - Remove nsGfxCheckboxControlFrame and nsGfxRadioControlFrame; use nsFormControlFrame instead.

(I'll rename nsFormControlFrame to something more appropriate in a later patch.)

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3 - Remove a few unused function declarations.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 4a - Rename nsFormControlFrame to nsCheckboxRadioFrame (automated change).

This is a tree-wide change of s/nsFormControlFrame/nsCheckboxRadioFrame/.
I'll fix a few things manually in the next part (and fold that before landing).
I figured it'd be easier to review the automated/manual parts separately.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 4b - Rename nsFormControlFrame to nsCheckboxRadioFrame (a few manual fixes).

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8911608 [details] [diff] [review]
part 1 - Add missing #include for NS_THEME_* constants.

Review of attachment 8911608 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8911609 [details] [diff] [review]
part 2 - Remove nsGfxCheckboxControlFrame and nsGfxRadioControlFrame; use nsFormControlFrame instead.

Review of attachment 8911609 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8911610 [details] [diff] [review]
part 3 - Remove a few unused function declarations.

Review of attachment 8911610 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8911611 [details] [diff] [review]
part 4a - Rename nsFormControlFrame to nsCheckboxRadioFrame (automated change).

Review of attachment 8911611 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mats Palmgren (:mats) from comment #5)
> This is a tree-wide change of s/nsFormControlFrame/nsCheckboxRadioFrame/.
> I'll fix a few things manually in the next part (and fold that before
> landing).
> I figured it'd be easier to review the automated/manual parts separately.

Thanks! Yeah, that was easier.  Do remember to fold before landing -- I suspect 4a won't build successfully on its own, since moz.build files are strict about alphabetical ordering.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8911611 [details] [diff] [review]
part 4a - Rename nsFormControlFrame to nsCheckboxRadioFrame (automated change).

Review of attachment 8911611 [details] [diff] [review]:
-----------------------------------------------------------------

(sorry, meant to r+, but hit the "-" by accident)

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8911612 [details] [diff] [review]
part 4b - Rename nsFormControlFrame to nsCheckboxRadioFrame (a few manual fixes).

Review of attachment 8911612 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on 4b (folded) with one indentation nit addressed:

::: layout/forms/nsCheckboxRadioFrame.cpp
@@ +114,5 @@
>  }
>  
>  void
> +nsCheckboxRadioFrame::Reflow(nsPresContext*   aPresContext,
> +                           ReflowOutput&      aDesiredSize,

Indentation is still off here. (This patch fixes indentation of the arg *names*, but not the indentation of their *types*.)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/191af237952d
part 1 - Add missing #include for NS_THEME_* constants.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/b47d2dbb3dcd
part 2 - Remove nsGfxCheckboxControlFrame and nsGfxRadioControlFrame; use nsFormControlFrame instead.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8bca0c06b6
part 3 - Remove a few unused function declarations.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/051041ba94d6
part 4 - Rename nsFormControlFrame to nsCheckboxRadioFrame.  r=dholbert

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/191af237952d
https://hg.mozilla.org/mozilla-central/rev/b47d2dbb3dcd
https://hg.mozilla.org/mozilla-central/rev/2a8bca0c06b6
https://hg.mozilla.org/mozilla-central/rev/051041ba94d6