Move HTMLObjectElement to WebIDL

RESOLVED FIXED in mozilla22

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: bz)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla22
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 4 obsolete attachments)

19.84 KB, patch
Details | Diff | Splinter Review
10.09 KB, patch
Details | Diff | Splinter Review
26.71 KB, patch
Details | Diff | Splinter Review
34.00 KB, patch
Details | Diff | Splinter Review
20.75 KB, patch
peterv
: review+
Details | Diff | Splinter Review
19.51 KB, patch
peterv
: review+
johns
: review+
Details | Diff | Splinter Review
2.40 KB, patch
Benjamin Smedberg
: review+
Details | Diff | Splinter Review
2.04 KB, patch
peterv
: review+
Details | Diff | Splinter Review
26.67 KB, patch
peterv
: review+
Details | Diff | Splinter Review
5.82 KB, patch
peterv
: review+
Details | Diff | Splinter Review
26.02 KB, patch
peterv
: review+
johns
: review+
Details | Diff | Splinter Review
6.11 KB, patch
peterv
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
My patches also implement HTMLObjectElement.contentWindow and move ValidityState to Web IDL as bonus points.
(Reporter)

Comment 1

5 years ago
Created attachment 698471 [details] [diff] [review]
Part 1: Rename nsHTMLObjectElement to mozilla::dom::HTMLObjectElement
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #698471 - Flags: review?(bzbarsky)
(Reporter)

Comment 2

5 years ago
Created attachment 698472 [details] [diff] [review]
Part 2: Rename nsDOMValidityState to mozilla::dom::ValidityState
Attachment #698472 - Flags: review?(bzbarsky)
(Reporter)

Comment 3

5 years ago
Created attachment 698493 [details] [diff] [review]
Part 3: Move HTMLObjectElement and ValidityState to Web IDL bindings

