Peter Van der Beken [:peterv] Assignee
	Description • 12 years ago

      No description provided.

	Peter Van der Beken [:peterv] Assignee
	Comment 1 • 12 years ago

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

	Peter Van der Beken [:peterv] Assignee
	Comment 2 • 12 years ago

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

	Peter Van der Beken [:peterv] Assignee
	Comment 4 • 12 years ago

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

Just renaming and moving code.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 12 years ago

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.

	Peter Van der Beken [:peterv] Assignee
	Comment 8 • 12 years ago

(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.

	Peter Van der Beken [:peterv] Assignee
	Comment 9 • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 12 years ago

> 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 12 years ago

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