1290541 - Fix broken test_fooUrl.js and allow cleaner JsAccount override of cloning

Status: RESOLVED FIXED

(MailNews Core :: Networking, defect)

Description

[Kent James (:rkent)]

+++ This bug was initially created as a clone of Bug #1289933 +++

Comment 1

[Kent James (:rkent)]

Attachment: fix

I have a try run going with this, I'll ask for review after that completes successfully.

JsAccount, in order to have the same capability as similar mailnews methods, requires that key overrides be in XPCOM. That forces us to add cloneInternal to the interface, and rely on comments to prevent misuse. I had the same issue in the proposed changes for the msg database in the open/openInternal methods.

But really the correct way to do the other overrides is as expressed here, namely to override CloneInternal and let the specific methods (clone, cloneIgnoringRef, etc) stay the same. This requires a virtual CloneInternal which the existing code does not have.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8776168 [details] [diff] [review]
fix

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

Nothing surprising here. Looks good.

::: mailnews/jsaccount/test/unit/test_fooUrl.js
@@ +85,3 @@
>  
>      // But then it gets tricky. We can call cloneIgnoringRef in the C++ base
> +    // but it will use the virtual cloneInternal which is overridden.

Can you please explain the mechanism a bit more here. Your using a base class, but somehow something that happened in a superclass has an effect here. Now is the time to document it ;-)

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8776168 [details] [diff] [review]
fix

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

Another drive-by comment ;-)

::: mailnews/base/public/nsIMsgMailNewsUrl.idl
@@ +62,5 @@
> +   * @param aRefHandlingMode  one of the Ref constants defined above.
> +   * @param aNewRef           the new reference, if needed.
> +   * @return                  a cloned version of the current object.
> +   */
> +  nsIURI cloneInternal(in unsigned long aRefHandlingMode, in AUTF8String aNewRef);

How about we call this cloneBase() since it's not really "internal" any more?

Comment 4

[Kent James (:rkent)]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #2)
> Comment on attachment 8776168 [details] [diff] [review]
> fix
> 
> Review of attachment 8776168 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nothing surprising here. Looks good.
> 
> ::: mailnews/jsaccount/test/unit/test_fooUrl.js
> @@ +85,3 @@
> >  
> >      // But then it gets tricky. We can call cloneIgnoringRef in the C++ base
> > +    // but it will use the virtual cloneInternal which is overridden.
> 
> Can you please explain the mechanism a bit more here. Your using a base
> class, but somehow something that happened in a superclass has an effect
> here. Now is the time to document it ;-)

That comment was supposed to be the explanation. Do you understand what it means for a class to be virtual, giving polymorphic behavior? The underlying C++ in the base class for cloneIgnoringRef calls the method cloneInternal. Due to the polymorphic behavior of C++ objects, it uses the overridden version.

I need to assume that people who read the comment understand polymorphic behavior in C++ classes, and the JsAccount technology is designed to support that. If you do understand polymorphic behavior, how would you suggest that I make the comment clearer?

From Wikipedia:

Polymorphism: Subtyping, a form of polymorphism, is when calling code can be agnostic as to whether an object belongs to a parent class or one of its descendants. For example, a function might call "make_full_name()" on an object, which will work whether the object is of class Person or class Employee. This is another type of abstraction which simplifies code external to the class hierarchy and enables strong separation of concerns.

Comment 5

[Jorg K (CEST = GMT+2)]

I believe I understand C++ polymorphism well enough, but perhaps I lack the understanding on how that mixes with JS in general and your special cppBase attribute (and cppDelegator) in particular. It is not clear from which class in the hierarchy the methods are ultimately run.

We're looking at these four lines:
1) let cppBase = url.QueryInterface(Ci.msgIOverride).cppBase.QueryInterface(Ci.nsIURI);
2) cppBase instanceof Ci.nsIMsgMailNewsUrl; // so it sees cloneInternal
3) Assert.equal(cppBase.cloneInternal(Ci.nsIMsgMailNewsUrl.HONOR_REF, null).spec, "https://test.invalid/folder#modules");
4) Assert.equal(cppBase.cloneIgnoringRef().spec, "https://test.invalid/FOLDER");