This patch crashes in nsObjectLoadingContent::NotifyContentObjectWrapper when calling GetWrappedNativeOfNativeObject (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#2578).  I'm not quite sure how to make that correctly work with objects that do not go through xpconnect.

Here's a stack of the assertion failure:

Assertion failure: IS_WRAPPER_CLASS(js::GetObjectClass(obj)), at /Users/ehsanakhgari/moz/mozilla-central/js/xpconnect/src/xpcpublic.h:81

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
IS_WN_WRAPPER_OBJECT (obj=0x1251321c0) at xpcpublic.h:81
81	    MOZ_ASSERT(IS_WRAPPER_CLASS(js::GetObjectClass(obj)));
(gdb) bt
#0  IS_WN_WRAPPER_OBJECT (obj=0x1251321c0) at xpcpublic.h:81
#1  0x0000000102db3b15 in IS_SLIM_WRAPPER_OBJECT (obj=0x1251321c0) at xpcpublic.h:86
#2  0x0000000102db4394 in XPCWrappedNative::GetUsedOnly (ccx=@0x7fff5fbfc320, Object=0x116769480, Scope=0x124c2c300, Interface=0x10c8a9ac0, resultWrapper=0x7fff5fbfc2e0) at /Users/ehsanakhgari/moz/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:812
#3  0x0000000102d262f8 in nsXPConnect::GetWrappedNativeOfNativeObject (this=0x10c6722c0, aJSContext=0x124a41600, aScope=0x12512d060, aCOMObj=0x116769480, aIID=@0x105541550, _retval=0x7fff5fbfc488) at /Users/ehsanakhgari/moz/mozilla-central/js/xpconnect/src/nsXPConnect.cpp:1378
#4  0x0000000101eef82d in nsObjectLoadingContent::NotifyContentObjectWrapper (this=0x116769510) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:2580
#5  0x0000000101eef248 in nsObjectLoadingContent::InstantiatePluginInstance (this=0x116769510, aIsLoading=false) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:756
#6  0x0000000101ef74fe in nsObjectLoadingContent::SyncStartPluginInstance (this=0x116769510) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:2359
#7  0x0000000101eec596 in nsAsyncInstantiateEvent::Run (this=0x124c74f00) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:129
#8  0x0000000103bb2be6 in nsThread::ProcessNextEvent (this=0x10053ac40, mayWait=false, result=0x7fff5fbfc8d3) at /Users/ehsanakhgari/moz/mozilla-central/xpcom/threads/nsThread.cpp:627
#9  0x0000000103b1712c in NS_ProcessPendingEvents_P (thread=0x10053ac40, timeout=20) at nsThreadUtils.cpp:187
#10 0x000000010341255f in nsBaseAppShell::NativeEventCallback (this=0x10c67f7e0) at /Users/ehsanakhgari/moz/mozilla-central/widget/xpwidgets/nsBaseAppShell.cpp:97
#11 0x00000001033a230c in nsAppShell::ProcessGeckoEvents (aInfo=0x10c67f7e0) at /Users/ehsanakhgari/moz/mozilla-central/widget/cocoa/nsAppShell.mm:387
#12 0x00007fff8f61e101 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ ()
#13 0x00007fff8f61da25 in __CFRunLoopDoSources0 ()
#14 0x00007fff8f640dc5 in __CFRunLoopRun ()
#15 0x00007fff8f6406b2 in CFRunLoopRunSpecific ()
#16 0x00007fff8a25e0a4 in RunCurrentEventLoopInMode ()
#17 0x00007fff8a25de42 in ReceiveNextEventCommon ()
#18 0x00007fff8a25dcd3 in BlockUntilNextEventMatchingListInMode ()
#19 0x00007fff8ea11613 in _DPSNextEvent ()
#20 0x00007fff8ea10ed2 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#21 0x00000001033a0ae7 in -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (self=0x10c7efd30, _cmd=0x7fff8f23f444, mask=18446744073709551615, expiration=0x422d63c37f00000d, mode=0x7fff7a4911b0, flag=1 '\001') at /Users/ehsanakhgari/moz/mozilla-central/widget/cocoa/nsAppShell.mm:164
#22 0x00007fff8ea08283 in -[NSApplication run] ()
#23 0x00000001033a2de1 in nsAppShell::Run (this=0x10c67f7e0) at /Users/ehsanakhgari/moz/mozilla-central/widget/cocoa/nsAppShell.mm:741
#24 0x0000000102fec56c in nsAppStartup::Run (this=0x10c664380) at /Users/ehsanakhgari/moz/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:288
#25 0x00000001013dc382 in XREMain::XRE_mainRun (this=0x7fff5fbfe700) at /Users/ehsanakhgari/moz/mozilla-central/toolkit/xre/nsAppRunner.cpp:3823
#26 0x00000001013dcb4f in XREMain::XRE_main (this=0x7fff5fbfe700, argc=6, argv=0x7fff5fbfefb0, aAppData=0x7fff5fbfe938) at /Users/ehsanakhgari/moz/mozilla-central/toolkit/xre/nsAppRunner.cpp:3890
#27 0x00000001013dcf9d in XRE_main (argc=6, argv=0x7fff5fbfefb0, aAppData=0x7fff5fbfe938, aFlags=0) at /Users/ehsanakhgari/moz/mozilla-central/toolkit/xre/nsAppRunner.cpp:4093
#28 0x000000010000202e in do_main (argc=6, argv=0x7fff5fbfefb0, xreDirectory=0x100527500) at /Users/ehsanakhgari/moz/mozilla-central/browser/app/nsBrowserApp.cpp:195
#29 0x000000010000163d in main (argc=6, argv=0x7fff5fbfefb0) at /Users/ehsanakhgari/moz/mozilla-central/browser/app/nsBrowserApp.cpp:388
Attachment #698493 - Flags: feedback?(bzbarsky)
You can't do GetWrappedNativeOfNativeObject on non-XPConnect objects, since those do not in fact have any sort of nsIXPConnectWrappedNative.

The good news is that this thing doesn't care about the XPCWN; it just wants the JSObject for the element.  So you can just replace all that jazz with:

  JSObject* obj = GetWrapper();
  if (!obj) {
    // Nothing to do here if there's no wrapper for mContent. The proto
    // chain will be fixed appropriately when the wrapper is created.
    return;
  }
(Reporter)

Comment 5

5 years ago
But the nsIXPConnectWrappedNative* is also passed to the nsHTMLPluginObjElementSH::SetupProtoChain call further down that function.  What shall I do with that?
Well, you need a replacement for nsHTMLPluginObjElementSH::SetupProtoChain, right?  It needs be called from the relevant WrapNode implementations...  And here you'll want to call either SetupProtoChain or the new method, depending on whether IsDOMBinding(), I think.

For what it's worth, the only things SetupProtoChain wants out of the XPCWrappedNative are:

1)  The canonical prototype for the class.  You'll presumably need to add a virtual method for this or something, since the canonical proto is different for different subclasses of nsObjectLoadingContent.

