Closed Bug 514409 Opened 15 years ago Closed 10 years ago

Create a mechanism to use much of base/util from JS

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX
Future

People

(Reporter: jcranmer, Assigned: jcranmer)

References

Details

Attachments

(3 files, 6 obsolete files)

In order to create new account types, it is absolutely essential to implement nsIMsgIncomingServer (and nsIMsgProtocolInfo, but that's trivial) and may be necessary to implement nsIMsgFolder, nsIMsgMailnewsUrl, and nsIMsgDatabase as well. The size of the interfaces and the degree to which they change are such that asking extension authors to completely reimplement some or all of them by themselves is simply untenable. It would also be bad design to have to keep two separate implementations of these in many places, not to mention the degree to which one would have to rip out logic in nsIMsgDatabase which assumes a single, closed implementation.

So, my current in-progress solution is to provide a mechanism that would allow JS implementors to subclass certain classes (nsMsgIncomingServer and nsMailboxServer to start) and have it just work from both the C++ point of view and the JS point of view. I also intend to move the RSS implementation from C++ to JS to demonstrate its efficacy and hopefully also get some testing (both automated and dogfood) on the feature so we minimize the chance of shipping a broken mechanism.

I hope to have this feature land very early in the Thunderbird 3.next cycle so as to allow maximum baking. With any luck, I should find time to iron out all of xpconnect's complaints by the time 3.0 ships.

This blocks the "easily plugin new account types" bug, IMO.
Attached patch Preliminary work (obsolete) — Splinter Review
This patch is what I have right now. Until I started with the nsIXPCScriptable stuff, it had been working, but it now manages to crash when doing a call to the nsILocalMailIncomingServer that the JS implements. Oh, and xpconnect complains every few calls about not having canCreateWrapper but I have no idea who is calling that where, since it's not going via the other mechanisms that I would have expected it to.
Attached patch More preliminary work (obsolete) — Splinter Review
More preliminary work. I got the bifurcated QI working properly now, by blacklisting nsISecurityCheckedComponent (?), so I can theoretically make RSS pure JS now. Except that I got a shutdown crash at the GetContext because someone QI'd to 9674489b-1f6f-4550-a730-ccaedd104cf9 (NS_CYCLECOLLECTIONPARTICIPANT_IID).

My next task will be to develop the callers such that all of nsIMsgIncomingServer (except methods with outparams) can be implemented via this framework. After that, I'll start working on the JS -> C++ side of things... :-(
Attachment #398372 - Attachment is obsolete: true
Attached patch Yet more preliminary work (obsolete) — Splinter Review
Hmm, I'm running out of names for attachments.

In any case, I'm very close to being able to delete nsRssIncomingServer (just a single variable assignment left!).

At this point, all attributes from nsIMsgIncomingServer can be overriden by JS stuff, but I'm still working on getting a generic macro setup for the regular methods. Once that's done, I can just shove JSMailboxServer to using NS_IMPL_NSIMSGINCOMINGSERVER and then filling in the rest of the virtual methods ad-hoc.

My next patch should also include field get/set support, at least for simple primitive types. Expect to see templates and pointers-to-members there.

It still leaks like a bucket with no bottom, but I'll worry about that later.
Attachment #401369 - Attachment is obsolete: true
Attached patch Now it autogenerates! (obsolete) — Splinter Review
This patch adds in field support, tests, autogeneration support (though I haven't switched JSMailboxServer to that yet), and support for return values. What I don't have yet is:
1. Outparam support.
2. [array] support.
3. xpcom-like non-xpidl methods
4. Super-like constructs.

jsexgen.py is heavily cribbed from qsgen.py if you really couldn't tell.
Attachment #403935 - Attachment is obsolete: true
Attached patch With methods and fields (obsolete) — Splinter Review
Slightly more advanced, I have:
* Tweaked autogenerated code
* Added some more tests
* Added field support
* Added method support
* Refactored some stuff
* Made some more documentation
* Added outparam support
* Removed the non-autogenerated code
* Fixed the QI issues... kind of

Problems:
1. Blows up if QI'd during GC.
2. Needs more method support.
3. The JS goodies module needs more love.
4. No array support (so can't do nsIMsgFolder yet)
5. Methods need to be better crafted.
6. Wanna bet this blows up in MSVC?
Attachment #408009 - Attachment is obsolete: true
Attached patch More progress (obsolete) — Splinter Review
This has a lot more progress. I just need to finish writing the tests, and then do some cleanup.

I may end up removing the nsIVariant return type conversion in favor of just publicizing more generic JS<->C++ methods.
Attachment #411959 - Attachment is obsolete: true
Can this be structured as an extension?  While this is obviously tremendous technical work, the patch's net benefit from a purely mailnews perspective is currently a reduction in net code size for the (very simple and long stable) RSS account implementation at the cost of a net increase in code size for the framework to support that and a serious mental complexity increase (from a what-could-go-wrong perspective).

It seems like it would make sense to wait for a killer feature that would be impractical to develop in C++ to exist that uses the technology before bringing this into mailnews 'core'.  If this exists as an extension, it would allow developers to use the technology to create those killer features and adventurous users to try them out.

Since I think a lot of the JS magic is not really mailnews specific, another possibility is to get the JS magic reviewed and landed in mozilla/ proper.  (Review for the JS account stuff would still need to consider the risk/complications of assumptions in existing C++ code calling into accounts where it might not expect the multitude of things JS code could get up to.)
This patch adds just the jsextended library, its test code, and the necessary build fu to make and package it.
Attachment #420597 - Attachment is obsolete: true
Attachment #422043 - Flags: superreview?(bugzilla)
Attachment #422043 - Flags: review?(jorendorff)
This patch adds in the JS versions of nsMailboxServer, nsMsgIncomingServer, and nsMsgDBFolder, as well as converts RSS into a pure JS component.

It obviously depends on the library patch to build and work.
Attachment #422044 - Flags: superreview?(dmose)
Attachment #422044 - Flags: review?(bienvenu)
Joshua, what was the word on whether this can be structured as an extension?
Sorry about the delay in response.

If this were to be structured as an extension, it would be a binary component that would need to be recompiled anytime nsIMsgFolder or nsIMsgIncomingServer were changed--which is about once a month, according to hg log. They would also have to be prepared on the three distributions, which many would-be extension developers probably do not have access to.

In terms of what can or can't be done in C++, I believe that webscraping is much harder to do in C++, if not impossible, although this is from very vague recollections.
(In reply to comment #11)
> If this were to be structured as an extension, it would be a binary component
> that would need to be recompiled anytime nsIMsgFolder or nsIMsgIncomingServer
> were changed--which is about once a month, according to hg log. They would also
> have to be prepared on the three distributions, which many would-be extension
> developers probably do not have access to.

Perhaps periodically-initiated try builds would then be an acceptable mechanism for this functionality to prove itself.

> In terms of what can or can't be done in C++, I believe that webscraping is
> much harder to do in C++, if not impossible, although this is from very vague
> recollections.

I'm not making an argument that development in C++ is fun or that we should encourage new account development in C++.  My concern is primarily a cost/benefit concern over attempting to review and maintain this non-trivial set of code now without a more compelling use-case.  Additionally, having participated in the review of storage code dealing with a lot of the same JS magic issues with surprising edge cases, it is easier to be confident about the code the more use it has seen.

I completely agree with the end goal of enabling the authoring of new message sources without resorting to C++ development.  Since there is also a gloda-centric path to this goal which is where the mozilla messaging manpower is currently going on said goal, it seems wise to hold off on the costs (review/maintenance) of this until there is an immediate benefit.

My concern is that we spend a lot of effort to review and land this, some new and disturbing bugs turn up into RSS that we need to track down, but no new message source ever shows up that uses this mechanism.

I should be clear that I think there is great potential for this, especially in the short term since it enables all of the current UI to be aware of new message sources (whereas the gloda solution depends on other UI enhancements).  But I think that potential needs to be realized with non-trivial benefit before we do that.  This is especially true since the RSS example is replacing an existing quasi-supported account type and the introduction of completely new account types could reveal serious issues where the existing legacy-ish code's assumptions about local mail folders/RSS folders starts falling down.  (This could be mitigated in cases where the message sources are basically still e-mail; mail archives seem like they would fit in a straightforward fashion, for example, while twitter might be a bit more difficult.)
Found a small Typo in Patch 422044:
In nsIMsgProtocolInfo.idl the attribute canLoginAtStartUp is defined with a capital Up, in nsRssImpl.js its used as canLoginAtStartup with small up.
Comment on attachment 422043 [details] [diff] [review]
JSExtends library, version 1

Similar to previous discussions on this bug, I think this needs to be proven as an extension with some other users before we can look to reviewing and landing this. It has potential but the risk seems too high at the moment.

I'm quite sure that the try server could be adjusted/poked to build extensions and package them - which would greatly ease the building of an extension on all three platforms - and I'm happy to work with you on this if that's what you want to do.
Attachment #422043 - Flags: superreview?(bugzilla) → superreview-
Attachment #422043 - Flags: review?(jorendorff)
Attachment #422044 - Flags: superreview?(dmose)
Attachment #422044 - Flags: review?(bienvenu)
(In reply to comment #14)
> Similar to previous discussions on this bug, I think this needs to be proven as
> an extension with some other users before we can look to reviewing and landing
> this. It has potential but the risk seems too high at the moment.

Ironically the target audience of this patch is probably unwilling or unable to build the binary components of this patch which are necessary to test it. Which I believe is your next point...

> I'm quite sure that the try server could be adjusted/poked to build extensions
> and package them - which would greatly ease the building of an extension on all
> three platforms - and I'm happy to work with you on this if that's what you
> want to do.

You're suggesting that the try server would build these components/modules into an extension that an extension author could require be installed before their extension (i.e. to ensure the modules are loaded). I would certainly be interested in using this (this would also solve half of the first point you said -- having users).
While attempting to patch comm-central I ran into a few conflicts.  This patch is against the current state of comm-central.
(In reply to comment #16)
> Created an attachment (id=443792) [details]
> Unbitrotted version of JSExtends library
> 
> While attempting to patch comm-central I ran into a few conflicts.  This patch
> is against the current state of comm-central.

Don't forget to request review and sr when your patch is ready !
(In reply to comment #17)
> Don't forget to request review and sr when your patch is ready !

The previous comments requesting useful extensions before review is requested still stand in the case of this bug...
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.