830879 - HTML Element WebIDL bindings require classinfo

Status: RESOLVED FIXED

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

Description

[David Humphrey (:humph)]

I've been trying to update the patch in bug 629350, and at the suggestion of Ms2ger, I tried doing it with just HTMLTrackElement.webidl and no nsIDOMHTMLTrackElement.idl, or classinfo additions.

I was able to get close, but not close enough.  Here is what I ended-up with:

HTMLTrackElement.h:
-------------------

#include "nsIDOMHTMLElement.h"
#include "nsIDOMEventTarget.h"
#include "nsGenericHTMLElement.h"
#include "nsGkAtoms.h"

namespace mozilla {
namespace dom {

class HTMLTrackElement : public nsGenericHTMLElement,
                         public nsIDOMHTMLElement
{
public:
  HTMLTrackElement(already_AddRefed<nsINodeInfo> aNodeInfo)
    : nsGenericHTMLElement(aNodeInfo)
  {
    SetIsDOMBinding();
  }
  virtual ~HTMLTrackElement();

  NS_DECL_ISUPPORTS_INHERITED

  NS_FORWARD_NSIDOMNODE_TO_NSINODE

  NS_FORWARD_NSIDOMELEMENT_TO_GENERIC

  NS_FORWARD_NSIDOMHTMLELEMENT_TO_GENERIC

  ...

  virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const;
  virtual nsXPCClassInfo* GetClassInfo();
  virtual nsIDOMNode* AsDOMNode() { return this; }

protected:
  virtual JSObject* WrapNode(JSContext *aCx, JSObject *aScope,
                             bool *aTriedToWrap) MOZ_OVERRIDE;
};

} // namespace dom                                   
} // namespace mozilla


HTMLTrackElement.cpp:
---------------------
#include "mozilla/dom/HTMLTrackElement.h"
#include "mozilla/dom/HTMLTrackElementBinding.h"
#include "mozilla/ErrorResult.h"
#include "nsContentUtils.h"

NS_IMPL_NS_NEW_HTML_ELEMENT(Track)
                             