2)  Pass to GetPluginInstanceIfSafe, which only wants the wrapper so it can extract an nsIContent from it.  We can get the nsIContent directly from the JSObject as needed, for the WebIDL binding case.
(Reporter)

Comment 7

5 years ago
(In reply to comment #6)
> Well, you need a replacement for nsHTMLPluginObjElementSH::SetupProtoChain,
> right?  It needs be called from the relevant WrapNode implementations...  And
> here you'll want to call either SetupProtoChain or the new method, depending on
> whether IsDOMBinding(), I think.
> 
> For what it's worth, the only things SetupProtoChain wants out of the
> XPCWrappedNative are:
> 
> 1)  The canonical prototype for the class.  You'll presumably need to add a
> virtual method for this or something, since the canonical proto is different
> for different subclasses of nsObjectLoadingContent.
> 
> 2)  Pass to GetPluginInstanceIfSafe, which only wants the wrapper so it can
> extract an nsIContent from it.  We can get the nsIContent directly from the
> JSObject as needed, for the WebIDL binding case.

So, do I need to keep the current code paths, and add alternate ones to work around these two items?
For now, yes, until all things that are using nsHTMLPluginObjElementSH are on WebIDL.

Updated

5 years ago
Duplicate of this bug: 280277
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Comment 10

5 years ago
Created attachment 698885 [details] [diff] [review]
Part 3: Move HTMLObjectElement and ValidityState to Web IDL bindings

Alright, I got this to a point where I'm crashing when calling ReleaseSliceNow on an nsISupports* which seems to be freed already.  I think I'm giving up on this for now -- I really don't have any idea what I'm doing any more...

I think we can take part 1 and 2 here for now anyway.  Boris, I can also look into ripping out the ValidityState parts and land them separately if you want.
(Reporter)

Updated

5 years ago
Attachment #698493 - Attachment is obsolete: true
Attachment #698493 - Flags: feedback?(bzbarsky)
(Reporter)

Comment 11

5 years ago
Created attachment 701210 [details] [diff] [review]
Part 2: Rename nsDOMValidityState to mozilla::dom::ValidityState

This fixes a friend declaration to satisfy g++.
Attachment #698472 - Attachment is obsolete: true
Attachment #698472 - Flags: review?(bzbarsky)
(Reporter)

Comment 12

5 years ago
Comment on attachment 701210 [details] [diff] [review]
Part 2: Rename nsDOMValidityState to mozilla::dom::ValidityState

Not sure why I didn't set the review flag here...
Attachment #701210 - Flags: review?(bzbarsky)
Comment on attachment 701210 [details] [diff] [review]
Part 2: Rename nsDOMValidityState to mozilla::dom::ValidityState

r=me
Attachment #701210 - Flags: review?(bzbarsky) → review+
Comment on attachment 698471 [details] [diff] [review]
Part 1: Rename nsHTMLObjectElement to mozilla::dom::HTMLObjectElement

r=me

Please feel free to assign to me once you land the reviewed bits.
Attachment #698471 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 15

5 years ago
So I first landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c95996fc3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b3fc69a1893

