Skip to content
This repository has been archived by the owner on Feb 26, 2022. It is now read-only.

Bug 706590 - API for implementing XPCOM interfaces #305

Merged
merged 37 commits into from Feb 5, 2012

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Dec 15, 2011

https://bugzilla.mozilla.org/show_bug.cgi?id=706590

This change modifies xpcom module in order to:

  • Provide simple & consistent (via Base & Namespace) API for implementing complex XCOM interfaces via javascript.
  • Makes XPCOM factory registration / unregistration manual via exposed functions as in case of shared modules Bug 706590 - API for implementing XPCOM interfaces #305 this is more desired (Auto registration / unregistration can be defined via subclassing).
  • Reduces dependencies on this module by extracting UUID generation code into a separate module.
  • Updates all XPCOM dependencies to a new API.

Please note: This work branched out from #263 since it depends on those changes.

}
return headers;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: defineLazyGetter is special, it is used to fetch and set an attribute lazily and only once:
https://developer.mozilla.org/en/JavaScript_code_modules/XPCOMUtils.jsm#defineLazyGetter%28%29
I'd imagine it has been used for performances reason. And it doesn't necessary worth its noise nor using mozilla-dark-feature use!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly it also assumes that this represents a scope, which is valid now, but in of my loader prototypes it was not, so I'd prefer not to use it. If we need to be lazy just use a function that caches return value.

throw e;
else
throw Cr.NS_ERROR_FAILURE;
return this.component.QueryInterface(iid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my long comment first, #305 (comment)
but it seems broken, it looks like you are calling prototype method instead of instance one?
We would need something like:

if (this.singleton)
  return this.singleton;
this.singleton = this.component.new();
return this.singleton.QueryInterface(iid);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what you suggested above was a better. I don't think of a service as of a static object, I think of it as a static object. If one needs singleton component it should be his responsibility to implement a factory with such a new method that creates only one instance.

@Gozala
Copy link
Contributor Author

Gozala commented Feb 1, 2012

See my commens on #305 (comment) below:

1/ Unknown should not provide wrappedJSObject

I do agree, I'll change that

2/ Make Factory inherit from Unknown and Service inherit from Factory.

I guess it will make sense. It's just getService in XPCOM is a strange beast, it's not defined in any interface and everything implicitly implements it. Anyway doing inheritance other way round makes more sense, specially since
registerFactory indeed registers factories.

3/ .createInstance should call .new

Yeah I have been thinking back and forth on this & finally decided that it was more logical this way, since most of the XCOM components that require initialization implement init method and require manual initialization. But you make a good point that requirement of .wrappedJSObject.initialize is probably not very reasonable request either, so I'm fine to make .new instead.

4/ Do not provide default contractID

I see your point, but I would like to avoid introduction of yet another requirement, the whole API is no longer simple :( What about generation of those contractsID's ? It's just you will need to provide validations for things like these and there still may be a conflicts if same ID is used. That being said, on conflict register throws exception with quite well described message.

@Gozala
Copy link
Contributor Author

Gozala commented Feb 1, 2012

BTW thanks for the detailed comments, this really helps when fighting with XPCOM demons!!

@Gozala
Copy link
Contributor Author

Gozala commented Feb 2, 2012

@ochameau I consider this to be ready. Can you please take another look at it ?

@ghost ghost assigned ochameau Feb 2, 2012
throw e;
else
throw Cr.NS_ERROR_FAILURE;
return this.create().QueryInterface(iid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: wrong identation on return this.create().QueryInterface(iid);

@ochameau
Copy link
Contributor

ochameau commented Feb 4, 2012

Tests are failing in test-request.js/test-weakmaps.js.
test-request.testSimpleJSON: timed out
test-request.testStatus200: failure
error: fail: Weakmap still contains our wrapper!

There is a first issue here:
https://github.com/Gozala/addon-sdk/blob/7de95cb4fbdf33c1d160ed4f29b3fd0ea01830e8/packages/addon-kit/tests/test-request.js#L314
I think we should replace if (typeof(o1) != "object") by if (typeof(o1) != "object" || o1 === null)
in order to display clearer errors.

@Gozala
Copy link
Contributor Author

Gozala commented Feb 4, 2012

@ochameau what are you testing against ? All tests pass on my machine both on nightly and stable FF. Are you sure you don't have any local changes or something ?

@ochameau
Copy link
Contributor

ochameau commented Feb 5, 2012

I was testing on FF10, but I'm not able to reproduce anymore :/
Unfortunatly I had to reboot my laptop, and, with a clean Windows instance, it doesn't fail anymore.
It may be some race condition? My laptop was very laggy because of a heavyweight VM instance in background.
The failing test was this one:
https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/tests/test-request.js#L132
response.json was null.
Can you just see if there is any reason why it would be end up being null?

You may want to fix assertDeepEqual method. It throws incomprehensible exception when obj1 is null.
https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/tests/test-request.js#L307

In anycase, I r+ this patch again.

On a side note, I went through Openwebapp source code, and I haven't seen any usecase of xpcom.js.
But we may ping them about this new module that is going to simplify their codebase:
https://github.com/mozilla/openwebapps/blob/develop/addons/activities/lib/main.js#L43

Thanks again for this patch, I really like this module!

@Gozala
Copy link
Contributor Author

Gozala commented Feb 5, 2012

Can you just see if there is any reason why it would be end up being null?

If JSON.parse would have failed then it would be null but I don't see any reason for that to happen, maybe local http server dropped connection early and did not managed to flush all json data.

You may want to fix assertDeepEqual method. It throws incomprehensible exception when obj1 is null.
https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/tests/test-request.js#L307

I'll create a new bug for that.

On a side note, I went through Openwebapp source code, and I haven't seen any usecase of xpcom.js.
But we may ping them about this new module that is going to simplify their codebase:
https://github.com/mozilla/openwebapps/blob/develop/addons/activities/lib/main.js#L43

I'll drop them a note about this ;)

Thanks!

Gozala added a commit that referenced this pull request Feb 5, 2012
fix bug 706590 - API for implementing XPCOM interfaces r=@ochameau
@Gozala Gozala merged commit aae9630 into mozilla:master Feb 5, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants