563659 - Make nsDocumentFragment not inherit from nsGenericElement

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Currently we can't rely on all things inheriting from nsGenericElement being an element, so we have to have custom unwrap helpers for quickstubs for example. Boris also mentioned that he's not convinced that nsDocumentFragment should be an nsIContent (and I think I agree).

Looking at the methods that we forward to nsGenericElement, most of them end up forwarding to or calling nsINode methods, so those should be simple to convert. I think the harder part of this is going to be looking for all the places that take nsIContent arguments, and have them deal with DocumentFragments too. This might depends on a better strategy for splitting nodes that can have children from nodes that can't.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1, v1 -- Override GetNameSpaceElement in nsDocumentFragment

So while poking at this, one thing I noticed was that nsDocumentFragment didn't override nsGenericElement's implementation of GetNameSpaceElement, which was "return this;".  This seems kind of wrong, to say the least, and is exactly the sort of problem that this bug causes!  Try: https://tbpl.mozilla.org/?tree=Try&rev=92d5c419c527

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Attempt at a patch, doesn't link

This compiles but doesn't link.  I'm getting the macro voodoo magic wrong somehow.  I don't know how much sense it makes, or how anyone is going to review it, but hey, it's an attempt.  Ms2ger has kindly volunteered to try fidgeting with it, since I've banged my head against it for a while.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Fwiw, you shouldn't need to move the Element flags to the other header.  None of them ever apply to document fragments.

Do fragments ever need nsDOMSlots?

What was your link error?

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Fwiw, you shouldn't need to move the Element flags to the other header. 
> None of them ever apply to document fragments.
> 
> Do fragments ever need nsDOMSlots?

I'm sure someone who knows more about how this code works would be able to split it up more intelligently!  My approach was basically to make skeleton files, try to compile, and move everything it complained about from nsGenericElement to ElementOrFragment.  I think I moved things like the element flags because there was some method in ElementOrFragment.cpp that used them and that I had to move to nsGenericElement.cpp.

I didn't want to try figuring out what actually applied only to elements -- I figured that stuff could be moved out later.  I was just aiming for something that built.

> What was your link error?

Unfortunately, I didn't save it.  It was related to some machinery like cycle collection or COM or whatnot, not anything I understand (yet).  I got to the point of a linker failure once or twice before that, though.  What would happen is that I would move some more code to fix the linker error, and then the compiler would notice more errors that it didn't before.  After repeating this for a couple of hours, I got bored.

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 633879 [details] [diff] [review]
Attempt at a patch, doesn't link

This patch would have been impossible to review.  I have a better approach worked out:

Patch 1: Copy nsGenericElement.{h,cpp} to FragmentOrElement.{h,cpp}.  Change FragmentOrElement.{h,cpp} by renaming everything from nsGenericElement to FragmentOrElement.  Change nsGenericElement.{h,cpp} by making it inherit from FragmentOrElement instead of Element, and #ifdef out basically everything.  (Using #ifdef instead of just deleting it allows subsequent patches to move things from FragmentOrElement to nsGenericElement without messing up blames, and in an easily reviewable fashion.)

Patch 2: Make Element inherit from FragmentOrElement instead of the reverse.  Change nsGenericElement to inherit from Element instead of FragmentOrElement.

Patch 3: Make nsDocumentFragment inherit from FragmentOrElement instead of nsGenericElement.  This will probably require moving lots of stuff from nsGenericElement to FragmentOrElement, but that's okay, because patch 1 was designed to make this easy to review.

This should preserve blame, and be reviewable.  I already have patch 1 compiling.  Don't know how easy patches 2 or 3 will be, but I'm hopeful.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That plan sounds like it will force everyone including Element to include nsGenericElement, which is likely to lead to include hell: nsGenericElement pulls in all sorts of stuff....  Just as a warning.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Hmm, noted.  That seems more or less inevitable if we're going to have the hierarchy we want, unless FragmentOrElement doesn't inherit from nsIContent but rather is its own class off to the side.  Element is going to have to be below it.  Well, I'll see -- if stuff blows up it will be at step 2, which is what I'm up to.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

So now I'm getting errors like this:

