430716 - Develop a scriptable, plug-able protocol layer sample (Experimental)

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Emre Birol]

Develop a plug-able, scriptable "protocol layer sample" to allow people to understand and extend the present protocol support in TB.

Currently most of the protocols (Imap, pop3, nttp, rss) consist of the following components:

o ns<protocol>Service: General service for the protocol
o ns<protocol>Protocol: Parser, and session manager for the protocol
o ns<protocol>IncomingServer: nsIIncomingServer implementation for the protocol
o ns<protocol>Url: Necko handler of this protocol specific URLs.
o ns<protocol>Folder: nsIIMsgFolder implementation of the protocol

The sample is going to contain a simple implementation of these components to show how to extend TB for other protocols.

Primary Goals:
1) Making the protocol layer more accessible to contributors
2) Understanding and communicating the internals of the protocol architecture

Secondary Goals:
1) Documenting the architecture

Comment 1

[Joshua Cranmer [:jcranmer]]

Marking as blocking the pluggability bug.

I've done some work on this, and I hope to provide JS forms of the key classes in mailnews/base/utils/ as part of the work.

Comment 2

[Kent James (:rkent)]

Should this bug be the master bug for the planned SkinkGlue merge in Thunderbird 38, or is there another better one? For now I'll use it.

Comment 4

[Kent James (:rkent)]

Attachment: Part 1, use mailnews url override as initial conceptual demo

Here is a full implementation of a scriptable, overridable mailnews url. The url has enough complexity in interfaces to demo the override technology, but does not have any difficult questions of deciding base functionality.

Comment 5

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8651421 [details] [diff] [review]
Part 1, use mailnews url override as initial conceptual demo

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

About the file hierarchy: I'd rather this sit in mailnews/ than mailnews/extensions/; the latter's grab-bag nature sits uneasily with me. I'm also unsure that the public/src distinction is actually useful here.

Most of the comments I'm giving at this point are minor stylistic comments. What this patch needs before I want to really review the technical merit is a detailed document to be checked into the tree that covers:
1. How to add a new C++ interface to this map.
2. How to use this in JS client code.
3. How this all works under the hood on a technical level.

It's not that I can't reverse-engineer these technical merits from the code, but that this stuff is sufficiently black magic that it's not going to be well-understood by anyone but you and me without such a document, and I think that would be extremely detrimental to the future of TB.

One technical concern that worries me heavily when I read this code is that there is a lot of boilerplate, and I think that such boilerplate should be reduced, perhaps even by generating the C++ files at build-time with python scripts if need be.

