Closed Bug 1214077 Opened 7 years ago Closed 7 years ago

Remove FRAMEARENA_HELPERS from nsRubyContentFrame


(Core :: Layout, defect)

Not set



Tracking Status
firefox44 --- affected


(Reporter: xidorn, Assigned: xidorn)




(1 file)

Only its subclasses nsRubyBaseFrame/nsRubyTextFrame should be instantiated.
Summary: Remove FRAMEARENA_HELPERS of nsRubyContentFrame → Remove FRAMEARENA_HELPERS from nsRubyContentFrame
Attached patch patchSplinter Review
Attachment #8674184 - Flags: review?(dholbert)
Thanks! Sorry for not catching this in review on bug 1099807.
Depends on: 1099807
Attachment #8674184 - Flags: review?(dholbert) → review+
Hmm, actually, looks like nsContainerFrame and nsSplittableFrame each have these macros, despite never being directly instantiated:

Can/should we remove them there, too?  Or is nsRubyContentFrame different somehow?

(See also bug 1216332.)
It looks like these classes (nsContainerFrame and nsSplittableFrame) were discussed as being never-instantiated in the bug that added FRAMEARENA_HELPERS -- see bug 497495 comment 16 & 17 -- and yet we still ended up using these macros for these classes:
(though we didn't use the macros for nsMathMLFrame, another superclass-only frame class that was mentioned there).

dbaron, do you know if we actually needed/need these macros in nsContainerFrame & nsSplittableFrame? Or was this just a mistake? (in which case we can drop FRAMEARENA_HELPERS macros from e.g. nsContainerFrame.* and nsSplittableFrame.*)
Flags: needinfo?(dbaron)
I tend to think we should only remove them if there's something that enforces that the frame class cannot be instantiated.
Flags: needinfo?(dbaron)
(And, ideally, we should enforce that all frame classes either have the macros, or have something that prevents them from being instantiated.)
That's fair. Bug 1216332 is filed on the enforcing, at least.  In the meantime, we probably shouldn't take this bug's patch (for nsRubyContentFrame) until we've got a way of doing the enforcement.  (And we should give nsRubyContentFrame the enforcement in the same patch where we remove the macros, atomically.)  Hence, marking this bug as depends-on bug 1216332.
Depends on: 1216332
Ah well, I suppose I r+'d too eagerly.  (I assume xidorn hadn't seen comment 7 when prepping to push.)

Feel free to back out per comment 7, but I'm also OK with this staying in, given that we can pretty easily manually sanity-check that nsRubyContentFrame isn't instantiated in MXR (and I did when reviewing).
I'm fine with this being backed out. Let's wait for bug 1216332 first.
Flags: needinfo?(quanxunzhen)
(looks like bug 1216332 may end up fixing this -- its current patch swaps out this class's FRAMEARENA macro for an ABSTRACT alternative, at least. If so, we can just dupe this over I suppose.)
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1216332
You need to log in before you can comment on or make changes to this bug.