In file included from ../../../../../dist/include/nsAString.h:11:0,
                 from ../../../../../dist/include/nsSubstring.h:11,
                 from ../../../../../dist/include/nsString.h:13,
                 from ../../../../../dist/include/nsAttrValue.h:15,
                 from ../../../../../dist/include/nsAttrAndChildArray.h:18,
                 from ../../../../../dist/include/mozilla/dom/FragmentOrElement.h:22,
                 from ../../../../../dist/include/mozilla/dom/Element.h:10,
                 from ../../../../../dist/include/mozilla/dom/Link.h:14,
                 from /mnt/extra/checkouts/mozilla-central/toolkit/components/places/tests/cpp/mock_Link.h:14,
                 from /mnt/extra/checkouts/mozilla-central/toolkit/components/places/tests/cpp/test_IHistory.cpp:12:
../../../../../dist/include/nsStringFwd.h:16:2: error: #error Internal string headers are not available from external-linkage code.
In file included from ../../../../../dist/include/nsAString.h:35:0,
                 from ../../../../../dist/include/nsSubstring.h:11,
                 from ../../../../../dist/include/nsString.h:13,
                 from ../../../../../dist/include/nsAttrValue.h:15,
                 from ../../../../../dist/include/nsAttrAndChildArray.h:18,
                 from ../../../../../dist/include/mozilla/dom/FragmentOrElement.h:22,
                 from ../../../../../dist/include/mozilla/dom/Element.h:10,
                 from ../../../../../dist/include/mozilla/dom/Link.h:14,
                 from /mnt/extra/checkouts/mozilla-central/toolkit/components/places/tests/cpp/mock_Link.h:14,
                 from /mnt/extra/checkouts/mozilla-central/toolkit/components/places/tests/cpp/test_IHistory.cpp:12:
../../../../../dist/include/nsTSubstring.h:8:2: error: #error Cannot use internal string classes without MOZILLA_INTERNAL_API defined. Use the frozen header nsStringAPI.h instead.
In file included from ../../../../../dist/include/nsAString.h:40:0,
                 from ../../../../../dist/include/nsSubstring.h:11,
                 from ../../../../../dist/include/nsString.h:13,
                 from ../../../../../dist/include/nsAttrValue.h:15,
                 from ../../../../../dist/include/nsAttrAndChildArray.h:18,
                 from ../../../../../dist/include/mozilla/dom/FragmentOrElement.h:22,
                 from ../../../../../dist/include/mozilla/dom/Element.h:10,
                 from ../../../../../dist/include/mozilla/dom/Link.h:14,
                 from /mnt/extra/checkouts/mozilla-central/toolkit/components/places/tests/cpp/mock_Link.h:14,
                 from /mnt/extra/checkouts/mozilla-central/toolkit/components/places/tests/cpp/test_IHistory.cpp:12:
../../../../../dist/include/nsTSubstring.h:8:2: error: #error Cannot use internal string classes without MOZILLA_INTERNAL_API defined. Use the frozen header nsStringAPI.h instead.

Which seem to be telling me "don't include nsAttrAndChildArray.h in a file you want to export".  This seems to be what Boris was warning about.  What's a good solution to this?  Should I just scrap my existing patches and make FragmentOrElement not inherit from anything, so nsGenericElement inherits from both FragmentOrElement and Element directly?  The FragmentOrElement class is almost useless to have public anyway -- it's not like any public methods are actually going to return one.  Or is there a way to salvage this way of doing it?

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v1 -- Create FragmentOrElement and move all nsGenericElement functionality to it

This is basically just copy plus lots of s/nsGenericElement/FragmentOrElement/.  For sanity's sake, I wasn't picky about making sure all the whitespace lined up right.

This results in basically all the same stuff being in nsGenericElement.{h,cpp} and FragmentOrElement.{h,cpp}.  I #if 0 out all the stuff in nsGenericElement.  Subsequent patches will uncomment it and comment out the stuff in FragmentOrElement, so blame will be preserved.  In the final patch I'll kill all the #if 0 stuff.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3, v1 -- Make Element inherit from FragmentOrElement

This mostly involves moving Element methods from FragmentOrElement to nsGenericElement.  In the last patch FragmentOrElement still inherited from Element, so it was fine to implement its methods, but now it can't.  nsDocumentFragment still inherits from nsGenericElement, so it still gets all the Element methods anyway.

This results in Element trying to instantiate a bunch of stuff that's supposed to be internal, which causes things to complain.  This is the reason for the changes of headers from nsString.h to nsStringGlue.h, MOZILLA_INTERNAL_API usage, etc.  Actually, it's possible these parts of the patch should really be part of the last patch . . . I *think* I successfully compiled part 2 without part 3, but maybe I'm confused?

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 4, v1 -- Make nsDocumentFragment stop inheriting from nsGenericElement

