1214077 - Remove FRAMEARENA_HELPERS from nsRubyContentFrame

Status: RESOLVED DUPLICATE of bug 1216332

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Only its subclasses nsRubyBaseFrame/nsRubyTextFrame should be instantiated.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

Thanks! Sorry for not catching this in review on bug 1099807.

Comment 3

[Daniel Holbert [:dholbert]]

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?

(See also bug 1216332.)

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I tend to think we should only remove them if there's something that enforces that the frame class cannot be instantiated.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(And, ideally, we should enforce that all frame classes either have the macros, or have something that prevents them from being instantiated.)

Comment 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/668563c7fab88c1fdcef3aad6ff2a42f13196652
Bug 1214077 - Remove FRAMEARENA_HELPERS from nsRubyContentFrame. r=dholbert

Comment 9

[Daniel Holbert [: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).

Comment 10

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

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

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

I'm fine with this being backed out. Let's wait for bug 1216332 first.

Comment 12

[Daniel Holbert [:dholbert]]

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