Closed Bug 726053 Opened 12 years ago Closed 12 years ago

Add an instanceof wrapper to SpecialPowers

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: cviecco, Assigned: cviecco)

References

Details

Attachments

(1 file, 4 obsolete files)

Components.interfaces is now currently available from non site javascript. In order for this to be removed the test must be modified to that calls to 'instance of Components.interfaces.XXXX' can be done within the test suite. However this test cannot be done via wrapping Components as the proxy instance of is not transparent to proxy objects.
Assignee: nobody → cviecco
Attachment #597567 - Flags: review? → review?(ted.mielczarek)
Comment on attachment 597567 [details] [diff] [review]
Addition an in-chrome context call of instance-of

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

Unfortunate, but okay.
Attachment #597567 - Flags: review?(ted.mielczarek) → review+
Can we get this pushed?
(along with a patch that replaces all of the current calls to instanceof with SpecialPowers.call_Instanceof?)
ACK, will work on the second patch.
Summary: Expand SpecialPowers API to allow checking for having a components interface → Add an instanceof wrapper to SpecialPowers
Blocks: 697554
Attached patch Use of proposed patch on tests. (obsolete) — Splinter Review
Attachment #601823 - Flags: review?
Note the latest past has NOT yet passes trough try. I have test it locally against the complete mochitest suite on Linux 64bit without issues.
Comment on attachment 601823 [details] [diff] [review]
Use of proposed patch on tests.

Hi Camilo,

When you flag for review, you need to flag a specific person, otherwise the review request is likely to go nowhere. In this case, ted is probably the best person to review this.

In the mean time though, there are a few issues with the patch:

1 - arguments to functions should be commas-separated. So foo(a, b) rather than foo(a,b).

2 - When a line gets very long, it's good to break it up and align it properly. So, for example:

SpecialPowers.call_Instanceof(SpecialPowers.getNodePrincipal(document),
                              Components.interfaces.nsIPrincipal)

3 - This patch shouldn't actually insert any calls to SpecialPowers.wrap. We'll do all of that at once in a central place once instanceof is no longer an issue.
Attachment #601823 - Flags: review? → review-
Attachment #601823 - Attachment is obsolete: true
Attachment #602069 - Flags: review?
Attachment #602069 - Flags: review? → review?(ted.mielczarek)
Comment on attachment 602069 [details] [diff] [review]
Version2 of the actual changes to the tests

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

This is a lot uglier, but I guess it's for the greater good.

::: content/base/test/test_bug422537.html
@@ +36,5 @@
>    var xhr = new XMLHttpRequest();
>    xhr.open("POST", url, true);
>    xhr.send(i);
>    var chan = xhr.channel;
> +  if (!(SpecialPowers.call_Instanceof(chan, Components.interfaces.nsIUploadChannel)))

You can drop the extra parentheses here now.

@@ +41,4 @@
>      throw "Must be an upload channel";
>    var stream = chan.uploadStream;
> +  if (!stream || !(SpecialPowers.call_Instanceof(stream,
> +                                                 Components.interfaces.nsISeekableStream)))

Here too.

::: content/base/test/test_bug435425.html
@@ +28,5 @@
>    var i = 0;
>    while ((currentEvents.length != i) &&
>           currentEvents[i].optional &&
>           ((currentEvents[i].type != evt.type) ||
> +          !(SpecialPowers.call_Instanceof(evt.target, currentEvents[i].target)))) {

and here

::: content/base/test/test_bug450160.html
@@ +36,2 @@
>         "Document should be an HTML document!");
> +    ok(!(SpecialPowers.call_Instanceof(doc1, Components.interfaces.nsIDOMSVGDocument)),

and here

@@ +67,5 @@
>    ok(docType1.ownerDocument, "docType should have ownerDocument!");
>    var doc1 = document.implementation.createDocument(null, null, docType1);
>    is(docType1.ownerDocument, doc1, "docType should have ownerDocument!");
>    ok(!doc1.documentElement, "Document shouldn't have document element!");
> +  ok(!(SpecialPowers.call_Instanceof(doc1, Components.interfaces.nsIDOMHTMLDocument)),

etc (I'm not going to point out all the rest of these)
Attachment #602069 - Flags: review?(ted.mielczarek) → review+
Attachment #602069 - Attachment is obsolete: true
Attachment #610206 - Flags: review?(ted.mielczarek)
Attachment #610206 - Flags: review?(ted.mielczarek) → review+
Blocks: 750638
This may not end up being necessary. Bug 703537 (Harmony direct proxies) is in the process of landing and being reviewed, and when I chatted with dherman about instanceof and proxies, IIRC he said that some sensible behaviour with regards to the proxied object could be possible. I'll follow up and figure out what the time frame for those patches are, and whether I'm remembering our conversation correctly.
Unbitrotting one of the hunks, carrying over review.
Attachment #610206 - Attachment is obsolete: true
Attachment #623972 - Flags: review+
No longer blocks: 750638
Blocks: 750638
Push backed out for failures in test_bug435425.html:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b4a63a0b90c2

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf99db30a20c

Seeing as this has bounced twice, please can there be a full green try run before this lands again (with URL pasted in-bug).
Attachment #623972 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f700556031c7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: