Replace HTMLElement quickstubs with new binding methods

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
a month ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

7 years ago
Currently compilation fails for castable interfaces in union types because we don't include the binding headers in UnionConversion.h but then then try to use the PrototypeTraits etc.
Attachment #688401 - Flags: review?(bzbarsky)
Comment on attachment 688401 [details] [diff] [review]
Support castable types in unions v1

r=me
Attachment #688401 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 3

7 years ago
It's annoying to have to change the dom/worker code just because we added an interface that has a longer prototype chain than the current max.
Attachment #688409 - Flags: review?(bzbarsky)
Comment on attachment 688409 [details] [diff] [review]
Remove hardcoded interface list in workers v1

I like.  Well, not the necessity, but the solution.  ;)
Attachment #688409 - Flags: review?(bzbarsky) → review+
Comment on attachment 688411 [details] [diff] [review]
Add a constructor to callbacks to allow conversion between callback types v1

r=me
Attachment #688411 - Flags: review?(bzbarsky) → review+
Comment on attachment 688413 [details] [diff] [review]
Replace HTMLElement quickstubs with new binding methods v1

>+#define EVENT(name_, id_, type_, struct_) /* nothing; handled by the nsINode */

s/the nsINode/nsINode/

>+    nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(win);           \
>+    nsGlobalWindow* globalWin = nsGlobalWindow::FromSupports(sgo);          \

Why not just QI to nsISupports, since FromSupports _has_ to work with the canonical nsISupports?

Same in a few more places.

>+     return new EventHandlerNonNull(errorHandler);                         \

I would much rather we had this method return already_AddRefed<EventHandlerNonNull> and addref inside here as needed, to make sure that the caller is not just sticking it into a raw ptr.

As a side note, maybe we should allow resultNotAddRefed for callbacks?

>+  OnErrorEventHandlerNonNull* errorHandler =                                \
>+    new OnErrorEventHandlerNonNull(handler);                                \

nsRefPtr, please, so we're not passing in a 0-refcount object.

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

Could you file a followup on using some of the existing nsDOMEventTargetHelper bits in this code?

r=me with the above nits fixed.
Attachment #688413 - Flags: review?(bzbarsky) → review+
Comment on attachment 688413 [details] [diff] [review]
Replace HTMLElement quickstubs with new binding methods v1

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

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +1896,5 @@
> +                                                                              \
> +  return nsINode::SetOn##name_(handler, error);                               \
> +}
> +#include "nsEventNameList.h"
> +#undef ERROR_EVENT  

Trailing whitespace

::: dom/base/nsGlobalWindow.cpp
@@ +11330,5 @@
>  #include "nsEventNameList.h"
>  #undef TOUCH_EVENT
>  #undef WINDOW_ONLY_EVENT
> +#undef BEFOREUNLOAD_EVENT  
> +#undef ERROR_EVENT  

And here

::: dom/base/nsGlobalWindow.h
@@ +673,5 @@
> +#include "nsEventNameList.h"
> +#undef TOUCH_EVENT
> +#undef WINDOW_ONLY_EVENT
> +#undef BEFOREUNLOAD_EVENT  
> +#undef ERROR_EVENT  

And here

Updated

6 years ago
Depends on: 819751
Depends on: 833518
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.