Closed Bug 338871 Opened 18 years ago Closed 18 years ago

Warning: nsIWindowCreator2 is scriptable but inherits from the non-scriptable interface nsIWindowCreator

Categories

(Core Graveyard :: Embedding: APIs, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jhpedemonte, Assigned: jhpedemonte)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

When building, I get this warning:

./nsIWindowCreator2.idl:93: Warning: nsIWindowCreator2 is scriptable but
inherits from the non-scriptable interface nsIWindowCreator
Attached patch patch (obsolete) — Splinter Review
nsIWindowCreator2.idl has the following comment:

 55 // This interface is not generally scriptable: only the const(s)
 56 // should be visible.

So this interface needs to remain scriptable.  This patch makes nsIWindowCreator.idl scriptable, and adds [noscript] to its sole method, matching the layout of nsIWindowCreator2.idl.
Assignee: nobody → jhpedemonte
Status: NEW → ASSIGNED
Attachment #222921 - Flags: review?(benjamin)
Flags: blocking1.9a2?
Comment on attachment 222921 [details] [diff] [review]
patch

I don't think we need [noscript] on the method... I can't figure out what danm was worried about in any case, and bug 166442 doesn't provide any useful clues, if that is indeed the correct bug# from the nsIWindowCreator2.idl cvslog.
Attachment #222921 - Flags: superreview?(bzbarsky)
Attachment #222921 - Flags: review?(benjamin)
Attachment #222921 - Flags: review+
Well, then, do you think nsIWindowCreator2.createChromeWindow2() should have [noscript] removed?
yes, probably
I'm really not sure we should be changing a frozen interface like this....
Isn't there some funny weak pointer thing going on here that makes the API non-scriptable?  Or am I thinking of one of the other embedding APIs?
Nevermind my last comment.  I was thinking of another interface.

As for making a non-scriptable frozen interface scriptable, that sounds harmless enough... unless, maybe there could be existing implementors who do not expect their interface to be exposed to script?  In this case, I don't think we need to worry about that.
Attached patch patch v2Splinter Review
Looking through the code for uses of CreateChromeWindow() and CreateChromeWindow2(), I didn't see anything stick out as to why these two methods should be [noscript].  In fact, they are used quite a bit by embedders.  So it would be good to make this available to Java (and any other language), for embedding purposes.  This patch makes nsIWindowCreator scriptable and removes the [noscript] tag from nsIWindowCreator2's method.
Attachment #222921 - Attachment is obsolete: true
Attachment #223942 - Flags: superreview?(bzbarsky)
Attachment #223942 - Flags: review?(benjamin)
Attachment #222921 - Flags: superreview?(bzbarsky)
I'm still not happy with changing the scriptability of a frozen interface.  If someone is happy with it and is willing to sr this, that's fine by me, I guess, but I'm not willing to do so myself.
Boris, do you have any specific objections?  What problem are you concerned about?
Is it a problem that justifies creating a new, parallel interface that is scriptable?
That wouldn't help Javier unless we changed the inheritance of nsIWindowCreator2
Attachment #223942 - Flags: review?(benjamin) → review+
That's right, and it means that we'd need to create new versions of both interfaces.  So, my question changes slightly:  is that better than making this frozen interface scriptable?
I frankly can't explain why it gives me the creepies to change the scriptable flag on this specific frozen interface...

We could certainly change the inheritance of nsIWindowCreator2; that might be a better approach....
I don't think changing the inheritance of nsIWindowCreator2 is the way to go. As I mentioned in comment #8, these two interfaces are implemented by embedders.  If we want Java (and other languages) to be on par with C++ for embedding, and if we create a new scriptable ancestor to nsIWindowCreator2, then we would also have to change embedding/components/windowwatcher/src/nsWindowWatcher.cpp, along with all the embedders.
Comment on attachment 223942 [details] [diff] [review]
patch v2

I was thinking that nsIWindowCreator2 would inherit from nsISupports.  And yes, existing impls of nsIWindowCreator2 would need changing...

I guess you want all of these api changes on an api-stable branch, eh?  So changing the inheritance is not really an option either...

In any case, I stand by comment 9. I simply don't have time (and won't until probably July) to think deeply enough about this to maybe convince myself it's ok.  If someone else does, great.
Attachment #223942 - Flags: superreview?(bzbarsky)
So, Benjamin, Darin, where do we stand on this bug?  Do we go forward with this patch, or look for a workaround?  My views are in comment #15.  And to respond to bz's previous comment, yes, I would like this to be fixed on the 1.8 branch.
Comment on attachment 223942 [details] [diff] [review]
patch v2

I'm still ok with the proposed patch.
Attachment #223942 - Flags: superreview?(darin)
Comment on attachment 223942 [details] [diff] [review]
patch v2

I'm okay with this too.  sr=darin
Attachment #223942 - Flags: superreview?(darin) → superreview+
Checked in to trunk.  ->FIXED
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attachment #223942 - Flags: approval-branch-1.8.1?(darin)
Attachment #223942 - Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+
Keywords: fixed1.8.1
Bug is fixed on trunk.
Flags: blocking1.9a2?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: