HTML Element WebIDL bindings require classinfo

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: humph, Assigned: peterv)

Tracking

(Blocks 1 bug)

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

6 years ago
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
Assignee

Comment 1

6 years ago
Posted patch v1 (obsolete) — Splinter Review
I was already working on making this possible, I think this should help.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Reporter

Comment 2

6 years ago
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 :)
Blocks: webvtt
Assignee

Updated

6 years ago
Blocks: 833446
Assignee

Comment 3

6 years ago
Posted patch v1.1 (obsolete) — Splinter Review
This replaces a compile-time check (pure virtual GetClassInfo()) with a runtime-assertion, but there's no way around that I think.
Attachment #702428 - Attachment is obsolete: true
Attachment #707525 - Flags: review?(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?
Assignee

Comment 5

6 years ago
Posted patch v1.2Splinter Review
Bleh, this seems more like what I wanted to do.
Attachment #707525 - Attachment is obsolete: true
Attachment #707525 - Flags: review?(bzbarsky)
Attachment #707781 - Flags: review?(bzbarsky)
Comment on attachment 707781 [details] [diff] [review]
v1.2

Yeah, this I buy.  r=me
Attachment #707781 - Flags: review?(bzbarsky) → review+
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.
Reporter

Updated

6 years ago
Blocks: 629801
https://hg.mozilla.org/mozilla-central/rev/b48d4a8141bd
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 842101
No longer depends on: 842101
You need to log in before you can comment on or make changes to this bug.