namespace mozilla {
namespace dom {

HTMLTrackElement::~HTMLTrackElement()
{
}

NS_IMPL_ADDREF_INHERITED(HTMLTrackElement, Element)
NS_IMPL_RELEASE_INHERITED(HTMLTrackElement, Element)

// QueryInterface implementation for HTMLTrackElement
NS_INTERFACE_TABLE_HEAD(HTMLTrackElement)
  NS_HTML_CONTENT_INTERFACE_TABLE1(HTMLTrackElement,
                                   nsIDOMHTMLElement)
  NS_HTML_CONTENT_INTERFACE_TABLE_TO_MAP_SEGUE(HTMLTrackElement,
                                               nsGenericHTMLElement)
NS_HTML_CONTENT_INTERFACE_MAP_END

NS_IMPL_ELEMENT_CLONE(HTMLTrackElement)

JSObject*
HTMLTrackElement::WrapNode(JSContext* cx, JSObject* scope, bool* triedToWrap)
{
  return HTMLTrackElementBinding::Wrap(cx, scope, this, triedToWrap);
}
...

This fails to build:

12:27 < humph> is it the
12:27 < humph>   virtual nsXPCClassInfo* GetClassInfo();
12:28 < bz> Yes
12:28 < bz> nuke that

Removing that line then produces:

1:00.87 HTMLTrackElement.cpp
 1:02.43 /Users/dave/Sites/repos/mozilla-central/content/html/content/src/HTMLTrackElement.cpp:32:23: error: allocating an object of abstract class type 'mozilla::dom::HTMLTrackElement'
 1:02.43 NS_IMPL_ELEMENT_CLONE(HTMLTrackElement)
 1:02.43                       ^
 1:02.43 ../../../../dist/include/mozilla/dom/Element.h:1175:26: note: expanded from macro 'NS_IMPL_ELEMENT_CLONE'
 1:02.43   _elementName *it = new _elementName(ni.forget());                         \
 1:02.43                          ^
 1:02.43 ../../../../dist/include/nsINode.h:1495:27: note: unimplemented pure virtual method 'GetClassInfo' in 'HTMLTrackElement'
 1:02.43   virtual nsXPCClassInfo* GetClassInfo() = 0;
 1:02.43                           ^
 1:02.51 In file included from /Users/dave/Sites/repos/mozilla-central/content/html/content/src/HTMLTrackElement.cpp:6:
 1:02.51 In file included from ../../../../dist/include/mozilla/dom/HTMLTrackElement.h:11:
 1:02.51 /Users/dave/Sites/repos/mozilla-central/content/html/content/src/nsGenericHTMLElement.h:1805:16: error: allocating an object of abstract class type 'mozilla::dom::HTMLTrackElement'
 1:02.51     return new U(aNodeInfo);
 1:02.51                ^
 1:02.51 /Users/dave/Sites/repos/mozilla-central/content/html/content/src/HTMLTrackElement.cpp:11:1: note: in instantiation of function template specialization 'mozilla::dom::NewHTMLElementHelper::Create<nsHTMLTrackElement, mozilla::dom::HTMLTrackElement>' requested here
 1:02.51 NS_IMPL_NS_NEW_HTML_ELEMENT(Track)
 1:02.51 ^
 1:02.51 /Users/dave/Sites/repos/mozilla-central/content/html/content/src/nsGenericHTMLElement.h:1837:10: note: expanded from macro 'NS_IMPL_NS_NEW_HTML_ELEMENT'
 1:02.51   return mozilla::dom::NewHTMLElementHelper::                                \
 1:02.51          ^
 1:02.51 2 errors generated.

12:29 < Ms2ger> Does anything implement it on the parent chain?
12:29 < humph> new problem
12:30 < Ms2ger> Yeah
12:30 < humph> the clone bit is wrong now
12:30 < Ms2ger> No, it's just new HTMLTrackElement() failing
12:30 < Ms2ger> Because you removed GetClassInfo() :)
12:30 < bz> er..
12:30 < bz> Why does GetClassInfo affect HTMLTrackElement()?
12:30 < bz> Is it pure virtual?
12:31 < Ms2ger> Yeah
12:31 < bz> damn
12:31  * bz thinks
12:32 < Ms2ger> Would returning null be an issue?
12:32 < bz> So one option is to instead inherit from HTMLElement
12:32 < bz> instead of nsGenericHTMLEleemnt
12:33 < bz> I don't know
12:33 < bz> the GetClassInfo() caller seems to be in xpcObjectHelper
12:33 < bz> fwiw
12:33 < Ms2ger> Yeah
12:33 < Ms2ger> Do we hit that?
12:33 < bz> Not sure
12:34 < bz> Could, for passing these objects to non-webidl methods
12:34 < bz> Or something

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

I was already working on making this possible, I think this should help.

Comment 2

[David Humphrey (:humph)]

This patch, with my current classinfo-less HTMLTrackElement work, let me build successfully:

[15:02:55.348] HTMLTrackElement
[15:02:55.358] function HTMLTrackElement() {
    [native code]
}

And it lives in the DOM too :)

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1.1

This replaces a compile-time check (pure virtual GetClassInfo()) with a runtime-assertion, but there's no way around that I think.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 707525 [details] [diff] [review]
v1.1

The assert makes no sense.  That code is only reached when mXPCClassInfo is nonnull.

Furthermore, this is going to end up doing a CallQI for webidl-binding nodes, for the same reason.  Is that purposeful?

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Bleh, this seems more like what I wanted to do.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 707781 [details] [diff] [review]
v1.2

Yeah, this I buy.  r=me

Comment 7

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

Comment on attachment 707781 [details] [diff] [review]
v1.2

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

::: js/xpconnect/src/XPCQuickStubs.cpp
@@ +1026,5 @@
> +xpcObjectHelper::AssertGetClassInfoResult()
> +{
> +    MOZ_ASSERT(mXPCClassInfo ||
> +               static_cast<nsINode*>(GetCanonical())->IsDOMBinding(),
> +               "GetClassInfo() should only return null for new DOM bindings!");

Also assert mIsNode, please.

Comment 8

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b48d4a8141bd

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b48d4a8141bd