824007 - Convert HTMLBodyElement, HTMLDataListElement, HTMLFontElement, HTMLFrameSetElement and HTMLLabelElement to new DOM bindings

Status: RESOLVED FIXED

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

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Make NS_IMPL_NS_NEW_HTML_ELEMENT macros work with HTML element classes in mozilla::dom v1

This enables us to keep using the same macro for both nsHTML*Element and mozilla::dom::HTML*Element classes.

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Move and rename some HTML element classes v1

This shouldn't change any behaviour, it's mostly search and replace and moving files around.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 694912 [details] [diff] [review]
Move and rename some HTML element classes v1

Just renaming and moving code.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 694911 [details] [diff] [review]
Make NS_IMPL_NS_NEW_HTML_ELEMENT macros work with HTML element classes in mozilla::dom v1

How about this comment before the "struct NewHTMLElementHelper":

// A helper struct to automatically detect whether FooElement is implemented as
// nsHTMLFooElement or as mozilla::dom::HTMLFooElement by using SFINAE to look
// for the InNavQuirksMode function (which lives on nsGenericHTMLElement) on both
// types and using whichever one the substitution succeeds with.

or something along those lines?

r=me with that

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 694912 [details] [diff] [review]
Move and rename some HTML element classes v1

>+++ b/content/base/public/nsINode.h
>+  // - HTMLBodyElement:       mContentStyleRule
>+  // - HTMLDataListElement: mOptions

Keep the mFoo lined up?

>+  // - HTMLFrameSetElement: mRowSpecs, mColSpecs

And here.

>+++ b/content/html/content/src/HTMLFontElement.h
>+    SetIsDOMBinding();

This should be in the next patch, right?

r=me with those fixed.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 694913 [details] [diff] [review]
v1

>+++ b/content/html/content/src/HTMLBodyElement.cpp
>+#define BEFOREUNLOAD_EVENT(name_, id_, type_, struct_)                         \
>+  already_AddRefed<EventHandlerNonNull>                                        \
>+  HTMLBodyElement::GetOn##name_()                                              \
...
>+        return new EventHandlerNonNull(handler);                               \

We need to actually addref that return value.

That said, why do we need this complexity?  Why not just make onbeforeunload of type BeforeUnloadEventHandler in HTMLBodyElement.webidl?

> #undef WINDOW_EVENT

Need to undef BEFOREUNLOAD_EVENT, right?

Though again, it seems like if the type just matched we might not need to even define it separately...

Same comments apply to frameset.

>+++ b/content/html/content/src/HTMLFrameSetElement.cpp

Need to call SetIsDOMBinding() in the constructor, right?

>+++ b/dom/webidl/HTMLBodyElement.webidl
>+  //attribute EventHandler onblur;
>+  //attribute EventHandler onfocus;

Why are those commented out?

>+  //attribute EventHandler onscroll;
>+  //attribute EventHandler onstorage;

And those?

Same thing for frameset.

r=me with the event handler bits sorted out.

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Though again, it seems like if the type just matched we might not need to
> even define it separately...

I'm not sure how, given that the return/argument type will be different.

> Why are those commented out?

That's how the spec does it right now.

Comment 9

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fec5950b88a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/840b27e18640
https://hg.mozilla.org/integration/mozilla-inbound/rev/824e649bab0c

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I'm not sure how, given that the return/argument type will be different.

Hmm... Yeah, indeed.

I have to admit to being a little lost in all the event handler stuff at this point.  :(  There are places where we're calling nsGenericHTMLElement methods, but those will examine the tag, right?  Really wish we could at least drop the XPCOM API for this stuff....

> That's how the spec does it right now.

Hrm.  Ok.  I suspect the spec is overcomplicated, but I guess that's a battle I should fight some other day.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fec5950b88a2
https://hg.mozilla.org/mozilla-central/rev/840b27e18640
https://hg.mozilla.org/mozilla-central/rev/824e649bab0c