Closed Bug 851639 Opened 7 years ago Closed 7 years ago

Let JS-implemented WebIDL code be used with window.navigator

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 5 obsolete files)

This seems like the highest priority followup for JS-implemented WebIDL.  The main question is whether it should be implemented now for the old-style navigator bindings, and then converted later when navigator is WebIDL-ized, or just wait until navigator is converted to new bindings.  That seems like mainly a question of time scales for these various things.

My plan here was to start looking into how to do this with the current setting, and we'll just see how long that takes, and where navigator conversion is when that might be vaguely ready for landing.  From what little I understand of it, it sounds like mainly a matter of coming up with a glob of code, much like the constructor generation, so hopefully it won't be that hard.
Attached patch rough prototype (obsolete) — Splinter Review
This is a rough patch, but it shows the basic idea.  Basically, I add a new hook nsScriptNameSpaceManager::RegisterNavigatorDefineDOMInterface() that shoves a dom::DefineInterface into the navigator hash table, which gets called in RegisterBindings like the window stuff.  In the resolve hook for navigator, if we find a new DOM binding thing, we call the DefineInterface to generate a new object, and stick it on the navigator object like the existing stuff.

But anyways, with my simple example class in bug 827486, and this patch, the following works:

  var bar = window.navigator.MyNumber;
  bar.value = 204;
  var foo = window.navigator.MyNumber;
  alert(bar.value + " !!! " + foo.value);

No additional modifications to MyNumber are required.  The alert outputs "204 !!! 204".

Limitations:
1. It does this for every JS implemented thing.
2. You can't pick what it is called.
3. I suspect that if you don't specify Constructor, the right bindings functions won't be generated, so it'll break.

I guess I'll define some kind of NavigatorProperty="myString" extended attribute to specify adding something to navigator, then generate the right stuff when this is specified.  The funny thing is, I don't think there's anything particularly specific to JS implemented WebIDL, so we should be able to stick any old thing on navigator.
I also need to do the Xray dance in the navigator resolve hook.
How hard would this be to extend this idea to other Web IDL interfaces?
As I said in the last sentence of comment 1, I think it could be used for anything implemented with WebIDL.
Comment on attachment 727885 [details] [diff] [review]
rough prototype

Boris, does this general approach seem reasonable?  In the DOM bindings calls, there was some discussion of putting things on the native, not the wrapper, which this does not do, but maybe I was misunderstanding.
Attachment #727885 - Flags: feedback?(bzbarsky)
So what this does is no worse than what we do now.  When we convert Navigator to WebIDL this won't be following the spec, but I guess it's ok for the moment.

That said, this is putting the _constructor_ on the navigator, not an instance, right?
Attachment #727885 - Flags: feedback?(bzbarsky) → feedback+
Andrew do you have a time estimation for this bug? Days, weeks,...?
Days.  If I can manage to focus on this tomorrow, I'll hopefully get a patch up tomorrow.
Okay, not today, but hopefully this week...
Attached patch rough prototype, more working (obsolete) — Splinter Review
As Boris pointed out, the previous version wasn't actually creating a new object to hang off of navigator.  My local test happened to work because it was just setting properties on the constructor object.  Oops.  Anyways, here's a new version that holds up to more in-depth testing, like checking that the initial value of the property is as expected, and that you can call methods.

A function is registered as before for the script name manager of navigator, we just codegen a proper function that actually constructs an object.  It codegens a function ConstructObjectHelper with the type that works well with the method call codegen, then manually codegens a ConstructObject that munges the types appropriately.  Now to add the other niceties.
Attachment #727885 - Attachment is obsolete: true
Blocks: 856820
Attached patch WIP (obsolete) — Splinter Review
Attachment #730373 - Attachment is obsolete: true
Remaining issues:

-- Should I rename mDefineDOMInterface and DefineDOMInterface because I'm making them more general?  CreateDOMThing?

-- Supporting init: should I just use nsIDOMGlobalPropertyInitializer?  This would be easy to implement: in the Constructor I generate, after the inner JS object is created, just attempt to QI to nsIDOMGPI and pass it the window.  This isn't a very "modern" approach (which would involve something more like bug 851178), but given how we're already quite reliant on XPCOM components, I don't see any particular reason not to do that.

Any thoughts on these, bz?
Flags: needinfo?(bzbarsky)
> Should I rename mDefineDOMInterface and DefineDOMInterface because I'm making them more
> general?

Well, more precisely you're doing something which has the same signature but different behavior.  The current mDefineDOMInterface actually defines properties on objects and whatnot, as I recall.

A struct would never need both an mDefineDOMInterface and your new thing, right?  Can you just do a union, with comments that mDefineDOMInterface is used for the global and the other is used for navigator?

It's not clear to me whether the Xray bits in the attached patch are at all right, but either way the "global" there is actually a navigator object.  Do the callees expect an actual global?

> should I just use nsIDOMGlobalPropertyInitializer?

For now, yeah.  Makes it easier to switch people over.

You probably want to get Kyle to review the parser changes.  I do wonder whether we need to do that or whether we can construct (no pun intended) something that quacks the right way directly in codegen...
Flags: needinfo?(bzbarsky)
> Can you just do a union
Sounds good.

> It's not clear to me whether the Xray bits in the attached patch are at all right
Yeah, I'm not really sure.  I can throw together some kind of test.