This moves a couple of methods back to FragmentOrElement from nsGenericElement, because they're needed for nsIDOMNode or such.  If I wanted to be really tidy I'd have put those parts back in a previous patch, but I had enough arguing with the compiler as it is.

There are some nsIContent virtual methods I had to reimplement in nsDocumentFragment, like BindToTree.  I'm not totally sure what they do, so be sure to scrutinize them.  For List() and DumpContents(), I just made something up that looked good enough, by copy-pasting from nsGenericElement and then deleting element-specific stuff.

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 5, v1 -- Remove #if 0'd code

Self-explanatory.

Green try run for the series: https://tbpl.mozilla.org/?tree=Try&rev=072b8092f49c

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: New FragmentOrElement.h

As requested.  Tell me what to move back to nsGenericElement.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 639630 [details]
New FragmentOrElement.h

Sorry for the lag here....

These are almost certainly movable back to nsGenericElement:

  nsTouchEventReceiverTearoff
  nsInlineEventHandlersTearoff
  IsNodeOfType (since presumably you need different impls for fragment and
                element anyway, right?)
  MappedAttributeEntry
  FindAttributeDependence stuff
  AddScriptEventListener
  LeaveLink
  InternalIsSupported (maybe this should move to nsContentUtils?)
  ShouldBlur
  DispatchClickEvent
  DispatchEvent
  GetPrimaryFrame
  nsAttrInfo
  GetParsedAttr
  GetAttributeMap
  RecompileScriptEventHandlers
  NodeInfoChanged
  ParseCORSValue
  StringToCORSMode
  AttrValueToCORSMode
  The various named bools for SetAttrAndNotify
  BeforeSetAttr
  AfterSetAttr
  GetEventListenerManagerForAttr
  InternalGetExistingAttrNameFromQName
  CheckHandleEventForLinksPrecondition
  PreHandleEventForLinks
  PostHandleEventForLinks
  GetLinkTarget
  NS_IMPL_ELEMENT_CLONE
  NS_IMPL_ELEMENT_CLONE_WITH_INIT
  DOMCI_NODE_DATA (yes, this means some consumers will need to keep including
                   nsGenericElement.h)
  NS_IMPL_STRING_ATTR

And these are likely, but might not be quite as mechanical:
  PostQueryInterface stuff
  NS_ELEMENT_INTERFACE_MAP_END 
  NS_ELEMENT_INTERFACE_TABLE_TO_MAP_SEGUE

For GetClassAttributeName it's probably best to just move it from nsIContent to Element.

For GetExistingAttrNameFromQName, it might be nice to put the return null on nsIContent and then move it from here to nsGenericElement.

IsAttributeMapped should move from nsIContent to Element.

nsNodeWeakReference can move into nsFragmentOrElement.cpp, I think.

The comment describing the class should say it's a base class for elements and document fragments.

GetAttributeChangeHint should move from nsIContent to Element.

I wish we could move nsDOMSlots, but I guess we actually use mChildrenList from it?  That's pretty annoying.  :(

Do you want to modify the earlier patches, or just do a followup patch or two?

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #14)
> Do you want to modify the earlier patches, or just do a followup patch or
> two?

I'll do the changes in a new patch 5 that I insert before the current one.  After the current patch 5 of this series lands, moving functions back and forth will break hg blame.  Before it, moving things only consists of adding and removing #if 0's, so blame will remain intact.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Followup to fix <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsDOMQS.h#66>?

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #14)
> Comment on attachment 639630 [details]
> New FragmentOrElement.h
> 
> Sorry for the lag here....
> 
> These are almost certainly movable back to nsGenericElement:
> 
>   nsTouchEventReceiverTearoff
>   nsInlineEventHandlersTearoff

These are referenced in cycle-collection macros that I don't understand, so I don't know how to move them back.

>   IsNodeOfType (since presumably you need different impls for fragment and
>                 element anyway, right?)
>   MappedAttributeEntry
>   FindAttributeDependence stuff
>   AddScriptEventListener
>   LeaveLink

Moved.

>   InternalIsSupported (maybe this should move to nsContentUtils?)

This is called by IsSupported(), which is part of nsIDOMNode, so it looks like it needs to stay.