On the JS side, I suspect we could simplify a lot of the code if we use JS classes, since we support at least extends and super.foo() (we don't support super() in constructors yet, see bug 1141863). Some example play code:

class A { constructor() {} foo() { console.log(this.baz()); } baz() { return "A" } }
class B extends A { constructor() {} foo() { console.log("Ha ha, this is B"); } baz() { return "B" } bar() { return super.foo(); } }
var x = new B();
x.bar()

=> B

This does suggest that it's possible to get the super/this binding issues resolved with substantially less boilerplate on either side, but a lot rides on how xpconnect plays with all of this code, which is investigation that I (or you, if you feel up to it) need to spend a few hours doing.

::: mailnews/extensions/jsaccount/public/JsAccount.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Contains the definitions of contract IDs for modules in JaAccounts.
> +

If the only purpose of this header is defining CIDs and contract IDs, then the naming convention for these files is fooCID.h in mailnews, and I don't see much reason to change that. Such CID files are also traditionally exported.

::: mailnews/extensions/jsaccount/public/msgIOverride.idl
@@ +17,5 @@
> +interface msgIOverride : nsISupports
> +{
> +  /**
> +   Specify an existing base class method that will be overridden using a
> +   javascript implementation.

Some style nits:
/**
 * Foo bar biz bar.
 *
 * @param aName biz bar baz foo.
 */
is preferable for Doxygen-style documentation.

Also, JavaScript or JS is preferable to javascript.

::: mailnews/extensions/jsaccount/src/JaModule.cpp
@@ +5,5 @@
> +#include "mozilla/ModuleUtils.h"
> +#include "../public/JsAccount.h"
> +
> +#include "JaUrl.h"
> +

I also see no reason not to put the XPCOM binary component cruft in nsMailModule.cpp with everyone else.

::: mailnews/extensions/jsaccount/src/JaUrl.h
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> + 

EWHITESPACE (also some floating around in most of the .js files)

@@ +11,5 @@
> +#include "msgIOverride.h"
> +#include "nsAutoPtr.h"
> +#include "nsIMsgFolder.h"
> +#include "nsIFile.h"
> +#include "nsDataHashtable.h"

This list of includes really should be sorted.

::: mailnews/extensions/jsaccount/test/unit/test_componentsExist.js
@@ +21,5 @@
> +
> +function run_test()
> +{
> +  var test;
> +  while (test = tests.shift(), !!test) {

How about:
for (let [contract, interface] of tests) {
}

People do cargo-cult test code a lot, so it's a good idea to write good idiomatic code here when you're starting from scratch.

Comment 6

[Joshua Cranmer [:jcranmer]]

Attachment: Cleaner version on the JS side

Here's some code I wrote up that makes the interface much easier to use from JS. By "much easier," I mean "add a call to a constructor and a prototype of the prototype" and that's all you need to do to make it work--no manual work of specifying the method overrides, no playing games with factories, no messing around with setting up the objects. It's all hidden away in an ugly but somewhat-commented JSM.

The downside is that the magic is somewhat brittle: the use of JSMs means that we have some cross-origin wrappers to worry about, and that means that certain operations (like the magic QI) need to happen in very precise orders or things will blow up.

The prototype magic I think I can replace with a proxy, but that does rely on getting precisely the right object set as this, and the documentation isn't clear on which object is this in the proxy. I'm also not 100% sure that the proxy code is correct (this translation may be needed, as we have three objects claiming to be "this" floating around o_O).

Comment 7

[Kent James (:rkent)]

Comment on attachment 8652164 [details] [diff] [review]
Cleaner version on the JS side

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

Really clever stuff! One little comment so far since I killed hours on this previously.

::: mailnews/extensions/jsaccount/src/JSAccountUtils.jsm
@@ +54,5 @@
> +
> +    // Store the C++ object into the current object. We actually don't need to
> +    // do this, strictly speaking, but some consumers may find this an easier
> +    // API to use if they want to avoid using ES6 classes.
> +    this.cppObject = cppObject;

This line, storing the cppObject into this, I spent about 8 hours yesterday figuring out was preventing the release of objects. I had to check twice to make sure that I did not accidentally include it in submitted code! So with this line, objects are never released and this all leaks (at least in my original version)

Comment 8

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #7)
> This line, storing the cppObject into this, I spent about 8 hours yesterday
> figuring out was preventing the release of objects. I had to check twice to
> make sure that I did not accidentally include it in submitted code! So with
> this line, objects are never released and this all leaks (at least in my
> original version)

We probably need to cycle-collect the C++ and JS object.

Comment 9

[Kent James (:rkent)]

Comment on attachment 8652164 [details] [diff] [review]
Cleaner version on the JS side

What this is missing is the ability to define additional interfaces on the customized objects. I gave a simplified test example (because I was not expecting you to try to "simplify" it to optimize for this case), but typically the new object will need a custom interface, the equivalent of nsIImapUrl.idl.  Could you define an additional interface on the test object, and show how you would adopt your system to handle that?

Comment 10

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #9)
> Comment on attachment 8652164 [details] [diff] [review]
> Cleaner version on the JS side
> 
> What this is missing is the ability to define additional interfaces on the
> customized objects. I gave a simplified test example (because I was not
> expecting you to try to "simplify" it to optimize for this case), but
> typically the new object will need a custom interface, the equivalent of
> nsIImapUrl.idl.  Could you define an additional interface on the test
> object, and show how you would adopt your system to handle that?

QueryInterface(iid) {
  if ([Ci.nsIImapUrl].some(iface => iface.equals(iid))
    return this;
  return super.QueryInterface(iid); (or this.parent, as desired).
}

That should do the trick, although I haven't tried it.

Comment 11

[Kent James (:rkent)]

Attachment: My abandoned attempt to make this work

I've now spent about 15 hours playing with various aspects of jcranmer's patch, trying to make it work. I'm going to abandon that approach, and mostly go back to what I proposed originally. Here's the state it is in as I abandon it.

Overall I find the multiple complex levels of abstraction very hard to work with and understand, on top of what is already a difficult patch. No doubt you can find errors in my abandoned work here and perhaps get it to work. But there are additional obstacles down the road when you try to use this that will make it hard later as well. Remember I've got a few years of experience with this technology now.

I think that I can find a way to use the automated detection of overridden functions, but I'm going to go back to having the split base/overridden js classes. I need to have code that I can understand and work with if I am going to be responsible for this. Bleeding edge, abstract javascript is not necessary to make this work, and is not the approach that I prefer.

Comment 12

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #11)
> Created attachment 8658468 [details] [diff] [review]
> My abandoned attempt to make this work
> 
> I've now spent about 15 hours playing with various aspects of jcranmer's
> patch, trying to make it work. I'm going to abandon that approach, and
> mostly go back to what I proposed originally. Here's the state it is in as I
> abandon it.
> 
> Overall I find the multiple complex levels of abstraction very hard to work
> with and understand, on top of what is already a difficult patch. No doubt
> you can find errors in my abandoned work here and perhaps get it to work.
> But there are additional obstacles down the road when you try to use this
> that will make it hard later as well. Remember I've got a few years of
> experience with this technology now.

While it's somewhat unfortunate, I do understand that reflection is very hard to wrap your head around (I think there's around 6-10 different objects that can claim to be "this" at various stages). The way XPConnect works also makes this extremely annoying, since it's not really designed to work properly with JS models of reference like the WebIDL bindings are (in fact, IIRC, there actually is a bug to make all of this stuff work with WebIDL).

Nonetheless, I have observed sshagarwal struggling to use this code properly, and I rather suspect this is going to end up the sort of code that is poorly understood by anyone other than the original author.

> I think that I can find a way to use the automated detection of overridden
> functions, but I'm going to go back to having the split base/overridden js
> classes. I need to have code that I can understand and work with if I am
> going to be responsible for this. Bleeding edge, abstract javascript is not
> necessary to make this work, and is not the approach that I prefer.

I think I would like to have the following requirements:
1. Automatic override detection
2. Much simpler factory registration
3. Clear, concise rules on how to handle cases like returning "new CustomUrl()" versus using Cc.createInstance or xpcom weirdness like wrappedJSObject.

Comment 13

[Kent James (:rkent)]

Attachment: Combo of JC and RKJ code

This is my latest incarnation. This compiles, and the tests pass.

While doing this, the goal sort of shifted. Originally I was totally focused on new account types, but now I want to have two goals, still now accounts but also allow the incremental replacement of C++ base methods with JS methods. To make that work, we would need to insert the delegate class (here JaDelegateUrl) between the base C++ class and the specialized C++ class.

URL probably isn't the best example of this, but that is where we started so let's finish here.

I believe that I have implemented most of your suggestions, but it really is critical to keep C++ XPCOM objects off of the JS XPCOM object prototype path. That is much clearer here, since the actual C++ XPCOM object is a pure delegator.

Exceptions:

"3. Clear, concise rules on how to handle cases like returning "new CustomUrl()" versus using Cc.createInstance or xpcom weirdness like wrappedJSObject."

That actually is a good question. For now, the answer is that you must use the XPCOM object on the javascript side, that is Cc[].createInstance(). But in the long run that is not efficient. I think that we should implement a JS delegator, similar to the C++ delegator, that would serve as the base class in C++ implementations. But I think that can be delayed until later.

The demo code here in the tests is actually intended as a demo of replacing the C++ base object with a JS implementation, rather than a new account. I need to add another test layer that demos how you would create a custom URL for a custom account type, that derives from the JaURL object that is a partially-JS base msgUrl object.

I also have not moved the code away from /extension, or added the modules to the main module yet as you have suggested.

Comment 14

[Kent James (:rkent)]

Another issue that I still need to address is how to add a custom interface for a new account type to overridden code. You'll see some unfinished pieces of that in the unused test interface in this code (this would be the equivalent of nsIIMAPIncomingServer.idl).

What I am leaning toward doing is to not attempt to make such new interfaces real XPCOM interfaces on the onject (that, following rules such that any implementation of the object can QI to the new interface, and back to others, from the same XPCOM object) but instead implement nsIInterfaceRequestor to return pointers to the new interface implementations. That was I can avoid a lot of work trying to get the C++ side to easily recognize and use the new interfaces. It would be much more common to need these on the JS side, but you will need them. (So a JS equivalent of nsIMAPFolder would probably get a server XPCOM object of type nsIMsgDBFolder, and need some way to get the nsIIMAPIncomingServer object).

The hope is that nsIInterfaceRequestor will be able to return a pure JS XPCOM version of these interface pointers, without having to worry about the C++ issues. For new account types, it would be rare (or never) that the existing C++ code would need to know of these new interfaces. It might be an issue during conversion though.

At some point I would like to get some pieces of this landed, even if we leave some issues open.

Comment 15

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8672327 [details] [diff] [review]
Combo of JC and RKJ code

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

You're missing msgJsAccountCID.h.

The more I think about, the more scared I am that we're doing all of this magic without using cycle collection. I know njn has expressed in the past alarm that the mailnews core code has lots of cycles that rely on being manually broken, and this code makes it really easy to accidentally create a cycle (if the JS object ever contains a strong reference to the cpp delegate object, which I think it might need to, particularly if you start wanting to have half-and-half implementations). So I think we should bite the bullet and make the delegate classes cycle-collected.

I've still not finished the review, but here's some stuff for you to work on.

::: mailnews/extensions/jsaccount/moz.build
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public

Nit: this is missing the vim modelines typical of moz.build files (× a few).

::: mailnews/extensions/jsaccount/public/msgIDelegateList.idl
@@ +8,5 @@
> + * This interface provides a list of methods that should be delegated to
> + * a JsObject rather than a C++ XPCOM base object in JsAccount classes.
> + */
> +
> +[scriptable, uuid(627D3A34-F8A3-40eb-91FE-E413D6638D27)]

This should be builtinobject, should it not?

::: mailnews/extensions/jsaccount/public/msgIOverride.idl
@@ +24,5 @@
> +   *
> +   * @param methodsToDelegate    A list of methods in the C++ base class that
> +   *                             will be delegated JS delegate.
> +  */
> +  attribute msgIDelegateList methodsToDelegate;

So I'm wondering why you're having the model here being that the JS user creates this object and sets this method rather than having the Delegate object construct an empty one that is retrieved for modification?

::: mailnews/extensions/jsaccount/readme.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <meta content="text/html; charset=windows-1252" http-equiv="content-type">

The modern way of writing this is <meta charset="UTF-8">.

@@ +37,5 @@
> +      basic method of using a base class extended for specific types is also used
> +      in other ways in mailnews code, including for addressbook types and views.
> +      The technology may also be applied to those other object types as well.</p>
> +    <h2>Role of JsAccount</h2>
> +    <p>The JavaScript class system works very differently than the C++ system,

Pedantic mode enabled:

It's not actually the JS prototype-based OOP that's the problem here. It's xpconnect not really following the prototype system. I'm pretty sure that a better C++-to-JS reflection mechanism (i.e., WebIDL) would actually allow subclassing of C++ classes in JS "for free" (at least, as far as JS code is concerned).

::: mailnews/extensions/jsaccount/src/DelegateList.cpp
@@ +15,5 @@
> +
> +NS_IMETHODIMP DelegateList::Add(const char* aMethodName)
> +{
> +  // __FUNCTION__ is the undecorated function name in gcc, but decorated in
> +  // Windows. __func__ will resolve this when supported in VS 2015.

Hmm. I wonder what clang-cl does here.

@@ +17,5 @@
> +{
> +  // __FUNCTION__ is the undecorated function name in gcc, but decorated in
> +  // Windows. __func__ will resolve this when supported in VS 2015.
> +  nsCString prettyFunction;
> +#if defined (XP_WIN32)

Wouldn't this break in Win64 builds as well?

::: mailnews/extensions/jsaccount/src/DelegateList.h
@@ +18,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_MSGIDELEGATELIST
> +  DelegateList() { }
> +  nsDataHashtable<nsCStringHashKey, bool> mMethods;

This is perhaps being a bit too premature optimization, but hashtables tend to have worse performance than arrays for small datasets (I believe n ~= 16 is the usual break-even point). I rather suspect that you'd get better performance by using an nsTArray<nsCString> and using .Contains() on it.

@@ +22,5 @@
> +  nsDataHashtable<nsCStringHashKey, bool> mMethods;
> +
> +protected:
> +  virtual ~DelegateList() { }
> +};

I feel like you could make some more C++ code simply if you added a method

template <typename T>
T *ChooseDelegate(T *original, T *delegate, const nsLiteralCString &methodName) {
  if (!delegate || !mMethods->Contains(methodName))
    return original;
  return delegate;
}

If you also ensure that the delegate url always has a non-null delegate list, you turn the forward message call into:
NS_FORWARD_NSIMSGMESSAGEURL(mDelegateList->ChooseDelegate(mParent, mJSIMsgMessageUrl, __FUNCTION__))

::: mailnews/extensions/jsaccount/src/JSAccountUtils.jsm
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> +  This file implements helper methods to make the transition of base mailnews

Nits be nits, but the style is definitely
/**
 *
 *
 */

for these kinds of comments. You've managed to pepper three different styles of this kind of comment in the same patch ;-)

@@ +8,5 @@
> +  versions of those accounts using only JS XPCOM implementations.
> +
> +  In C++ land, the XPCOM component is a generic C++ class that does nothing
> +  but delegate any calls to interfaces known in C++ to either the generic
> +  C++ implementation (such as nsMsgIncomingServer.cpp) or a JavaScript

Nit: s/.cpp//

@@ +103,5 @@
> +
> +    if (jsDelegate.delegateList) {
> +      cppObject.methodsToDelegate = jsDelegate.delegateList;
> +    }
> +    else {

I'm quite certain that this bracing style isn't the mailnews JS style ;-)

::: mailnews/extensions/jsaccount/src/JaModule.cpp
@@ +8,5 @@
> +#include "JaUrl.h"
> +
> +using namespace mozilla::mailnews;
> +
> +NS_GENERIC_FACTORY_CONSTRUCTOR(JaDelegateUrl)

Again, you should just add all of this stuff to mailnews/build/nsMailModule.cpp instead of making a new file for this.

::: mailnews/extensions/jsaccount/src/JaUrl.h
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> + 

EWHITESPACE

@@ +57,5 @@
> +  NS_DECL_MSGIOVERRIDE
> +  NS_FORWARD_NSIMSGMAILNEWSURL((
> +    pIMsgMailNewsUrl = !mJsIMsgMailNewsUrl || !mMethods ||
> +                       !mMethods->Contains(nsLiteralCString(__FUNCTION__)) ?
> +                         mParentIMsgMailNewsUrl : mJsIMsgMailNewsUrl)->)

Should you be able to use mParent here instead of QI'ing to all of the different interfaces? I see why you have separate mJS objects, but the mParent is confusing.

@@ +74,5 @@
> +
> +  JaDelegateUrl(void);
> +
> +private:
> +  virtual ~JaDelegateUrl(void) {

(void) is superfluous in C++; just use ().

::: mailnews/extensions/jsaccount/test/moz.build
@@ +8,5 @@
> +]
> +
> +XPIDL_MODULE = 'testJsAccount'
> +
> +# Don't add our test-only .xpt files to the normal manifests.

The usual thing I've seen for test files like this is to reference the code in support-files in the xpcshell.ini.

@@ +12,5 @@
> +# Don't add our test-only .xpt files to the normal manifests.
> +XPIDL_NO_MANIFEST = True
> +
> +EXTRA_COMPONENTS = [
> +  'unit/resources/TestComponents.manifest',

This is definitely going to be embedded in the final manifest the way you've written it (another reason why the supports-files route is usually traversed).

::: mailnews/extensions/jsaccount/test/unit/resources/TestComponents.manifest
@@ +1,3 @@
> +component {F6402536-85E5-4d0d-BA32-6414556F87E0} TestJaUrl.js
> +contract @mozilla.org/jsaccount/testjaurl;1 {F6402536-85E5-4d0d-BA32-6414556F87E0}
> +interfaces testJsAccount.xpt

I suspect this requires the xpt file to be installed? I don't see this working properly on packaged tests.

/me points to the js/xpconnect/tests/unit logic.

Comment 16

[Joshua Cranmer [:jcranmer]]

(In reply to Joshua Cranmer [:jcranmer] from comment #15)
> ::: mailnews/extensions/jsaccount/src/DelegateList.cpp
> @@ +15,5 @@
> > +
> > +NS_IMETHODIMP DelegateList::Add(const char* aMethodName)
> > +{
> > +  // __FUNCTION__ is the undecorated function name in gcc, but decorated in
> > +  // Windows. __func__ will resolve this when supported in VS 2015.
> 
> Hmm. I wonder what clang-cl does here.

clang-cl does the gcc thing and returns the undecorated name, according to a test :froydnj ran...

> @@ +17,5 @@
> > +{
> > +  // __FUNCTION__ is the undecorated function name in gcc, but decorated in
> > +  // Windows. __func__ will resolve this when supported in VS 2015.
> > +  nsCString prettyFunction;
> > +#if defined (XP_WIN32)
> 
> Wouldn't this break in Win64 builds as well?

So you probably want _MSC_VER (or whatever the spelling is?) here instead of XP_WIN32...

Comment 17

[Kent James (:rkent)]

Partial response to comment 16:

::: mailnews/extensions/jsaccount/public/msgIOverride.idl
@@ +24,5 @@
> +   *
> +   * @param methodsToDelegate    A list of methods in the C++ base class that
> +   *                             will be delegated JS delegate.
> +  */
> +  attribute msgIDelegateList methodsToDelegate;

"So I'm wondering why you're having the model here being that the JS user creates this object and sets this method rather than having the Delegate object construct an empty one that is retrieved for modification?"

I'm not sure what you are saying. The model IS that the Delegate object JaCppUrlDelegator creates an empty one that is retrieved for modification. I suppose other methods are possible, but the advantage this is that JaCppUrlDelegator knows the correct Windows prefix needed:

  mDelegateList = new DelegateList("mozilla::mailnews::JaCppUrlDelegator::");

so it creates the DelegateList properly initialized. The JS implementation does not need to know about the Windows prefix (which will go away eventually).

Comment 18

[Kent James (:rkent)]

More comments on review:

"@@ +8,5 @@
> +  versions of those accounts using only JS XPCOM implementations.
> +
> +  In C++ land, the XPCOM component is a generic C++ class that does nothing
> +  but delegate any calls to interfaces known in C++ to either the generic
> +  C++ implementation (such as nsMsgIncomingServer.cpp) or a JavaScript

Nit: s/.cpp//"

Huh? I really did mean nsMsgIncomingServer.cpp which is the implementation.

"@@ +103,5 @@
> +
> +    if (jsDelegate.delegateList) {
> +      cppObject.methodsToDelegate = jsDelegate.delegateList;
> +    }
> +    else {

I'm quite certain that this bracing style isn't the mailnews JS style ;-)"

I'm not sure exactly what you are objecting to, and I do see this style. What are is the issue? Braces around a single-line call? Braces on the same line as if? New line for else instead of } else { ?

Comment 19

[Kent James (:rkent)]

As I have added a few more methods, I've come to realize that the simplifications that I have attempted here relative to SkinkGlue are not going to actually work, but instead I need to go back to using the "class Super" architecture from SkinkGlue.

Here's the issue. In the patches so far here, an XPCOM method call is delegated to one of two classes, either a JS override class, or a class that represents an implementation of a base object, like nsIMsgIncomingServer.cpp

Suppose that you start a call using that C++ object, say to server->GetRootFolder(). If this has no JS override, the current patches will delegate that to a C++ object that is essentially an instance of nsMsgIncomingServer.cpp  Once you get into the C++ code, internal C++ calls in the server such as GetServerURI() will not see the delegator class, only the nsMsgIncomingServer class. It has no way to know that the delegator class wants to override certain methods, so the internal C++ calls are sent to the non-overridden methods in nsMsgIncomingServer.

SkinkGlue gets around this by definine a Super() class that can be represented by a pointer in the forward call, but internally does a second forward like this:

NS_FORWARD_NSIABDIRECTORY(mFakeThis->msqSgAbDirectory::)

where mFakeThis is a pointer to the delegator object. That way the class that is seen in the C++ is the delegator class.

I need to see if there is some way that I can do the same thing without having to create this artificial Super() class, but that method will work.

Comment 20

[Kent James (:rkent)]

Attachment: Part 1: URL (and general demo)

Here's the initial URL patch with all of the needed changes, including things I discovered with later patches.

As it turns out, I needed to add the Super class so that I would have something to pass to JS when it requested the parent. Since I have to have that class, I used it as part of the standard delegation as well.

I experimented with cycle collection enough to know that I could add it, but I would have to modify the base classes as well. We'll do that as followup patches, it does not seem to affect the fundamental concepts.

Comment 21

[Kent James (:rkent)]

Attachment: Part 2, AbDirectory

The AbDirectory item is outside of the mainstream of mailnews account classes, but it is a very simple implementation. I use the SkinkGlue equivalent as the precursor for EWS contact management. I did this second because it is a very straightforward class, and let me test some of the macros for creating these things.

I think we should include it because it is easy and low risk, but this is not strictly necessary, since AFAIK there are no major obstacles to use a full JS implementation of an AB directory.

Comment 22

[Kent James (:rkent)]

Attachment: Part 3, IncomingServer

The server is a key part, but was surprisingly straightforward (particularly considering some of the issues in Send and Compose that will be addressed in later patches.

The next patch will include the folder, and once both folder and server are there, we can start using the account manager to create things. The tests in Part 4 will exercise the server a little.

In fact, it was there that the issue surfaced that showed me why the Super class is needed (as has been always used in SkinkGlue). The issue is, whenever an incoming server method calls another incoming server method, you must make sure that you call the virtual (possible overridden) method, and not the nsMsgIncomingServer implementation. So the underlying object must be the delegator class, meaning the call has to look like:

(ptrToDelegator)->(referenceToBase)::

which is done in the Super class.

Comment 23

[Kent James (:rkent)]

Attachment: Part 4, MsgFolder

Now things start to get more complex, as we need to deal with various underlying issues that require design modification in the core code. Many of these are needed to get rid of code that typically has "if (imap) then ..." because we have to generalize to account type that are not defined.

First, we separate the localStoreType (used to create URLs to access messages) and the localDatabaseType (which most of the time will be "mailbox" to use the default database class.

Within the JaBaseCppMsgFolder, there is a default implementation of GetDatabase. This uses (and creates if needed) a dummy file to represent a folder, as the conflation of msgFolder name with localFolder name in mailnews code requires such as thing (that would be the mbox file traditionally). Many new account types will not use mbox, but they still need some file there to represent folder structure. There are other issues dealt with there copied from SkinkGlue.

Also, I add more extensive tests here since there is enough created now to allow it.

Comment 24

[Kent James (:rkent)]

Attachment: Part 5, Compose

The Compose changes were not as difficult as I expected. The issue is that a single compose must be used for everything (since you can easily switch the destination account of a message), but you need to be able to switch to a non-SMTP send process, which has zero support in the current code.

This is done in this implementation by allowing the former _SendMsg to be an XPCOM method that can be overridden. That is how current SkinkGlue/ExQuilla solves that problem, but it is less that ideal. An alternate was is also added in the next patch for Send, which is to add an observer at the key moment so that an alternate nsISend implementation can be inserted.

Comment 25

[Kent James (:rkent)]

Attachment: Part 6, Send

A number of changes are added to nsIMsgSend which are things learned from experience in SkingGlue/ExQuilla, including an XPCOM method to access the attachment handler, which any non-SMTP JS sender needs.

The idea is that the existing code snarfs the attachments, then you override GatherMimeAttachments as the access point to actually complete the send operation after all of the attachments are sitting in accessible files.

Comment 26

[Kent James (:rkent)]

I believe that this completes the core pieces needed for JsAccount to be feasible. What issues remain?

Obviously I need to do some actual implementations of this. I should be able to convert the existing ExQuilla code to using the AbDirectory, Send, and Compose classes, as those already use Js backends in ExQuilla. Unfortunately that code, while viewable, is not open licensed. But it should let me make sure that the JsAccount implementation is adequate in a real, complex implementation.

I also intend to do a series of blog postings on how to use this, with some very simple example account types.

The intention is also to land this for TB45.

I would like though the implementation to be considered experimental throughout TB45, but allow more extensive updates than would be permitted for typical released code for this relatively self-contained set of classes. At the most a handful of people might try to use it, and they should be warned to keep in contact with use to be warned of any potentially breaking changes. I realize this is a little different than our normal patter. But this is code intended for extensions, and it is really not practical to develop extensions based on this if we can't fix critical issues that are discovered in less than 9 months.

Comment 27

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #18)
> More comments on review:
> 
> "@@ +8,5 @@
> > +  versions of those accounts using only JS XPCOM implementations.
> > +
> > +  In C++ land, the XPCOM component is a generic C++ class that does nothing
> > +  but delegate any calls to interfaces known in C++ to either the generic
> > +  C++ implementation (such as nsMsgIncomingServer.cpp) or a JavaScript
> 
> Nit: s/.cpp//"
> 
> Huh? I really did mean nsMsgIncomingServer.cpp which is the implementation.

The generic implementation of nsIMsgIncomingServer is nsMsgIncomingServer, not nsMsgIncomingServer.cpp...


> I'm not sure exactly what you are objecting to, and I do see this style.
> What are is the issue? Braces around a single-line call? Braces on the same
> line as if? New line for else instead of } else { ?

It's the } else { in particular.

(More comments to follow).

Comment 28

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #26)
> I believe that this completes the core pieces needed for JsAccount to be
> feasible. What issues remain?

I've had problems in the past with implementing nsIMsgMessageService correctly. In particular, this line in nsNntpService (and probably the others, but it's been several years since I poked at this in detail):
    rv = docShell->LoadURI(aUrl, loadInfo, nsIWebNavigation::LOAD_FLAGS_NONE, false);

That method is [noscript] (although I'm not entirely sure why).

As a result, it was difficult or perhaps even impossible to implement an account that didn't act like a mailbox service and kept all of its folders remote.

The other two things that may need this magic are nsMsgDatabase and nsMsgProtocol, but there's very little reason to override the former so long as nsMsgDatabase itself is constructable, and the latter interface would provide little useful to JS once I've got the email-socket stuff fleshed out.

> Obviously I need to do some actual implementations of this.

The RSS and Movemail incoming servers could easily be moved into JS once you think it's stable. The RSS service as well, too, given that 90% of the logic is all in JS anyways. (Note that the movemail service can't be written in JS easily, since it's using locking primitives not known to OS.File or nsIFile.

As a forward path, all the URLs should be easy to convert, with the important caveat that IMAP relies on them being threadsafe, which JS ain't. Servers are probably the next easiest to convert (IMAP uses a sync helper, although I think import doesn't, although import really needs to be fixed in its thread-safety ignorance), with folders being the hardest. For everything else, blowing up the current code is likely a more fruitful endeavor than porting everything.

Comment 29

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8700358 [details] [diff] [review]
Part 1: URL (and general demo)

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

I see two tests that need to be present that aren't:

1. The effects of returning the C++ Delegator instance versus the JS instance.
2. A C++ method calling a virtual method that invokes the JS method.

::: mailnews/jsaccount/public/msgIOverride.idl
@@ +25,5 @@
> +   * be created once for all instances, then set when the instance is created.
> +   *
> +   * @param methodsToDelegate    A list of methods in the C++ base class that
> +   *                             will be delegated to the JS delegate.
> +  */

Nit: misaligned *

@@ +26,5 @@
> +   *
> +   * @param methodsToDelegate    A list of methods in the C++ base class that
> +   *                             will be delegated to the JS delegate.
> +  */
> +  attribute msgIDelegateList methodsToDelegate;

It wasn't clear to me at first why you had this be settable, which suggests that you should explain why in the documentation.

@@ +39,5 @@
> +   * C++ base class used to implement default functionality. This is used when
> +   * JavaScript methods want to call the base class default action, bypassing a
> +   * possible JS override.
> +   */
> +  readonly attribute nsISupports parent;

This is a really bad name. nsIMsgFolder has a parent attribute, and I don't know what xpconnect will do here if you QI'd to conflicting names.

::: mailnews/jsaccount/public/msgJsAccountCID.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Contains the definitions of contract IDs for modules in JaAccounts.

Nit: JsAccounts ?

::: mailnews/jsaccount/readme.html
@@ +30,5 @@
> +      nsMsgIncomingServer.cpp that implements that base interface
> +      nsIMsgIncomingServer.idl. For imap, there is a specific class
> +      nsImapIncomingServer.cpp that inherits from nsMsgIncomingServer.cpp,
> +      overrides some of the methods in nsIMsgIncomingServer.idl, and also
> +      implements an imap-specific interface nsIImapIncomingServer.idl All of

Nit: you're missing a period.

::: mailnews/jsaccount/src/DelegateList.cpp
@@ +14,5 @@
> +namespace mailnews {
> +
> +NS_IMPL_ISUPPORTS(DelegateList, msgIDelegateList)
> +
> +NS_IMETHODIMP DelegateList::Add(const char* aMethodName)

Minor nit: We use T *foo (and T **foo), in mailnews C++ code. The pointer-cuddling style here is all over the place, and I'd like to have at least one subdirectory that actually conforms to a single style, especially since this is all new code.

::: mailnews/jsaccount/src/DelegateList.h
@@ +22,5 @@
> +  NS_DECL_MSGIDELEGATELIST
> +  DelegateList(const char* aWindowsPrefix) :
> +    mPrefix(aWindowsPrefix)
> +  { }
> +  nsDataHashtable<nsCStringHashKey, bool> mMethods;

I'm not going to ask that you change this, but be aware that hashtables are likely to have high-ish overhead if the method list is relatively small, particularly compared to a sorted array.

::: mailnews/jsaccount/src/JSAccountUtils.jsm
@@ +50,5 @@
> +const PREF_LOG_DUMP = PREF_BRANCH_LOG + "dump";
> +
> +// Set default logging levels.
> +const LOG_LEVEL_DEFAULT = "Info"
> +const LOG_DUMP_DEFAULT = true;

I don't think these are sane defaults for shipping in release code.

@@ +61,5 @@
> + *
> + *  @param aProperties   This a a const JS object that describes the specific
> + *                       details of a particular JsAccount XPCOM object:
> + *     {
> + *       delegatorContractID: string contactID used to create the base generic C++

I think you meant baseContractID here?

@@ +66,5 @@
> + *                            object. This object must implement the interfaces in
> + *                            baseInterfaces, plus msgIOverride.
> + *
> + *       baseInterfaces: JS array of interfaces implemented by the base, generic
> + *                       C++ object.

I dislike having this property, but it's also difficult to come up with a simple way to avoid having to do this without making the relevant interfaces support class info support.

@@ +74,5 @@
> + *                           override specific methods in the base, generic C++
> + *                           object.
> + *
> + *       extraInterfaces: JS array of additional interfaces implemented by the
> + *                        component (accessed using getInterface())

You don't also use this property?

@@ +100,5 @@
> +    if (outer != null)
> +      throw Cr.NS_ERROR_NO_AGGREGATION;
> +
> +    // C++ delegator class.
> +    let delegator = Cc[aProperties.baseContractID]

This might be better named something like "baseImpl" or "baseObject" rather than delegator, particularly since it's closer to a delegatee than a delegator.

@@ +120,5 @@
> +    }
> +    if (delegateList instanceof Ci.msgIDelegateList) {
> +      delegator.methodsToDelegate = delegateList;
> +    }
> +    else {

As noted earlier, } else { is the style guideline we use in JS code.

@@ +123,5 @@
> +    }
> +    else {
> +      // Lazily create and populate the list of methods to delegate.
> +      let delegateList = delegator.methodsToDelegate;
> +      Object.keys(delegator).forEach(name => {log.debug("delegator has key " + name);});

These are also inadequate for logging... you might say delegators have various keys, but which one? You'd probably need to have a log before any of the log messages here saying what the object pair relationship being considered is.

@@ +154,5 @@
> +
> +        let upperCaseName = name[0].toUpperCase() + name.substr(1);
> +        if ('value' in jsDescriptor) {
> +          log.info("delegating " + upperCaseName);
> +          delegateList.add(upperCaseName);

It's worth noting that this approach won't work if someone implements a getter with a static property on the prototype (e.g., foo.prototype = { ... foo: 5 }; instead of foo.prototype = { ... get foo() { return 5; } }). However, I don't think there's any properties where this would happen for the main interfaces here; the only one I've been tempted to do it on is nsIMsgProtocolInfo.

(Explicit note: this is not something that I'd like you to try to fix. It's not really something that can easily be fixed :-) ).

::: mailnews/jsaccount/src/JaUrl.cpp
@@ +9,5 @@
> +#include "nsIFile.h"
> +#include "nsIMsgHdr.h"
> +#include "nsIMessenger.h"
> +#include "nsMsgBaseCID.h"
> +#include "nsComponentManagerUtils.h"

Nit: sort all the headers after JaUrl.h in alphabetical order.

@@ +13,5 @@
> +#include "nsComponentManagerUtils.h"
> +
> +namespace mozilla {
> +namespace mailnews {
> +  

Nit: extraneous whitespace

@@ +134,5 @@
> +                  nsIInterfaceRequestor)
> +
> +JaCppUrlDelegator::JaCppUrlDelegator() :
> +  mParent(new Super(this)),
> +  mMethods(nullptr)

These are in a different order from their definition in the class, which causes a warning:
 0:05.93 Warning: -Wreorder in /src/trunk/comm-central/mailnews/jsaccount/src/JaUrl.h: ‘mozilla::mailnews::JaCppUrlDelegator::mParent’ will be initialized after
 0:05.93 /src/trunk/comm-central/mailnews/jsaccount/src/JaUrl.h:111:31: warning: ‘mozilla::mailnews::JaCppUrlDelegator::mParent’ will be initialized after [-Wreorder]
 0:05.93    nsCOMPtr<nsIMsgMailNewsUrl> mParent;

::: mailnews/jsaccount/src/JaUrl.h
@@ +28,5 @@
> +// This class is an XPCOM component, usable in JS, that calls the methods
> +// in the C++ base class (bypassing any JS override).
> +class JaBaseCppUrl : public nsMsgMailNewsUrl,
> +                     public nsIMsgMessageUrl,
> +                     public nsIInterfaceRequestor

I'm curious as to why you're adding nsIInterfaceRequestor here.

::: mailnews/jsaccount/test/unit/resources/testJaBaseUrl.jsm
@@ +22,5 @@
> +// A partial JavaScript implementation of the base server methods.
> +
> +const JaBaseUrlProperties = {
> +  baseContractID:     "@mozilla.org/jacppurldelegator;1",
> +  baseInterfaces:     [ Ci.nsISupports,

You shouldn't ever need to list nsISupports explicitly.

::: mailnews/jsaccount/test/unit/resources/testJaFooUrl.js
@@ +63,5 @@
> +{ get: function() {
> +    return this.parent.QueryInterface(Ci.nsIMsgMailNewsUrl).maxProgress + 1234;
> +  },
> +  set: function(aMaxProgress) {
> +    this.parent.QueryInterface(Ci.nsIMsgMailNewsUrl).maxProgress = aMaxProgress;

Since you did a QI in the constructor, you should need to ever call the QI here...

::: mailnews/jsaccount/test/unit/test_componentsExist.js
@@ +3,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests that the components made available by JaAccount can be created with
> +// each supported interface.

What is the purpose of this test, as in, what plausible change would happen that would cause this test to fail and indicate a failure of code logic, not test logic?

::: mailnews/jsaccount/test/unit/test_fooUrl.js
@@ +49,5 @@
> +  Assert.equal(testFooUrl.jsDelegate.wrappedJSObject._hidden, "IAmHidden");
> +
> +  // Get the extra interface, and test that it works.
> +  Assert.ok(testFooUrl instanceof Ci.nsIInterfaceRequestor);
> +  let testIFooUrl = testFooUrl.getInterface(Ci.msgIFooUrl);

I'm... really wary of using getInterface instead of QI for this sort of stuff, particularly if we want to move existing code from C++ to JS using this stuff. On the other hand, getting QI working "naturally" is really playing with fire.

Comment 30

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8700359 [details] [diff] [review]
Part 2, AbDirectory

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

Since there's no real changes here except adding the interface, there's nothing really of interest to comment here, except to point back to the test_componentsExist.js comment earlier.

Comment 31

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8700366 [details] [diff] [review]
Part 6, Send

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

I'm currently rewriting nsIMsgSend in JS, and considering that the interface is one giant messed-up interface, I'm basically planning on ripping the entire interface and starting from scratch. So the basic advisory for people trying to override the compose stuff is that it will very likely change quite a lot in the near future.

::: mailnews/compose/public/nsIMsgAttachmentHandler.idl
@@ +5,5 @@
> +#include "nsISupports.idl"
> +interface nsIFile;
> +
> +// This interface provides minimal XPCONNECT access to objects of type
> +// nsMsgAttachmentHandler.

We already have way too many bloody interfaces of this kind.

Please add a note saying that this isn't the interface that you're looking to use most of the time.

@@ +19,5 @@
> +
> +  /// The temp file to which we save it.
> +  readonly attribute nsIFile tmpFile;
> +
> +  /// The name for the headers, if different from the URL. 

ETRAILINGWHITESPACE

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +1072,5 @@
>  
>      m_compFields->SetOrganization(organization);
> +
> +    // We need an nsIMsgSend instance to send the message. Allow extensions
> +    // to override the default SMTP sender by observing mail-set-sender.

I don't think this is necessarily the best way to override this... but I can't think of any other easy way to do this pre-send-rewrite.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +4900,5 @@
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  *aAttachment = m_attachments[aIndex];
> +  NS_IF_ADDREF(*aAttachment);
> +  return NS_OK;
> +} 

ETRAILINGWHITESPACE

Comment 32

[Kent James (:rkent)]

(In reply to Joshua Cranmer [:jcranmer] from comment #29)
> Comment on attachment 8700358 [details] [diff] [review]
> Part 1: URL (and general demo)

I've done a pass through the code, implementing the review requests, so I will make comments now where I did not simply follow the request while this is fresh in my mind.

> 
> I see two tests that need to be present that aren't:
> 
> 1. The effects of returning the C++ Delegator instance versus the JS
> instance.

I don't understand this request.

> 2. A C++ method calling a virtual method that invokes the JS method.

This relies on having a C++ call to the relevant method, and I found an acceptable
way to do this with Clone().

> ::: mailnews/jsaccount/public/msgIOverride.idl
> 
> @@ +39,5 @@
> > +   * C++ base class used to implement default functionality. This is used when
> > +   * JavaScript methods want to call the base class default action, bypassing a
> > +   * possible JS override.
> > +   */
> > +  readonly attribute nsISupports parent;
> 
> This is a really bad name. nsIMsgFolder has a parent attribute, and I don't
> know what xpconnect will do here if you QI'd to conflicting names.

OK I renamed this to cppBase.

> ::: mailnews/jsaccount/src/JSAccountUtils.jsm
> @@ +50,5 @@
> > +const PREF_LOG_DUMP = PREF_BRANCH_LOG + "dump";
> > +
> > +// Set default logging levels.
> > +const LOG_LEVEL_DEFAULT = "Info"
> > +const LOG_DUMP_DEFAULT = true;
> 
> I don't think these are sane defaults for shipping in release code.

I changed "Info" to "Error" but then immediately changed it back to "Debug" since I needed it for, well, debugging. I agree it should be "Error" in the final version. But then I would leave
LOG_DUMP_DEFAULT = true;

> @@ +74,5 @@
> > + *                           override specific methods in the base, generic C++
> > + *                           object.
> > + *
> > + *       extraInterfaces: JS array of additional interfaces implemented by the
> > + *                        component (accessed using getInterface())
> 
> You don't also use this property?

It is used in the demo code in testJaFooUrl.js

> 
> @@ +100,5 @@
> > +    if (outer != null)
> > +      throw Cr.NS_ERROR_NO_AGGREGATION;
> > +
> > +    // C++ delegator class.
> > +    let delegator = Cc[aProperties.baseContractID]
> 
> This might be better named something like "baseImpl" or "baseObject" rather
> than delegator, particularly since it's closer to a delegatee than a
> delegator.

I did not change this, because this object is indeed the delegator, that is the thing that delegates to either a C++ or a JS implementation.

> ::: mailnews/jsaccount/src/JaUrl.h
> @@ +28,5 @@
> > +// This class is an XPCOM component, usable in JS, that calls the methods
> > +// in the C++ base class (bypassing any JS override).
> > +class JaBaseCppUrl : public nsMsgMailNewsUrl,
> > +                     public nsIMsgMessageUrl,
> > +                     public nsIInterfaceRequestor
> 
> I'm curious as to why you're adding nsIInterfaceRequestor here.

The issue is that the nsIInterfaceReq
> 
> ::: mailnews/jsaccount/test/unit/resources/testJaBaseUrl.jsm
> @@ +22,5 @@
> > +// A partial JavaScript implementation of the base server methods.
> > +
> > +const JaBaseUrlProperties = {
> > +  baseContractID:     "@mozilla.org/jacppurldelegator;1",
> > +  baseInterfaces:     [ Ci.nsISupports,
> 
> You shouldn't ever need to list nsISupports explicitly.
> 
> ::: mailnews/jsaccount/test/unit/resources/testJaFooUrl.js
> @@ +63,5 @@
> > +{ get: function() {
> > +    return this.parent.QueryInterface(Ci.nsIMsgMailNewsUrl).maxProgress + 1234;
> > +  },
> > +  set: function(aMaxProgress) {
> > +    this.parent.QueryInterface(Ci.nsIMsgMailNewsUrl).maxProgress = aMaxProgress;
> 
> Since you did a QI in the constructor, you should need to ever call the QI
> here...
> 
> ::: mailnews/jsaccount/test/unit/test_componentsExist.js
> @@ +3,5 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +// Tests that the components made available by JaAccount can be created with
> > +// each supported interface.
> 
> What is the purpose of this test, as in, what plausible change would happen
> that would cause this test to fail and indicate a failure of code logic, not
> test logic?
> 
> ::: mailnews/jsaccount/test/unit/test_fooUrl.js
> @@ +49,5 @@
> > +  Assert.equal(testFooUrl.jsDelegate.wrappedJSObject._hidden, "IAmHidden");
> > +
> > +  // Get the extra interface, and test that it works.
> > +  Assert.ok(testFooUrl instanceof Ci.nsIInterfaceRequestor);
> > +  let testIFooUrl = testFooUrl.getInterface(Ci.msgIFooUrl);
> 
> I'm... really wary of using getInterface instead of QI for this sort of
> stuff, particularly if we want to move existing code from C++ to JS using
> this stuff. On the other hand, getting QI working "naturally" is really
> playing with fire.

Comment 33

[Kent James (:rkent)]

(In reply to Joshua Cranmer [:jcranmer] from comment #29)
> Comment on attachment 8700358 [details] [diff] [review]

Continuation of comment 32, something caused it to get posted before I was done.

> @@ +28,5 @@
> > +// This class is an XPCOM component, usable in JS, that calls the methods
> > +// in the C++ base class (bypassing any JS override).
> > +class JaBaseCppUrl : public nsMsgMailNewsUrl,
> > +                     public nsIMsgMessageUrl,
> > +                     public nsIInterfaceRequestor
> 
> I'm curious as to why you're adding nsIInterfaceRequestor here.

The issue here is that you can only access new methods that might be implemented by the JS delegate using the interface requestor, not a QI. In the event that the C++ code at some point needs to access an interface that is only defined in the JS, then you will need to use this. Ideally that would be never, but certainly that is not true in my current ExQuilla code.


> 
> ::: mailnews/jsaccount/test/unit/resources/testJaBaseUrl.jsm
> @@ +22,5 @@
> > +// A partial JavaScript implementation of the base server methods.
> > +
> > +const JaBaseUrlProperties = {
> > +  baseContractID:     "@mozilla.org/jacppurldelegator;1",
> > +  baseInterfaces:     [ Ci.nsISupports,
> 
> You shouldn't ever need to list nsISupports explicitly.

OK I tried removing this, and immediately failed. The use is here, in JSAccountUtils.jsm:

    for (let iface of aProperties.baseInterfaces)
      if (iid.equals(iface)) {
        log.debug("Successfully returning delegator " + delegator);
        return delegator;
      }

That is, you must be able to QI the delegator to nsISupports. I could of course deal with that as a special case here, but why not just leave it the way that I had it?

> 
> ::: mailnews/jsaccount/test/unit/resources/testJaFooUrl.js
> @@ +63,5 @@
> > +{ get: function() {
> > +    return this.parent.QueryInterface(Ci.nsIMsgMailNewsUrl).maxProgress + 1234;
> > +  },
> > +  set: function(aMaxProgress) {
> > +    this.parent.QueryInterface(Ci.nsIMsgMailNewsUrl).maxProgress = aMaxProgress;
> 
> Since you did a QI in the constructor, you should need to ever call the QI
> here...

I changed this as you requested, but I have had problems with this in the past, so I may need to change it back at some point.

> 
> ::: mailnews/jsaccount/test/unit/test_componentsExist.js
> @@ +3,5 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +// Tests that the components made available by JaAccount can be created with
> > +// each supported interface.
> 
> What is the purpose of this test, as in, what plausible change would happen
> that would cause this test to fail and indicate a failure of code logic, not
> test logic?

This test actually is the first one that I always run, and it catches probably half of more of issues after I do a code change. But to answer your question, you would find here for example a missing interface in this list:

NS_IMPL_ISUPPORTS_INHERITED(JaBaseCppUrl, nsMsgMailNewsUrl,
                            nsIMsgMessageUrl,
                            nsIInterfaceRequestor)

These lists are tricky to get correct, and different core methods have lots of different variations in how this must be done. Also, for some of the core interfaces I will not be doing tests showing access to all of the different interfaces, hence a single test that shows that all interfaces QI is useful.

> ::: mailnews/jsaccount/test/unit/test_fooUrl.js
> @@ +49,5 @@
> > +  Assert.equal(testFooUrl.jsDelegate.wrappedJSObject._hidden, "IAmHidden");
> > +
> > +  // Get the extra interface, and test that it works.
> > +  Assert.ok(testFooUrl instanceof Ci.nsIInterfaceRequestor);
> > +  let testIFooUrl = testFooUrl.getInterface(Ci.msgIFooUrl);
> 
> I'm... really wary of using getInterface instead of QI for this sort of
> stuff, particularly if we want to move existing code from C++ to JS using
> this stuff. On the other hand, getting QI working "naturally" is really
> playing with fire.

It also seems to me that this is a classic case of when we need nsIInterfaceRequestor, because there is not a single object that we can QI to different interfaces, violating a core concept of XPCOM QI. So yes it is true that with the existing implementation this is needed, and it would be tricky to come up with an implementation that did not need this.

Comment 34

[Kent James (:rkent)]

Attachment: part 1, URL and general demo (now with CPP as __proto__ to JS)

I believe this addresses the nits, and it also represents some significant changes. This version has been successfully adapted to ExQuilla, so I am more confident that it is usable.

The main difference is that I felt it necessary to construct a prototype for the JS objects that references the C++ base class, hence the JS object is now a full implementation of the object.

The reason this needs to be done rather than just adding another .prototype is that the lowest level object must access a particular instance of cppBase, while the normal JS prototype is independent of the particular instance. So I had to create JS-friendly methods that referenced a particular instance of the cppBase.

There are other changes, like I now realize that __proto__ can be safely used in the prototype literal definition, so I did not have to use those unwieldy Object methods to add properties. Also, there is not normally a reason for the Js base objects to be created independently of their specialized subclasses, so I removed that (and the corresponding test).

Comment 35

[Kent James (:rkent)]

It was also apparent that we need to have definitions of the Ja base classes in JS, as we really expect the account-specific users to derive from them. Previously only a test version of these was shown, now I will add them as I create objects for each type of mailnews function.

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8700360 [details] [diff] [review]
Part 3, IncomingServer

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

rs=jorgk on this one. I can't see that it does any harm.

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 8700365 [details] [diff] [review]
Part 5, Compose

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

rs=jorgk on this one. I can't see that it does any harm.

Comment 39

[Jorg K (CEST = GMT+2)]

Comment on attachment 8743096 [details] [diff] [review]
part 1, URL and general demo (now with CPP as  __proto__ to JS)

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

rs=jorgk on this one. I can't see that it does any harm. If it doesn't work, Kent will notice.
If the tests fail, we'll back it out ;-)

::: mailnews/jsaccount/modules/JaBaseUrl.jsm
@@ +2,5 @@
> +/* vim: set ts=2 et sw=2 tw=80 filetype=javascript: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> + 

Trailing space.

Comment 40

[Jorg K (CEST = GMT+2)]

Comment on attachment 8700364 [details] [diff] [review]
Part 4, MsgFolder

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

Maybe Aceman can look this over, he's closer to the database.

Comment 41

[:aceman]

Comment on attachment 8700364 [details] [diff] [review]
Part 4, MsgFolder

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

This is highly bitrotted so I couldn't even compile test it. Or I applied it in the wrong order (I tried part 1 ... part 6).

::: mailnews/base/public/nsIMsgIncomingServer.idl
@@ +95,5 @@
> +   * The schema for the nsIMsgDatabase implementation, such as "mailbox" or
> +   * "imap", that will be used to construct the database instance used by
> +   * message folders associated with this server.
> +   */
> +  readonly attribute ACString localDatabaseType;

So shouldn't this be a property of nsIMsgFolder? Or do we need all folders in a server/account be of the same type and same DB type?

::: mailnews/base/util/nsMsgIncomingServer.cpp
@@ +975,5 @@
>  
>  NS_IMETHODIMP
> +nsMsgIncomingServer::GetLocalDatabaseType(nsACString& aResult)
> +{
> +  NS_NOTYETIMPLEMENTED("nsMsgIncomingServer superclass not implementing GetLocalDatabaseType!");

Is this supposed to be ever implemented? Or is the *YET* misleading or wrongly used here?

::: mailnews/imap/src/nsImapMailFolder.h
@@ +398,5 @@
> +  NS_IMETHOD GetIncomingServerType(nsACString& serverType)
> +  {
> +    serverType.AssignLiteral("imap");
> +    return NS_OK;
> +  }

Does the implementation have to be in the .h file instead of nsImapMailFolder.cpp?

::: mailnews/jsaccount/src/JaMsgFolder.cpp
@@ +15,5 @@
> +#define MAILDATABASE_CONTRACTID_BASE "@mozilla.org/nsMsgDatabase/msgDB-"
> +
> +namespace mozilla {
> +namespace mailnews {
> +  

whitespace.

@@ +89,5 @@
> +
> +  return rv;
> +}
> +
> +/*

I think we use /** .

::: mailnews/jsaccount/src/JaMsgFolder.h
@@ +97,5 @@
> +      NS_FORWARD_NSIMSGTRAITCLASSIFICATIONLISTENER(mFakeThis->JaBaseCppMsgFolder::)
> +      NS_FORWARD_NSIINTERFACEREQUESTOR(mFakeThis->JaBaseCppMsgFolder::)
> +    private:
> +      virtual ~Super() {}
> +      JaCppMsgFolderDelegator* mFakeThis;

Maybe you could add a comment why the variable is called Fake :)

::: mailnews/jsaccount/test/unit/resources/testJaBaseIncomingServer.jsm
@@ +6,5 @@
> +/**
> +  This file creates a JS-based override of the JaIncomingServer implementation. It
> +  demos a minimal JS class, and is also used in testing the additional methods
> +  added to JaIncomingServer.cpp that are not in nsMsgDBFolder.cpp
> +**/

I think the ending bracket uses to be ' */'.

@@ +34,5 @@
> +  classID:            Components.ID("{0eec03cd-da67-4949-ab2d-5fa4bdc68135}"),
> +};
> +
> +function JaBaseIncomingServer(aDelegator, aBaseInterfaces) {
> +

Why this empty line?

@@ +36,5 @@
> +
> +function JaBaseIncomingServer(aDelegator, aBaseInterfaces) {
> +
> +  dump("JaBaseIncomingServer\n");
> +// Typical boilerplate to include in all implementations.

Why is this not indented?

@@ +51,5 @@
> +}
> +
> +JaBaseIncomingServer.prototype = {
> +
> +// Typical boilerplate to include in all implementations.

Why is this not indented?

::: mailnews/jsaccount/test/unit/resources/testJaBaseMsgFolder.jsm
@@ +6,5 @@
> +/**
> +  This file creates a JS-based override of the JaMsgFolder implementation. It
> +  demos a minimal JS class, and is also used in testing the additional methods
> +  added to JaMsgFolder.cpp that are not in nsMsgDBFolder.cpp
> +**/

As written above.

@@ +39,5 @@
> +};
> +
> +function JaBaseMsgFolder(aDelegator, aBaseInterfaces) {
> +
> +// Typical boilerplate to include in all implementations.

As written above.

@@ +54,5 @@
> +}
> +
> +JaBaseMsgFolder.prototype = {
> +
> +// Typical boilerplate to include in all implementations.

As written above.

@@ +69,5 @@
> +  delegateList: null,
> +
> +  // nsIMsgFolder overrides.
> +  get incomingServerType() { return "testja";},
> +  

whitespace

::: mailnews/jsaccount/test/unit/test_jaMsgFolder.js
@@ +2,5 @@
> +/* vim: set ts=2 et sw=2 tw=80 filetype=javascript: */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// This tests the the additional methods added to JaMsgFolder.cpp that are not

Double 'the'.

Comment 42

[Kent James (:rkent)]

Comment on attachment 8743096 [details] [diff] [review]
part 1, URL and general demo (now with CPP as  __proto__ to JS)

Pushed this first patch https://hg.mozilla.org/comm-central/rev/140e94c99688, more to come.

Comment 43

[Kent James (:rkent)]

Comment on attachment 8700359 [details] [diff] [review]
Part 2, AbDirectory

https://hg.mozilla.org/comm-central/rev/4ce90282d078

Comment 44

[Kent James (:rkent)]

(In reply to :aceman from comment #41)
> Comment on attachment 8700364 [details] [diff] [review]
> Part 4, MsgFolder
> ::: mailnews/base/public/nsIMsgIncomingServer.idl
> @@ +95,5 @@
> > +   * The schema for the nsIMsgDatabase implementation, such as "mailbox" or
> > +   * "imap", that will be used to construct the database instance used by
> > +   * message folders associated with this server.
> > +   */
> > +  readonly attribute ACString localDatabaseType;
> 
> So shouldn't this be a property of nsIMsgFolder? Or do we need all folders
> in a server/account be of the same type and same DB type?
>

I suppose that in theory we could have different database type for different folders, but the current implementations are that database type follows server type. This is also highly parallel now to the LocalStoreType implementations, which makes this easier to understand. I don't think we should add complexity because something is possible with no use case in mind, or current application. 

> ::: mailnews/base/util/nsMsgIncomingServer.cpp
> @@ +975,5 @@
> >  
> >  NS_IMETHODIMP
> > +nsMsgIncomingServer::GetLocalDatabaseType(nsACString& aResult)
> > +{
> > +  NS_NOTYETIMPLEMENTED("nsMsgIncomingServer superclass not implementing GetLocalDatabaseType!");
> 
> Is this supposed to be ever implemented? Or is the *YET* misleading or
> wrongly used here?

I just followed the pattern from the existing code for GetLocalStoreType. The interpretation is that when you get the error message, there is a missing implementation SOMEWHERE, it just isn't here it is in the superclass!
 
> ::: mailnews/imap/src/nsImapMailFolder.h
> @@ +398,5 @@
> > +  NS_IMETHOD GetIncomingServerType(nsACString& serverType)
> > +  {
> > +    serverType.AssignLiteral("imap");
> > +    return NS_OK;
> > +  }
> 
> Does the implementation have to be in the .h file instead of
> nsImapMailFolder.cpp?

Moved to nsImapFolder.cpp

> 
> ::: mailnews/jsaccount/src/JaMsgFolder.cpp
> @@ +15,5 @@
> > +#define MAILDATABASE_CONTRACTID_BASE "@mozilla.org/nsMsgDatabase/msgDB-"
> > +
> > +namespace mozilla {
> > +namespace mailnews {
> > +  
> 
> whitespace.
> 
> @@ +89,5 @@
> > +
> > +  return rv;
> > +}
> > +
> > +/*
> 
> I think we use /** .
> 

We use /** in formal documentation settings. This is not that, it is just a comment, but a little onger than the other comments that start with //

> ::: mailnews/jsaccount/src/JaMsgFolder.h
> @@ +97,5 @@
> > +      NS_FORWARD_NSIMSGTRAITCLASSIFICATIONLISTENER(mFakeThis->JaBaseCppMsgFolder::)
> > +      NS_FORWARD_NSIINTERFACEREQUESTOR(mFakeThis->JaBaseCppMsgFolder::)
> > +    private:
> > +      virtual ~Super() {}
> > +      JaCppMsgFolderDelegator* mFakeThis;
> 
> Maybe you could add a comment why the variable is called Fake :)

I'll try. Naming has been a big challenge in this whole project.

> 
> ::: mailnews/jsaccount/test/unit/resources/testJaBaseIncomingServer.jsm
> @@ +6,5 @@
> > +/**
> > +  This file creates a JS-based override of the JaIncomingServer implementation. It
> > +  demos a minimal JS class, and is also used in testing the additional methods
> > +  added to JaIncomingServer.cpp that are not in nsMsgDBFolder.cpp
> > +**/
> 
> I think the ending bracket uses to be ' */'.

Yes you are right, I checked around a bit and I have not been following the most common standards. I'll fix here.

Most comments accepted as is and implemented.

Comment 45

[Kent James (:rkent)]

Comment on attachment 8700360 [details] [diff] [review]
Part 3, IncomingServer

Landed http://hg.mozilla.org/comm-central/rev/8aeec6f9db89

Comment 46

[Kent James (:rkent)]

Comment on attachment 8700364 [details] [diff] [review]
Part 4, MsgFolder

Landed http://hg.mozilla.org/comm-central/rev/bae6bffb1690

Comment 47

[Kent James (:rkent)]

Comment on attachment 8700365 [details] [diff] [review]
Part 5, Compose

Landed http://hg.mozilla.org/comm-central/rev/33a6b316b229

Comment 48

[Kent James (:rkent)]

Comment on attachment 8700366 [details] [diff] [review]
Part 6, Send

Landed http://hg.mozilla.org/comm-central/rev/f547dd9a56ee

Comment 49

[Jorg K (CEST = GMT+2)]

Can this be closed?

Comment 50

[Jorg K (CEST = GMT+2)]

2016-06-23 makes this TB 50.