Closed Bug 1398520 Opened 2 years ago Closed 2 years ago

Remove nsGfxCheckboxControlFrame and nsGfxRadioControlFrame

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: mats, Assigned: mats)

References

Details

Attachments

(5 files)

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
Flags: needinfo?(mats)
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.
Priority: -- → P3
Removing a couple of files will change the build chunking,
so I'm fixing this problem upfront.
Flags: needinfo?(mats)
Attachment #8911608 - Flags: review?(dholbert)
(I'll rename nsFormControlFrame to something more appropriate in a later patch.)
Attachment #8911609 - Flags: review?(dholbert)
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.
Attachment #8911611 - Flags: review?(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
Attachment #8911608 - Flags: review?(dholbert) → review+
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
Attachment #8911609 - Flags: review?(dholbert) → review+
Comment on attachment 8911610 [details] [diff] [review]
part 3 - Remove a few unused function declarations.

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

r=me
Attachment #8911610 - Flags: review?(dholbert) → review+
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.
Attachment #8911611 - Flags: review?(dholbert) → review-
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)
Attachment #8911611 - Flags: review- → review+
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*.)
Attachment #8911612 - Flags: review?(dholbert) → review+
Pushed by mpalmgren@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.