> Do the callees expect an actual global?
Ah, it looks like not actually, because it constructs a GlobalObject from the object that is passed in, which in turn calls JS_GetGlobalForObject.

> I do wonder whether we need to do that or whether we can construct (no pun intended) something that quacks the right way directly in codegen...
Sure, I can try that if you think it would be better.
> Sure, I can try that if you think it would be better.

I think I would somewhat prefer that...
While I'm fiddling with things, right now you define a navigator property like so:

[NavigatorProperty="mozMyNumber", JSImplementation="@mozilla.org/my-number;1"]
interface MyNumber {
  ...

where the object becomes navigator.mozMyNumber.  I'm open to suggestions for things to call this besides "NavigatorProperty".
Calling it NavigatorProperty seems fine to me.
Once Navigator is converted to WebIDL, it would probably be better that these annotations live on Navigator, to avoid the current "come from" semantics.
That's not obvious, especially if we want to conditionally compile some of these...
Isn't that the purpose of WebIDL's "partial interface"?
Maybe, but our build system dependency tracking can't deal with partial interfaces defined in a file other than the main interface.
I looked at every component in the tree that implements nsIDOMGlobalPropertyInitializer, and for all but one, they basically all just use the native, except they return NULL to indicate failure.  The one exception is mozPay[1], which is a function on navigator, not an object.  That's not something we really support anyways, so if it seems reasonable, I'm only going to deal with undefined and null as return values, and error out otherwise.  I guess in the case of returning null, the property just gets set to null, there's otherwise no particular exception that gets fired off.

[1] mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.js
Blocks: 859878
Attached patch WIP (obsolete) — Splinter Review
Added support for global property initializer.  It ignores any return values of init(), so things like preference and permission checking will have to be handled elsewhere.

I also got rid of the weird IDLMethod for navigator constructor, and instead hard code most of what is needed.  Not too pretty.

I still need to fix various things in the non-codegen parts of the code.
Attachment #732132 - Attachment is obsolete: true
Depends on: 865260
RTCPeerConnection::Constructor could use this nsIDOMGlobalPropertyInitializer::Init call as well to get its window pointer, but with the current patch, it appears to not qualify as isNavigatorConstructor=True judging by the resulting generated code.

Can this be fixed?
No longer blocks: 823512
I split out the initializer stuff into bug 865544, as it is orthogonal to the navigator registration stuff, and of broader interest.
Attached patch WIP (obsolete) — Splinter Review
Attachment #735517 - Attachment is obsolete: true
I forgot to root a value.

My test case seemed to work okay:
  var foo = window.navigator.mozMyNumber;
  var num1 = foo.value;
  foo.value = 1;
  var bar = window.navigator.mozMyNumber;
  var num2 = bar.value;
  foo.value = 200;
  var num3 = foo.value;
  var num4 = bar.doubleValue();

  alert(num1 + " !!! " + num2 + " !!! " + num3 + " !!! " + num4 + " !!! " + foo.win);
Attachment #743360 - Attachment is obsolete: true
Attachment #743393 - Flags: review?(bzbarsky)
Comment on attachment 743393 [details] [diff] [review]
support dynamic registration on navigator

>+    JS::Rooted<JSObject*> domObject(cx, nullptr);

Just:

  JS::Rooted<JSObject*> domObject(cx);

should be fine.  It defaults to nullptr.

>+    mozilla::dom::ConstructNavigatorProperty aNaviConstructor,

I think I'd prefer "aNavConstructor".

>+'TestNavigatorWithCon' : {

'TestNavigatorWithConstructor', please.

Why does CGConstructNavigatorObjectHelper need both a definition_body and a generate_code?  I think you can get rid of the latter and just have the former return genConstructorBody.

>+    ThrowMethodFailedWithDetails<true>(aCx, rv, "${descriptorName}", "navigatorConstructor");

Why true instead of the "not descriptor.workers" value?

Why do we need the separate CGConstructNavigatorObjectHelper?  Just so we can easily share the genConstructorBody bits with CGJSImplMethod bits?  If so, please document that?

Why is the aEnabled argument to the construct function needed?  Is it ever set to false?

This setup won't show the new properties on Xrays for navigator, but afaict neither does the existing one, so that's ok.

r=me with the above fixed
Attachment #743393 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #28)
>   JS::Rooted<JSObject*> domObject(cx);
> should be fine.  It defaults to nullptr.
Done.

> I think I'd prefer "aNavConstructor".
Done.

> 'TestNavigatorWithConstructor', please.
Done.

> Why does CGConstructNavigatorObjectHelper need both a definition_body and a
> generate_code?  I think you can get rid of the latter and just have the
> former return genConstructorBody.
Done.

> Why true instead of the "not descriptor.workers" value?
Because I was copying code I didn't understand. :)  Fixed.

> If so, please document that?
Yeah, just to make code sharing easier.  I added a comment.

> Why is the aEnabled argument to the construct function needed?  Is it ever
> set to false?
Removed the enabled construct argument.

> This setup won't show the new properties on Xrays for navigator, but afaict
> neither does the existing one, so that's ok.

Yeah, I was just trying to copy the existing behavior for dynamic registration on navigator and window.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0b317249f19
https://hg.mozilla.org/mozilla-central/rev/e0b317249f19
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
This needs documenting too.
Keywords: dev-doc-needed
Documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

"If you want an instance of the class to be added to window.navigator, add an extended attribute NavigatorProperty="PropertyName" which will make the instance available as window.navigator.PropertyName."
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.