Last Comment Bug 796903 - Move DOMImplementation to Paris bindings
: Move DOMImplementation to Paris bindings
Status: RESOLVED FIXED
: addon-compat
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
:
Mentors:
Depends on:
Blocks: ParisBindings 802560
  Show dependency treegraph
 
Reported: 2012-10-02 06:20 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-10-21 08:11 PDT (History)
3 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (16.93 KB, patch)
2012-10-04 13:02 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v2 (25.73 KB, patch)
2012-10-04 13:20 PDT, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
Details | Diff | Splinter Review
Part b: Pass an nsIContent to InternalIsSupported (8.12 KB, patch)
2012-10-05 13:09 PDT, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
Details | Diff | Splinter Review
Part c: Remove nsIDOMDOMImplementation (21.60 KB, patch)
2012-10-05 13:09 PDT, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
Details | Diff | Splinter Review
Part d: Followup fixes (5.74 KB, patch)
2012-10-13 13:23 PDT, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-10-02 06:20:46 PDT

    
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-10-04 13:02:13 PDT
Created attachment 668136 [details] [diff] [review]
Patch v1
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-10-04 13:14:51 PDT
Shouldn't there be some nsDocument bits here too?
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2012-10-04 13:20:40 PDT
Created attachment 668144 [details] [diff] [review]
Patch v2

Let's try this again.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-10-04 13:37:25 PDT
Comment on attachment 668144 [details] [diff] [review]
Patch v2

>+++ b/content/base/src/DOMImplementation.cpp
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMDOMImplementation)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMDOMImplementation)

This was there in the code you copied from, but it's clearly silly.  Make the nsISupports bit not use AMBIGUOUS?

>+++ b/dom/bindings/Bindings.conf
>+'DOMImplementation': {
>+},
>+

Now that bug 792980 is fixed, you don't need that.

Given that this is not prefable, why did you leave nsIDOMDOMImplementation and associated gunk?  Seems like it can go....

r=me with those addressed.  Though if you do remove nsIDOMDOMImplementation, I'd like a look at the result.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2012-10-05 13:09:04 PDT
Created attachment 668577 [details] [diff] [review]
Part b: Pass an nsIContent to InternalIsSupported

This interface is pretty silly. I like this better. (Turns out I don't strictly need it, as DOMImplementation still inherit from nsISupports.)
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-10-05 13:09:29 PDT
Created attachment 668578 [details] [diff] [review]
Part c: Remove nsIDOMDOMImplementation
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-10-05 13:23:41 PDT
Comment on attachment 668577 [details] [diff] [review]
Part b: Pass an nsIContent to InternalIsSupported

r=me
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-10-05 13:25:32 PDT
Comment on attachment 668578 [details] [diff] [review]
Part c: Remove nsIDOMDOMImplementation

r=me
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2012-10-13 13:23:01 PDT
Created attachment 671128 [details] [diff] [review]
Part d: Followup fixes

This passes try, at least. (Should've thought about that earlier.)
Comment 10 Olli Pettay [:smaug] 2012-10-13 17:17:16 PDT
Do we really want to remove nsIDOMDOMImplementation? I would expect some
binary addons to use it to create documents.
(And in fact it is probably the safest way to create "data" documents to be used in web pages.)
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-10-13 18:05:58 PDT
> I would expect some binary addons to use it to create documents.

How would they get their hands on an nsIDOMDOMImplementation?
Comment 12 Olli Pettay [:smaug] 2012-10-13 18:13:47 PDT
From nsIDOMDocument
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-10-13 18:15:28 PDT
And in particular, note that the ability to create a DOMImplementation by contract went away in bug 675166.

So are you thinking about cases in which the binary addon would call GetImplementation on an existing document?
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-10-13 18:16:27 PDT
I guess if we think binary addons are using it, we could keep it...  It seems somewhat unfortunate, though.  :(
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2012-10-15 11:19:06 PDT
Comment on attachment 671128 [details] [diff] [review]
Part d: Followup fixes

r=me on this part, but I talked to smaug and we decided to keep nsIDOMDOMImplementation for now for C++ consumers.  :(  Still don't need classinfo, of course.

Note You need to log in before you can comment on or make changes to this bug.