Closed Bug 693341 Opened 8 years ago Closed 8 years ago

Add XPConnect test coverage for dependent params

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(9 files)

We currently have none, which is bad. Dependent params, especial arrays, have lots of special things about them that need to be handled.
Duplicate of this bug: 687662
Attachment #568308 - Flags: review?(khuey)
Attachment #568309 - Flags: review?(khuey)
Attachment #568311 - Flags: review?(khuey)
Attachment #568312 - Flags: review?(khuey)
Attached 7 patches. Flagging khuey for review.
Comment on attachment 568307 [details] [diff] [review]
part 2 - Test arrays of arithmetic types. v1

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

I should note that I'm mostly skimming these.

::: js/xpconnect/tests/idl/xpctest_params.idl
@@ +77,5 @@
> +                                       inout unsigned long bLength, [array, size_is(bLength)] inout short b,
> +                                       out unsigned long rvLength, [retval, array, size_is(rvLength)] out short rv);
> +  void                  testLongLongArray(in unsigned long aLength, [array, size_is(aLength)] in long long a,
> +                                          inout unsigned long bLength, [array, size_is(bLength)] inout long long b,
> +                                          out unsigned long rvLength, [retval, array, size_is(rvLength)] out long long rv);

Can we test both orders (in other words, have another copy with the array before the size parameter?)
Attachment #568307 - Flags: review?(khuey) → review+
Comment on attachment 568308 [details] [diff] [review]
part 3 - Test arrays of strings. v1

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

::: js/xpconnect/tests/unit/test_params.js
@@ +129,5 @@
>    doIsTest("testLongLongArray", [-10000000000], 1, [1, 3, 1234511234551], 3, arrayComparator);
> +  doIsTest("testStringArray", ["mary", "hat", "hey", "lid", "tell", "lam"], 6,
> +                              ["ids", "fleas", "woes", "wide", "has", "know", "!"], 7, arrayComparator);
> +  doIsTest("testWstringArray", ["沒有語言", "的偉大嗎?]"], 2,
> +                               ["we", "are", "being", "sooo", "international", "right", "now"], 7, arrayComparator);

yay unicode
Attachment #568308 - Flags: review?(khuey) → review+
Comment on attachment 568311 [details] [diff] [review]
part 6 - Test interface arrays. v1

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

::: js/xpconnect/tests/idl/xpctest_params.idl
@@ +42,5 @@
>   * covered by the intersection of return values and inout).
>   */
>  
>  #include "nsISupports.idl"
> +#include "xpctest_interfaces.idl"

Just forward declare the interfaces here.

::: js/xpconnect/tests/unit/test_params.js
@@ +135,5 @@
>                                ["ids", "fleas", "woes", "wide", "has", "know", "!"], 7, arrayComparator);
>    doIsTest("testWstringArray", ["沒有語言", "的偉大嗎?]"], 2,
>                                 ["we", "are", "being", "sooo", "international", "right", "now"], 7, arrayComparator);
> +  doIsTest("testInterfaceArray", [makeA(), makeA()], 2,
> +                                 [makeA(), makeA(), makeA(), makeA(), makeA(), makeA()], 6, arrayComparator);

Why aren't we using any makeB()s here?
Attachment #568311 - Flags: review?(khuey) → review+
Comment on attachment 568312 [details] [diff] [review]
part 7 - Test iid_is(). v1

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

::: js/xpconnect/tests/components/native/xpctest_params.cpp
@@ +320,5 @@
> +    // by XPConnect for the duration of the call. If we snatch it away from b
> +    // and leave no trace, XPConnect won't Release it. Since we also need to
> +    // return an already-AddRef'd pointer in rv, we don't need to do anything
> +    // special here.
> +    *rv = *b;

This is actually pretty scary stuff.
Attachment #568312 - Flags: review?(khuey) → review+
Any plans to test arrays with iid_is?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> Can we test both orders (in other words, have another copy with the array
> before the size parameter?)

I'd rather not add the extra goop that this would entail (since we save a lot of code by making assumptions about the ordering of arguments). But I'm open to being convinced if you think it's important.

> Why aren't we using any makeB()s here?

Because we're not using iid_is here, so the interfaces are statically typed, and the 'in' stuff needs to come out in the 'inout' position.

> Any plans to test arrays with iid_is?

That seems like a good thing to test, though it might require more goop. Let me investigate.
(In reply to Bobby Holley (:bholley) from comment #15)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> > Can we test both orders (in other words, have another copy with the array
> > before the size parameter?)
> 
> I'd rather not add the extra goop that this would entail (since we save a
> lot of code by making assumptions about the ordering of arguments). But I'm
> open to being convinced if you think it's important.

I don't think it's that important, was just wondering.

> > Why aren't we using any makeB()s here?
> 
> Because we're not using iid_is here, so the interfaces are statically typed,
> and the 'in' stuff needs to come out in the 'inout' position.

Ok.

> > Any plans to test arrays with iid_is?
> 
> That seems like a good thing to test, though it might require more goop. Let
> me investigate.

This is probably worth it.  Nothing in m-c uses it but there's some calendar stuff that depends on it ...

I learned that the hard way.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16)

> This is probably worth it.  Nothing in m-c uses it but there's some calendar
> stuff that depends on it ...
> 
> I learned that the hard way.

Fair enough. Patches attached, flagged for review.
Comment on attachment 569191 [details] [diff] [review]
part 8 - Improve array and interface comparisons. v1

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

::: js/xpconnect/tests/unit/test_params.js
@@ +65,5 @@
> +      for (var i = 0; i < a.length; ++i)
> +        if (!innerComparator(a[i], b[i]))
> +          return false;
> +      return true;
> +    } };

Nit:
  }
};
Attachment #569191 - Flags: review?(khuey) → review+
Pushed to mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/c5863a0662de
(and ancestors)
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.