Last Comment Bug 818219 - Replace HTMLElement quickstubs with new binding methods
: Replace HTMLElement quickstubs with new binding methods
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla20
Assigned To: Peter Van der Beken [:peterv] - away till Aug 1st
:
Mentors:
Depends on: 819745 819751 833518
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-04 12:06 PST by Peter Van der Beken [:peterv] - away till Aug 1st
Modified: 2013-01-22 16:24 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Support castable types in unions v1 (4.53 KB, patch)
2012-12-04 12:12 PST, Peter Van der Beken [:peterv] - away till Aug 1st
bzbarsky: review+
Details | Diff | Splinter Review
Remove hardcoded interface list in workers v1 (5.62 KB, patch)
2012-12-04 12:31 PST, Peter Van der Beken [:peterv] - away till Aug 1st
bzbarsky: review+
Details | Diff | Splinter Review
Add a constructor to callbacks to allow conversion between callback types v1 (3.59 KB, patch)
2012-12-04 12:36 PST, Peter Van der Beken [:peterv] - away till Aug 1st
bzbarsky: review+
Details | Diff | Splinter Review
Replace HTMLElement quickstubs with new binding methods v1 (44.51 KB, patch)
2012-12-04 12:37 PST, Peter Van der Beken [:peterv] - away till Aug 1st
bzbarsky: review+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] - away till Aug 1st 2012-12-04 12:06:27 PST

    
Comment 1 Peter Van der Beken [:peterv] - away till Aug 1st 2012-12-04 12:12:23 PST
Created attachment 688401 [details] [diff] [review]
Support castable types in unions v1

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.
Comment 2 Boris Zbarsky [:bz] 2012-12-04 12:17:42 PST
Comment on attachment 688401 [details] [diff] [review]
Support castable types in unions v1

r=me
Comment 3 Peter Van der Beken [:peterv] - away till Aug 1st 2012-12-04 12:31:01 PST
Created attachment 688409 [details] [diff] [review]
Remove hardcoded interface list in workers v1

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.
Comment 4 Peter Van der Beken [:peterv] - away till Aug 1st 2012-12-04 12:36:11 PST
Created attachment 688411 [details] [diff] [review]
Add a constructor to callbacks to allow conversion between callback types v1
Comment 5 Peter Van der Beken [:peterv] - away till Aug 1st 2012-12-04 12:37:53 PST
Created attachment 688413 [details] [diff] [review]
Replace HTMLElement quickstubs with new binding methods v1
Comment 6 Boris Zbarsky [:bz] 2012-12-04 12:40:15 PST
Comment on attachment 688409 [details] [diff] [review]
Remove hardcoded interface list in workers v1

I like.  Well, not the necessity, but the solution.  ;)
Comment 7 Boris Zbarsky [:bz] 2012-12-04 12:41:30 PST
Comment on attachment 688411 [details] [diff] [review]
Add a constructor to callbacks to allow conversion between callback types v1

r=me
Comment 8 Boris Zbarsky [:bz] 2012-12-04 13:02:18 PST
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.
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2012-12-05 01:15:41 PST
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

Note You need to log in before you can comment on or make changes to this bug.