Your comment for 3) is:
The cppBase object will not have the overrides.
Maybe clearer:
The cppBase object will not have the overrides since the method run comes from nsIMsgMailNewsUrl.
I hope that's right since nsIURI or nsIURL don't have a cloneInternal().

Your comment for 4) is:
We can call cloneIgnoringRef in the C++ base but it will use the virtual cloneInternal which is overridden.
Maybe clearer:
We can call cloneIgnoringRef in the C++ base nsIURI but it will use FooUrl::cloneIgnoringRef which in turn will use FooUrl::cloneInternal which causes the upper case.

I note that a few lines above you do something similar but get uppercase in both cases:
Assert.equal(cppDelegator.cloneInternal(Ci.nsIMsgMailNewsUrl.HONOR_REF, null).spec, "https://test.invalid/FOLDER#MODULES");
Assert.equal(cppDelegator.cloneIgnoringRef().spec, "https://test.invalid/FOLDER");
The cppDelegator must be your FooUrl class.

Comment 6

[Kent James (:rkent)]

Attachment: revision 2, trying to give more explanation

I've really tried to make this clearer, but the reality is that it is really complicated to explain. Hope you like this better.

Concerning "How about we call this cloneBase() since it's not really "internal" any more?" It actually still has a bit of internal meaning, since we do not want that version used directly, and it is only XPCOMified to allow the override. That, combined with the fact that this matches the naming of similar Mozilla code, makes me unenthusiastic about changing the name.

In the grand scheme of things, the override functionality pretty much works in the C++ -> JS conversions, so although it seems confusing here, it is simple to use. What is really hard is the memory management, plus there are many cases in the C++ where a method sets internal private variables that are inaccessible to JS but used in other parts of the C++ implementation. So it is not easy to make this all work in practice, but in the end it can work.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8777040 [details] [diff] [review]
revision 2, trying to give more explanation

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

Thanks for your effort! Posterity will appreciate it ;-)
r=jorgk

One nit and two questions.

::: mailnews/jsaccount/test/unit/resources/testJaFooUrlComponent.js
@@ +77,5 @@
>      }
>      return this.delegator.QueryInterface(iid);
>    },
>  
>      // nsIURI overrides

Is the following really an nsIURI overide?
nsIURI has no cloneInternal().
Shouldn't that also go under the next heading
// nsIMsgMailNewsUrl overrides?

::: mailnews/jsaccount/test/unit/test_fooUrl.js
@@ +34,5 @@
>    function test_msgIOverride() {
>      let url = newURL().QueryInterface(Ci.msgIOverride);
> +
> +    // test of access to wrapped JS object.
> + 

Nit: Trailing space.

@@ +73,5 @@
>  
>      // Demo of differences between the various versions of the object. The
>      // standard XPCOM constructor returns the JS implementation, as was tested
> +    // above. Try the same tests using the wrapped JS delegate, which should give
> +    // the same (overridden) results (overridden for clone, C++ class

s/clone/cloneInternal/ ?

Comment 8

[Kent James (:rkent)]

Pushed with nits and comments addressed.

http://hg.mozilla.org/comm-central/rev/ddc5d8a70ab1

Comment 9

[Jorg K (CEST = GMT+2)]

Aurora (TB 50):
https://hg.mozilla.org/releases/comm-aurora/rev/75e7f00fedba

Comment 10

[ISHIKAWA, Chiaki]

Hi, I see the following error in try-comm-central run.
TEST-UNEXPECTED-FAIL | mailnews/jsaccount/test/unit/test_fooUrl.js | test_nsIURI - [test_nsIURI : 58] "https://test.invalid/folder" == "https://test.invalid/FOLDER"

See https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f30908465c70de27213c32986744d96fda5a1100


Can this bugzilla entry be related???

Comment 11

[Jorg K (CEST = GMT+2)]

This is fixed, you need to rebase.

Comment 12

[ISHIKAWA, Chiaki]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #11)
> This is fixed, you need to rebase.

I see, thank you. I was under the impression that the source files from the latest repo was automatically used in try-comm-central job, but this use of latest files from the latest branch is only for files under ./mozilla (M-C directory) then.

Rechecking after updating my local tree.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to ISHIKAWA, Chiaki from comment #12)
> ... but this use of
> latest files from the latest branch is only for files under ./mozilla (M-C
> directory) then.
That's right, try uses your local head for C-C.