Closed Opened 5 years ago Closed 5 years ago

# Remove FRAMEARENA_HELPERS from nsRubyContentFrame

Not set
normal

## Tracking

### ()

RESOLVED DUPLICATE of bug 1216332
Tracking Status
firefox44 --- affected

## Attachments

### (1 file)

 1.93 KB, patch dholbert : review+ Details | Diff | Splinter Review
`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:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSplittableFrame.cpp?rev=2c434107469b#17
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp?rev=e8c7dfe727cd#45

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

```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:
http://hg.mozilla.org/mozilla-central/rev/7df4c375164f#l87.12
http://hg.mozilla.org/mozilla-central/rev/7df4c375164f#l43.2
(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
```https://hg.mozilla.org/integration/mozilla-inbound/rev/668563c7fab88c1fdcef3aad6ff2a42f13196652
Bug 1214077 - Remove FRAMEARENA_HELPERS from nsRubyContentFrame. r=dholbert```
```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).```
```Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/19c3ba35bcae for build bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=15896673&repo=mozilla-inbound```
Flags: needinfo?(quanxunzhen)
`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.)`
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1216332
You need to log in before you can comment on or make changes to this bug.