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]
:
Mentors:
Depends on: 819745 819751 833518
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-04 12:06 PST by Peter Van der Beken [:peterv]
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]
bzbarsky: review+
Details | Diff | Review
Remove hardcoded interface list in workers v1 (5.62 KB, patch)
2012-12-04 12:31 PST, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | 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]
bzbarsky: review+
Details | Diff | Review
Replace HTMLElement quickstubs with new binding methods v1 (44.51 KB, patch)
2012-12-04 12:37 PST, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | Review

Description Peter Van der Beken [:peterv] 2012-12-04 12:06:27 PST

    
Comment 1 Peter Van der Beken [:peterv] 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] (Out June 25-July 6) 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] 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] 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] 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] (Out June 25-July 6) 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] (Out June 25-July 6) 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] (Out June 25-July 6) 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 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.