and then immediately realized I forgot to export the patches out of git properly, and therefore did not do proper renaming. :(

So I backed out in agony: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6a29d22334

And then relanded more properly:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a28186bf9748
http://hg.mozilla.org/integration/mozilla-inbound/rev/0a8c4b6efd8c
(Reporter)

Updated

5 years ago
Whiteboard: [leave open]
(Reporter)

Comment 16

5 years ago
Created attachment 702676 [details] [diff] [review]
Part 3: Move ValidityState to Web IDL bindings

https://tbpl.mozilla.org/?tree=Try&rev=ebb8869e6201

This should be good to go independently of the HTMLObjectElement parts.
Attachment #698885 - Attachment is obsolete: true
Attachment #702676 - Flags: review?(bzbarsky)
(Reporter)

Comment 17

5 years ago
Created attachment 702776 [details] [diff] [review]
Part 3: Move ValidityState to Web IDL bindings

This should fix the compiler warning that failed the Windows build.
Attachment #702676 - Attachment is obsolete: true
Attachment #702676 - Flags: review?(bzbarsky)
Attachment #702776 - Flags: review?(bzbarsky)
Comment on attachment 702776 [details] [diff] [review]
Part 3: Move ValidityState to Web IDL bindings

>+  already_AddRefed<mozilla::dom::ValidityState> Validity();

Why does this need to return already_AddRefed?  Can we not just return a ValidityState* and addref only in the case that cares (i.e. the GetValidity method)?

r=me with that fixed or explained.
Attachment #702776 - Flags: review?(bzbarsky) → review+
Actually, one more thing.  GetHTMLUnsignedIntAttr doesn't really seem related to that patch, but I guess it's not a problem to land it as part of these changes...

I assume you'll post whatever your WIP is on <object> itself?
(Reporter)

Comment 20

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #18)
> Comment on attachment 702776 [details] [diff] [review]
> Part 3: Move ValidityState to Web IDL bindings
> 
> >+  already_AddRefed<mozilla::dom::ValidityState> Validity();
> 
> Why does this need to return already_AddRefed?  Can we not just return a
> ValidityState* and addref only in the case that cares (i.e. the GetValidity
> method)?

OK. will do.

(In reply to Boris Zbarsky (:bz) from comment #19)
> Actually, one more thing.  GetHTMLUnsignedIntAttr doesn't really seem
> related to that patch, but I guess it's not a problem to land it as part of
> these changes...

Nah, it's best if I take it out now.  I'll put it in the next patch.

> I assume you'll post whatever your WIP is on <object> itself?

Of course.  I was too sleepy last night.  :-)
(Reporter)

Comment 22

5 years ago
Created attachment 702900 [details] [diff] [review]
Part 4: Move HTMLObjectElement to Web IDL bindings (WIP)
(Reporter)

Comment 23

5 years ago
Over to you, Boris!
Assignee: ehsan → bzbarsky
(Reporter)

Comment 26

5 years ago
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5938f7cbfa8e to traverse the mValidity member of HTMLObjectElement as well.
Keywords: dev-doc-needed
Whiteboard: [leave open] → [leave open][need to document legacycaller and the NeedNewResolve bits]
Created attachment 717342 [details] [diff] [review]
part 4.  Split out an HTMLObjectElement header.
Attachment #717342 - Flags: review?(peterv)
Created attachment 717344 [details] [diff] [review]
part 5.  Refactor some of the nsHTMLPluginObjElementSH code to be able to work with WebIDL objects.
Attachment #717344 - Flags: review?(peterv)
Attachment #717344 - Flags: review?(jschoenick)
Created attachment 717346 [details] [diff] [review]
part 6.  Add a telemetry ping for object/embed callability actually working.
Attachment #717346 - Flags: review?(benjamin)
Created attachment 717348 [details] [diff] [review]
part 7.  Make nsJSNPRuntime work with WebIDL objects.
Attachment #717348 - Flags: review?(peterv)
Created attachment 717349 [details] [diff] [review]
part 8.  Implement legacycaller support in WebIDL.
Attachment #717349 - Flags: review?(peterv)
Created attachment 717350 [details] [diff] [review]
part 9.  Implement support for having a NewResolve hook in WebIDL.
Attachment #717350 - Flags: review?(peterv)
Created attachment 717352 [details] [diff] [review]
part 10.  Implement the WebIDL API for <object>.
Attachment #717352 - Flags: review?(peterv)
Attachment #717352 - Flags: review?(jschoenick)
Created attachment 717353 [details] [diff] [review]
part 11.   Move HTMLObjectElement to Web IDL bindings.
Attachment #717353 - Flags: review?(peterv)
Whiteboard: [leave open][need to document legacycaller and the NeedNewResolve bits] → [need review][need to document legacycaller and the NeedNewResolve bits]
Comment on attachment 717344 [details] [diff] [review]
part 5.  Refactor some of the nsHTMLPluginObjElementSH code to be able to work with WebIDL objects.

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

I'm not terribly familiar with wrapper/js/prototype goop here, but just going from what I do understand about plugin prototypes this looks okay.

With the additional of several new DoCrypticPluginJSProtoOrWrapperOperation functions in multiple files, some kind of overview comment for how the whole prototype hackery ties together for plugin tags might be useful (if explaining such a thing briefly is even possible).
Attachment #717344 - Flags: review?(jschoenick) → review+
Comment on attachment 717352 [details] [diff] [review]
part 10.  Implement the WebIDL API for <object>.

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

::: dom/webidl/HTMLObjectElement.webidl
@@ +77,5 @@
> +
> +[NoInterfaceObject]
> +interface MozObjectLoadingContent {
> +  // Mirrored chrome-only scriptable nsIObjectLoadingContent methods.  Please
> +  // make sure to update this list if nsIObjectLoadingContent changes.

How long do these both need to coexist?

@@ +175,5 @@
> +  [ChromeOnly]
> +  readonly attribute URI? srcURI;
> +
> +  // It's very important that this stay [ChromeOnly].  If not, the
> +  // IsCallerChrome check in the implementation will need to be reinstated.

Doesn't this apply to almost all the ChromeOnly items here...?
Attachment #717352 - Flags: review?(jschoenick) → review+
> some kind of overview comment for how the whole prototype hackery ties together for
> plugin tags might be useful 

I can write up something, sure.  Note that some of this goop will go away in bug 843627.

> How long do these both need to coexist?

As long as people are using nsIObjectLoadingContent.  After these patches and the ones in bug 843627 JS code can just stop using them; that will leave C++ code.

> Doesn't this apply to almost all the ChromeOnly items here...?

Most of the rest of them didn't use to prevent untrusted script access, as far as I can tell...
(In reply to Boris Zbarsky (:bz) from comment #38)
> > Doesn't this apply to almost all the ChromeOnly items here...?
> 
> Most of the rest of them didn't use to prevent untrusted script access, as
> far as I can tell...

None of these have any business being exposed to content AFAICT, things like displayedType and getContentTypeForMIMEType are exposed now, but are only useful for probing for more information than navigator.plugins exposes, which we certainly didn't intend. That we tried to block pluginFallbackType but not activated is probably accidental.
> I can write up something, sure.

    /**
     * When a plug-in is instantiated, it can create a scriptable
     * object that the page wants to interact with.  We expose this
     * object by placing it on the prototype chain of our element,
     * between the element itself and its most-derived DOM prototype.
     *
     * GetCanonicalPrototype returns this most-derived DOM prototype.
     *
     * SetupProtoChain handles actually inserting the plug-in
     * scriptable object into the proto chain if needed.
     *
     * DoNewResolve is a hook that allows us to find out when the web
     * page is looking up a property name on our object and make sure
     * that our plug-in, if any, is instantiated.
     */
> None of these have any business being exposed to content AFAICT

OK, sounds good.  The comments for this interface now say:

  // Mirrored chrome-only scriptable nsIObjectLoadingContent methods.  Please
  // make sure to update this list if nsIObjectLoadingContent changes.  Also,
  // make sure everything on here is [ChromeOnly].

Updated

5 years ago
Attachment #717346 - Flags: review?(benjamin) → review+
Attachment #717342 - Flags: review?(peterv) → review+
Comment on attachment 717344 [details] [diff] [review]
part 5.  Refactor some of the nsHTMLPluginObjElementSH code to be able to work with WebIDL objects.

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2765,5 @@
> +  // (perhaps because WrapObject can happen on a random compartment?)
> +  // so make sure to enter the compartment of aObject.
> +  JSAutoCompartment ac(aCx, aObject);
> +  MOZ_ASSERT(nsCOMPtr<nsIContent>(do_QueryInterface(
> +    static_cast<nsIObjectLoadingContent*>(this)))->IsDOMBinding());

do_QueryObject(this) might work.

::: dom/base/nsDOMClassInfo.cpp
@@ +8405,5 @@
>  }
>  
> +nsHTMLPluginObjElementSH::SetupProtoChainRunner::SetupProtoChainRunner(
> +    nsIXPConnectWrappedNative* wrapper,
> +    nsIScriptContext* scriptContext,

aWrapper, aScriptContext.
Attachment #717344 - Flags: review?(peterv) → review+
Attachment #717348 - Flags: review?(peterv) → review+
Comment on attachment 717349 [details] [diff] [review]
part 8.  Implement legacycaller support in WebIDL.

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

We should throw if we're using a proxy to implement an interface that uses legacycaller, if we're not implementing that (I hacked it up once using |js::SetReservedSlot(obj, js::JSSLOT_PROXY_CALL, JS::NullValue());| on the proxy to force handler's call(...) to be called and then implementing DOMProxyHandler::call).

::: dom/bindings/Codegen.py
@@ +157,5 @@
>      def declare(self):
>          return "extern DOMJSClass Class;\n"
>      def define(self):
>          traceHook = TRACE_HOOK_NAME if self.descriptor.customTrace else 'NULL'
> +        callHook = LEGACYCALLER_HOOK_NAME if self.descriptor.operations["LegacyCaller"] else 'NULL'

nullptr

@@ +7669,5 @@
> +                    # We already added this method
> +                    return
> +            if name == "LegacyCaller":
> +                if op.isIdentifierLess():
> +                    # XXXbz I wish we were consistent about our renaming here.

I'm not sure what these comments are about.

::: dom/bindings/Configuration.py
@@ +261,5 @@
>              for m in iface.members:
>                  if m.isMethod() and m.isStringifier():
>                      addOperation('Stringifier', m)
> +                # Don't worry about inheriting legacycallers either: in
> +                # practice these are on most-derived prototypes.

Do we enforce that somehow?

::: dom/bindings/parser/WebIDL.py
@@ +635,5 @@
>                  self.members.append(unforgeableAttr)
>  
>          # Ensure that there's at most one of each {named,indexed}
> +        # {getter,setter,creator,deleter}, at most one stringifier,
> +        # and at most one legacycaller.

So this deviates from the spec, which seems to allow multiple legacy callers. I'm ok with restricting it, but we should have a comment and probably code in Descriptor.__init__ to throw if it happens.
Attachment #717349 - Flags: review?(peterv) → review+
Attachment #717350 - Flags: review?(peterv) → review+
Attachment #717352 - Flags: review?(peterv) → review+
Comment on attachment 717344 [details] [diff] [review]
part 5.  Refactor some of the nsHTMLPluginObjElementSH code to be able to work with WebIDL objects.

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2598,5 @@
> +  nsHTMLPluginObjElementSH::SetupProtoChain(cx, obj, wrapper, canonicalPrototype);
> +}
> +
> +JSObject*
> +nsObjectLoadingContent::GetCanonicalPrototype(JSContext* aCx, JSObject* aGlobal)

Btw, you could just use mGetProto from the DOMClass of the JS object instead of this.
Attachment #717353 - Flags: review?(peterv) → review+
> do_QueryObject(this) might work.

Sadly, no: there is no QueryInterface declared in nsObjectLoadingContent itself, so the name lookup is ambiguous....  I could define it as pure-virtual if needed, but doesn't seem worth it.

> aWrapper, aScriptContext.

Done.

> We should throw if we're using a proxy to implement an interface that uses
> legacycaller, 

Done, in Configuration.py.

> nullptr

Done.

> I'm not sure what these comments are about.

About "LegacyCall" vs "LegacyCaller" and "Stringify" vs "Stringifier"....  As in, the fact that we can't use the name of the special op as the method name to call for those.

> Do we enforce that somehow?

No.  I added such enforcement to Configuration.py, just to be safe.  Inside the loop that goes up the parent chain in Descriptor.__init__, like so:

                    if m.isLegacycaller() and iface != self.interface:
                        raise TypeError("We don't support legacycaller on "
                                        "non-leaf interface %s.\n%s" %
                                        (iface, iface.location))

> So this deviates from the spec, which seems to allow multiple legacy callers

Indeed.  That's never used in practice.  I added a comment that this is a spec violation.

> and probably code in Descriptor.__init__ to throw if it happens.

You mean if our parser is ever aligned with the spec? Done.  The other option is to remove support for overloading legacycaller from the spec, which may be a good idea.  Posted to public-script-coord about this.

> Btw, you could just use mGetProto from the DOMClass of the JS object instead of this.

I'll roll this into bug 843627 part 4.
The tests are actually in bug 843627 so not landed yet.
Flags: in-testsuite+ → in-testsuite?
Documented legacycaller, the changes to stringifier, and NeedNewResolve at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings
Keywords: dev-doc-needed → dev-doc-complete
Whiteboard: [need to document legacycaller and the NeedNewResolve bits]
Depends on: 878195
No longer depends on: 878195

Updated

5 years ago
Depends on: 883968
You need to log in before you can comment on or make changes to this bug.