>   ShouldBlur
>   DispatchClickEvent
>   DispatchEvent
>   GetPrimaryFrame
>   nsAttrInfo
>   GetParsedAttr
>   GetAttributeMap
>   RecompileScriptEventHandlers
>   NodeInfoChanged
>   ParseCORSValue
>   StringToCORSMode
>   AttrValueToCORSMode
>   The various named bools for SetAttrAndNotify
>   BeforeSetAttr
>   AfterSetAttr
>   GetEventListenerManagerForAttr

Moved.

>   InternalGetExistingAttrNameFromQName

Moved, and added an implementation of GetExistingAttrNameFromQName for nsDocumentFragment that just does "return nsnull;".  (This kind of method should probably be moved to dom::Element anyway, but that's a separate story.)

>   CheckHandleEventForLinksPrecondition
>   PreHandleEventForLinks
>   PostHandleEventForLinks
>   GetLinkTarget
>   NS_IMPL_ELEMENT_CLONE
>   NS_IMPL_ELEMENT_CLONE_WITH_INIT
>   DOMCI_NODE_DATA (yes, this means some consumers will need to keep including
>                    nsGenericElement.h)
>   NS_IMPL_STRING_ATTR

Moved.

> And these are likely, but might not be quite as mechanical:
>   PostQueryInterface stuff
>   NS_ELEMENT_INTERFACE_MAP_END 
>   NS_ELEMENT_INTERFACE_TABLE_TO_MAP_SEGUE

I didn't touch these, because I don't know enough about how COM works to want to take a stab at it.

> For GetClassAttributeName it's probably best to just move it from nsIContent
> to Element.
> . . .
> IsAttributeMapped should move from nsIContent to Element.
> . . .
> GetAttributeChangeHint should move from nsIContent to Element.

I'm fine with that, but I'd like it in a followup bug because callers will probably have to be changed.

> For GetExistingAttrNameFromQName, it might be nice to put the return null on
> nsIContent and then move it from here to nsGenericElement.

Why not move that to Element too?

> nsNodeWeakReference can move into nsFragmentOrElement.cpp, I think.

Probably, but it's not really related to this bug, so I'd prefer to make it a followup.  Better to get this series landed sooner, particularly since every change to nsGenericElement.{h,cpp} bitrots it.

> The comment describing the class should say it's a base class for elements
> and document fragments.

Okay.

> I wish we could move nsDOMSlots, but I guess we actually use mChildrenList
> from it?  That's pretty annoying.  :(

Yeah, GetChildrenList relies on it.

(In reply to :Ms2ger from comment #16)
> Followup to fix
> <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsDOMQS.
> h#66>?

Feel free to file one.

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: New FragmentOrElement.h, v2

Here's the new FragmentOrElement.h, take two.  This time I didn't remove all the #if 0's, because it's tedious and has to be repeated every time I change things, so I'd like to put it off until you're okay with the substantive part.

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 5, v1 -- Move things from FragmentOrElement to nsGenericElement

Comment 20

[Boris Zbarsky [:bzbarsky]]

> These are referenced in cycle-collection macros that I don't understand

Er... Does nsGenericElement not implement cycle collection after your patches?

Seems like it should just implement it as inheriting from FragmentOrElement, and moving the CC macros should be trivial at that point.

> This is called by IsSupported(), which is part of nsIDOMNode

Yes, but it's called as a static method.  You could just include nsGenericElement.h in FragmentOrElement.cpp and call it as an nsGenericElement method.  But again, I think this should just move to nsContentUtils, in a followup if desired.  If that's the plan, I don't care that much where it lives for the moment.

> This kind of method should probably be moved to dom::Element anyway, but that's a
> separate story.

Yep.  Do please file a followup!


> I didn't touch these, because I don't know enough about how COM works to want
> to take a stab at it.

OK.  Please file a followup bug on it?

> I'm fine with that, but I'd like it in a followup bug because callers will
> probably have to be changed.

I checked that before commenting, actually.  For IsAttributeMapped, there is exactly one non-subclass caller, and it has an Element*.  For GetClassAttributeName, likewise.  For GetAttributeChangeHint, likewise.

I'm OK with a followup if you'd prefer, I guess.

> Why not move that to Element too?

That one _would_ need changes to callers, hence I didn't suggest it for this patch.  I'm pretty happy if you want to do that in a followup, though.  ;)

> Probably, but it's not really related to this bug, so I'd prefer to make it a
> followup.

I can live with that, I guess.  Just trying to reduce the amount of stuff we expose to all Element consumers...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Modulo comment 20 the new header looks good to me.

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #20)
> Er... Does nsGenericElement not implement cycle collection after your
> patches?
> 
> Seems like it should just implement it as inheriting from FragmentOrElement,
> and moving the CC macros should be trivial at that point.

Uh, I dunno.  Is there any documentation on how these methods are meant to work?  My new nsGenericElement class definition doesn't have any relevant-looking NS_DECL macros in it, although I might be missing some.  If this is causing leaks all over the place, well, I guess we don't have very good tests for that, because it passed try.

> > This is called by IsSupported(), which is part of nsIDOMNode
> 
> Yes, but it's called as a static method.  You could just include
> nsGenericElement.h in FragmentOrElement.cpp and call it as an
> nsGenericElement method.  But again, I think this should just move to
> nsContentUtils, in a followup if desired.  If that's the plan, I don't care
> that much where it lives for the moment.

Filed bug 776515.

> Yep.  Do please file a followup!

Bug 776517.

> OK.  Please file a followup bug on it?

Bug 776519.

> I checked that before commenting, actually.  For IsAttributeMapped, there is
> exactly one non-subclass caller, and it has an Element*.  For
> GetClassAttributeName, likewise.  For GetAttributeChangeHint, likewise.

Okay, I'll update my patch to move those.

Comment 23

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 5, v2 -- Move things from FragmentOrElement to nsGenericElement

Per last line of comment 22.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 633878 [details] [diff] [review]
Patch part 1, v1 -- Override GetNameSpaceElement in nsDocumentFragment

r=me

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 638213 [details] [diff] [review]
Patch part 2, v1 -- Create FragmentOrElement and move all nsGenericElement functionality to it

I think the SetIsElement() call in the constructor should stay in nsGenericElement instead of moving to FragmentOrElement.

r=me with that.

Can you file a followup bug to move the querySelector stuff out of nsGenericElement into nsINode.cpp?

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Is there any documentation on how these methods are meant to work?

Not sure.  Let's file a followup bug, with smaug cced, on this?

I don't think you're leaking right now; you just can't move private stuff out of a public header.  ;)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 638214 [details] [diff] [review]
Patch part 3, v1 -- Make Element inherit from FragmentOrElement

r=me, I guess.  This is pretty impossible to actually review; I'll trust that it matches the state of the FragmentOrElement file you posted.  ;)

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 638216 [details] [diff] [review]
Patch part 4, v1 -- Make nsDocumentFragment stop inheriting from nsGenericElement

Can we make bind/unbind on a fragment assert, please?

>+  NS_ABORT_IF_FALSE(mNodeInfo->NodeType() ==
>+                       nsIDOMNode::DOCUMENT_FRAGMENT_NODE &&
>+                     mNodeInfo->Equals(nsGkAtoms::documentFragmentNodeName,

Please fix the indent.

This removes the only consumer of ClearIsElement; you should remove the actual method too, since it's really kinda bizarre to have such a thing.

r=me with those.

Is there a reason to implement HasAttributes/GetAttributes on the superclass, with switching on IsElement() instead of directly in subclasses?  If not, a followup.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

which methods? nsCycleCollectionParticipant has documentation about CanSkip stuff.

Comment 30

[Boris Zbarsky [:bzbarsky]]

I think aryeh is looking for docs on the normal CC boilerplate and how to use it (declaring the helper, adding it to your QI, implementing traverse/unlink).  But _please_ let's do that in a separate bug.  This one is getting crowded.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 644922 [details] [diff] [review]
Patch part 5, v2 -- Move things from FragmentOrElement to nsGenericElement

r=me.  Thank you for bearing with the lag!

Comment 32

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #25)
> I think the SetIsElement() call in the constructor should stay in
> nsGenericElement instead of moving to FragmentOrElement.
> 
> r=me with that.

I didn't do this, because it's moved in part 4.  This patch is meant to make FragmentOrElement behave exactly the same as nsGenericElement -- the differences are added in later patches.

> Can you file a followup bug to move the querySelector stuff out of
> nsGenericElement into nsINode.cpp?

Bug 779084.

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #28)
> Is there a reason to implement HasAttributes/GetAttributes on the
> superclass, with switching on IsElement() instead of directly in subclasses?
> If not, a followup.

Probably not.  Bug 779085.

Thanks for the reviews!

Comment 33

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 6, v2 -- Remove #if 0'd code

Now I just need your signoff on this and I can push.  New try push to be on the safe side: https://tbpl.mozilla.org/?tree=Try&rev=f4cbb34909b6

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 647494 [details] [diff] [review]
Patch part 6, v2 -- Remove #if 0'd code

I believe RegisterFrezableElement/UnregisterFreezableElement can move to nsGenericElement as well.  Sorry I missed that before.

r=me with that.

Comment 35

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/657325841098
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc6363649ed4
https://hg.mozilla.org/integration/mozilla-inbound/rev/e349e8b0f48f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9de96cdf02c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/768dbbd4d968
https://hg.mozilla.org/integration/mozilla-inbound/rev/07ba7d3a5b56

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/657325841098
https://hg.mozilla.org/mozilla-central/rev/bc6363649ed4
https://hg.mozilla.org/mozilla-central/rev/e349e8b0f48f
https://hg.mozilla.org/mozilla-central/rev/9de96cdf02c2
https://hg.mozilla.org/mozilla-central/rev/768dbbd4d968
https://hg.mozilla.org/mozilla-central/rev/07ba7d3a5b56

Comment 37

[John Daggett (:jtd)]

(In reply to :Aryeh Gregor from comment #8)
> So now I'm getting errors like this:
> 
> In file included from ../../../../../dist/include/nsAString.h:11:0,
>                  from ../../../../../dist/include/nsSubstring.h:11,
>                  from ../../../../../dist/include/nsString.h:13,
>                  from ../../../../../dist/include/nsAttrValue.h:15,
>                  from ../../../../../dist/include/nsAttrAndChildArray.h:18,
>                  from
> ../../../../../dist/include/mozilla/dom/FragmentOrElement.h:22,
>                  from ../../../../../dist/include/mozilla/dom/Element.h:10,
>                  from ../../../../../dist/include/mozilla/dom/Link.h:14,
>                  from
> /mnt/extra/checkouts/mozilla-central/toolkit/components/places/tests/cpp/
> mock_Link.h:14,
>                  from
> /mnt/extra/checkouts/mozilla-central/toolkit/components/places/tests/cpp/
> test_IHistory.cpp:12:
> ../../../../../dist/include/nsStringFwd.h:16:2: error: #error Internal
> string headers are not available from external-linkage code.
> In file included from ../../../../../dist/include/nsAString.h:35:0,
>                  from ../../../../../dist/include/nsSubstring.h:11,
>                  from ../../../../../dist/include/nsString.h:13,
>                  from ../../../../../dist/include/nsAttrValue.h:15,
>                  from ../../../../../dist/include/nsAttrAndChildArray.h:18,
>                  from
> ../../../../../dist/include/mozilla/dom/FragmentOrElement.h:22,
>                  from ../../../../../dist/include/mozilla/dom/Element.h:10,
>                  from ../../../../../dist/include/mozilla/dom/Link.h:14,
>                  from

[..snip..]

> /mnt/extra/checkouts/mozilla-central/toolkit/components/places/tests/cpp/
> mock_Link.h:14,
>                  from
> /mnt/extra/checkouts/mozilla-central/toolkit/components/places/tests/cpp/
> test_IHistory.cpp:12:
> ../../../../../dist/include/nsTSubstring.h:8:2: error: #error Cannot use
> internal string classes without MOZILLA_INTERNAL_API defined. Use the frozen
> header nsStringAPI.h instead.
> 
> Which seem to be telling me "don't include nsAttrAndChildArray.h in a file
> you want to export".  This seems to be what Boris was warning about.  What's
> a good solution to this?  Should I just scrap my existing patches and make
> FragmentOrElement not inherit from anything, so nsGenericElement inherits
> from both FragmentOrElement and Element directly?  The FragmentOrElement
> class is almost useless to have public anyway -- it's not like any public
> methods are actually going to return one.  Or is there a way to salvage this
> way of doing it?

So I just blew a day trying to figure out why my local patches produce this very same error after you landed this patch.  It looks like you band-aided over the instances here but left it for others to experience!  In my case, adding #include "nsString.h" to a file included by nsFont.h caused the same problem.

It seems like the underlying test code in places shouldn't be including headers like these unless MOZILLA_INTERNAL_API is defined.

Please file bugs for these problems rather than just papering them over in the code you're working on.  Filed bug 780070.

Comment 38

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

IIRC, I didn't understand the problem and asked in #developers for advice, and someone suggested that I use nsStringGlue.h instead.  I had no reason to think it was an actual problem.