Closed Bug 855966 Opened 7 years ago Closed 7 years ago

Warn more harshly about failures in test_interfaces.html

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: dougt, Assigned: mccr8)

Details

Attachments

(1 file)

From Philor:

I suspect that hg log for test_interfaces.html will explain that the entire purpose of the test is to piss off everyone who lands a new interface, not positive though.


I tend to agree.  I can't see any reason why we want to have this test.  I vote for removing it with fire.
The point of the test is explained in the very first commit, the one that created it.

It's to prevent people from accidentally exposing things on the web in global scope without realizing that they're doing it.
I am going to morph the shit out of this bug....

http://hg.mozilla.org/mozilla-central/annotate/8aeabe064932/dom/tests/mochitest/general/test_interfaces.html

People have been just adding interfaces to this file without really thinking if things should be on the global.  We should take the time to go through this file, and inspect what has been added incorrect.
Summary: Remove dom/tests/mochitest/general/test_interfaces.html → test_interfaces.html lists things that probably should not be in the global scope
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee: doug.turner → continuation
This happened again, in bug 813756, but fortunately they asked Olli for review.

I'm going to make the explanation more dire, and add something about requiring a SR from a DOM peer to add anything to the file.   Maybe something about how if the name of your class is nsIDOMFoo, change it to nsIFoo to have it not be exposed to the web, or maybe I'll just put that in a comment.
Suggestions welcome on whatever.

I put the thing about needing a review at the top and bottom, because those are the most likely places somebody will add a new thing, and having it appear in the "patch window" means both patch author and reviewer will have a chance to see the message.  People seem to occasionally inject things into this list at random points, but hopefully they'll at least look at the test file and see one of the warnings.  I also included a fix for what is likely the most common cause, people calling something nsIDOMFoo and not realizing that puts it into the global scope.

This fails locally, but I'm assuming that it is not my patch at fault...
Attachment #742130 - Flags: superreview?(bzbarsky)
Attachment #742130 - Flags: review?(bzbarsky)
Comment on attachment 742130 [details] [diff] [review]
I used caps so you know it is important

>+     "Exposing a new interface to all webpages via the global scope: " + name);

How about:

  "DANGER: Are you sure you want to expose the new interface " + name + " to all web pages as a property on the window?"

r+sr=me
Attachment #742130 - Flags: superreview?(bzbarsky)
Attachment #742130 - Flags: superreview+
Attachment #742130 - Flags: review?(bzbarsky)
Attachment #742130 - Flags: review+
I went with "If this is failing: DANGER, are you sure you want to expose the new interface " + name + " to all webpages as a property on the window?"
Summary: test_interfaces.html lists things that probably should not be in the global scope → Warn more harshly about failures in test_interfaces.html
https://hg.mozilla.org/mozilla-central/rev/684f65a04b67
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.