Closed Bug 824007 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: peterv, Assigned: peterv)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

No description provided.
This enables us to keep using the same macro for both nsHTML*Element and mozilla::dom::HTML*Element classes.
This shouldn't change any behaviour, it's mostly search and replace and moving files around.
Attached patch v1Splinter Review
Attachment #694911 - Flags: review?(bzbarsky)
Comment on attachment 694912 [details] [diff] [review]
Move and rename some HTML element classes v1

Just renaming and moving code.
Attachment #694912 - Flags: review?(bzbarsky)
Attachment #694913 - Flags: review?(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
Attachment #694911 - Flags: review?(bzbarsky) → review+
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.
Attachment #694912 - Flags: review?(bzbarsky) → review+
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.
Attachment #694913 - Flags: review?(bzbarsky) → review+
(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.
> 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.
https://hg.mozilla.org/mozilla-central/rev/fec5950b88a2
https://hg.mozilla.org/mozilla-central/rev/840b27e18640
https://hg.mozilla.org/mozilla-central/rev/824e649bab0c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 841466
Duplicate of this bug: 843661
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.