Closed Bug 787574 Opened 8 years ago Closed 8 years ago

Add a script for creating stub implementations of Web IDL interfaces

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: ehsan, Assigned: ehsan)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #657460 - Flags: review?(bzbarsky)
Comment on attachment 657460 [details] [diff] [review]
Patch (v1)

A few comments:

1) It would be better to put the class in the mozilla::dom namespace.  That's
   the namespace the bindings expect the implementation class to be in by
   default; if you do it that way, you don't have to indicate the nativeType in
   the conf file.
2) Does it make sense to make GetParentObject pure vitrtual so people have to
   actually make sure to implement it correctly?
3) I would prefer we don't do the "moz" thing.  Just SkeletonBinding.  We
   shouldn't really be prefixing DOM APIs anyway, if we can avoid it.
4) The indent on the WrapObject function header is going to be off depending on
   the length of the classname.  Perhaps just put it all on one line?

r=me with the above at least considered.
Attachment #657460 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #2)
> Comment on attachment 657460 [details] [diff] [review]
> Patch (v1)
> 
> A few comments:
> 
> 1) It would be better to put the class in the mozilla::dom namespace.  That's
>    the namespace the bindings expect the implementation class to be in by
>    default; if you do it that way, you don't have to indicate the nativeType
> in
>    the conf file.

Good point, I'll do that.

> 2) Does it make sense to make GetParentObject pure vitrtual so people have to
>    actually make sure to implement it correctly?

Hmm, I thought about that...  But I don't really want to advertize that GetParentObject needs to be virtual.  How about I make the body of the function not compile by let's say, |return somethingSensible;|?

> 3) I would prefer we don't do the "moz" thing.  Just SkeletonBinding.  We
>    shouldn't really be prefixing DOM APIs anyway, if we can avoid it.

OK, will do.

> 4) The indent on the WrapObject function header is going to be off depending
> on
>    the length of the classname.  Perhaps just put it all on one line?

Sure, I'll put it all on the same line.
> How about I make the body of the function not compile by let's say, |return
> somethingSensible;|?

Sounds good.
Attachment #657460 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc505e515aa
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/0fc505e515aa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 658928 [details] [diff] [review]
Patch for landing

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

::: dom/bindings/stubgenerator/Skeleton.h
@@ +7,5 @@
> +#include "nsWrapperCache.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "mozilla/Attributes.h"
> +
> +class JSContext;

Nit: I believe some compilers will warn here, because JSContext is actually a struct, not a class.
(In reply to :Ms2ger from comment #8)
> Comment on attachment 658928 [details] [diff] [review]
> Patch for landing
> 
> Review of attachment 658928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/stubgenerator/Skeleton.h
> @@ +7,5 @@
> > +#include "nsWrapperCache.h"
> > +#include "nsCycleCollectionParticipant.h"
> > +#include "mozilla/Attributes.h"
> > +
> > +class JSContext;
> 
> Nit: I believe some compilers will warn here, because JSContext is actually
> a struct, not a class.

Thanks, fixed. https://hg.mozilla.org/mozilla-central/rev/8f9db0705d2f
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.