1158806 - Fix the include situation for nsIDOM* when NeedsGeneratedHasInstance

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Our generated hasinstance method uses an nsIDOM* interface and QIs to it.  We include the relevant header like so:

        hasInstanceIncludes = set("nsIDOM" + d.interface.identifier.name + ".h" for d
                                  in descriptors if
                                  NeedsGeneratedHasInstance(d) and
                                  d.interface.hasInterfacePrototypeObject())

But we only need a hasInstance at all if hasInterfaceObject(), so we should check for that here too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't try to include stuff for a generated hasInstance hook if we have no interface object, since in that case we don't need the include

Not that this matters much, since the only things that are NeedsGeneratedHasInstance() and have an proto object are EventTarget (due to xponnect impls which we should really kill) and XPathEvaluator (which we should consider just breaking the hasinstance for, maybe....)

Comment 2

[Axel Nennker]

This webidl triggers the include of "nsIDOMCredential.h".
https://github.com/AxelNennker/firefox_credentials/blob/master/Credentials.webidl

dictionary CredentialData {
  DOMString id;
  DOMString name;
  USVString iconURL;
};


interface Credential {
  readonly attribute DOMString id;
  readonly attribute DOMString type;
};

dictionary PasswordCredentialData : CredentialData {
  DOMString password;
};

[Constructor(optional PasswordCredentialData data), Exposed=Window]
interface PasswordCredential {
  Promise<Response> send(USVString url);
};
PasswordCredential implements Credential;

dictionary CredentialRequestOptions {
  sequence<USVString> federations;
};
  
dictionary CredentialRequest {
  CredentialRequestOptions options;
  boolean suppressUI = false;
  sequence<DOMString> types;
};

[JSImplementation="@mozilla.org/credentials/manager;1", NavigatorProperty="credentials", NoInterfaceObject]
interface CredentialsContainer {
  Promise<Credential?> get(optional CredentialRequest request);
  Promise<Credential> store(optional Credential credential);
  Promise<void> requireUserMediation();
};

 9:17.76 In file included from /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings/UnifiedBindings3.cpp:62:0:
 9:17.76 /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings/CredentialsBinding.cpp:15:30: fatal error: nsIDOMCredential.h: No such file or directory
 9:17.76  #include "nsIDOMCredential.h"
 9:17.76                               ^
 9:17.76 compilation terminated.
 9:20.01 Unified_cpp_webrtc_modules2.o
 9:22.03 Unified_cpp_webrtc_modules0.o
 9:22.03 Unified_cpp_webrtc_modules1.o
 9:22.24 
 9:22.24 In the directory  /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings
 9:22.24 The following command failed to execute properly:
 9:22.24 /usr/bin/ccache c++ -o UnifiedBindings3.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /home/ignisvulpis/mozilla/firefox/config/gcc_hidden.h -DOS_POSIX=1 -DOS_LINUX=1 -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DMOZ_GLUE_IN_PROGRAM -DAB_CD=en-US -DNO_NSPR_10_SUPPORT -I/home/ignisvulpis/mozilla/firefox/dom/bindings -I. -I../../dist/include/mozilla/dom -I/home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders -I/home/ignisvulpis/mozilla/firefox/dom/base -I/home/ignisvulpis/mozilla/firefox/dom/battery -I/home/ignisvulpis/mozilla/firefox/dom/bluetooth -I/home/ignisvulpis/mozilla/firefox/dom/camera -I/home/ignisvulpis/mozilla/firefox/dom/canvas -I/home/ignisvulpis/mozilla/firefox/dom/geolocation -I/home/ignisvulpis/mozilla/firefox/dom/html -I/home/ignisvulpis/mozilla/firefox/dom/indexedDB -I/home/ignisvulpis/mozilla/firefox/dom/media/webaudio -I/home/ignisvulpis/mozilla/firefox/dom/media/webspeech/recognition -I/home/ignisvulpis/mozilla/firefox/dom/svg -I/home/ignisvulpis/mozilla/firefox/dom/workers -I/home/ignisvulpis/mozilla/firefox/dom/xbl -I/home/ignisvulpis/mozilla/firefox/dom/xml -I/home/ignisvulpis/mozilla/firefox/dom/xslt/base -I/home/ignisvulpis/mozilla/firefox/dom/xslt/xpath -I/home/ignisvulpis/mozilla/firefox/dom/xul -I/home/ignisvulpis/mozilla/firefox/js/xpconnect/src -I/home/ignisvulpis/mozilla/firefox/js/xpconnect/wrappers -I/home/ignisvulpis/mozilla/firefox/layout/style -I/home/ignisvulpis/mozilla/firefox/layout/xul/tree -I/home/ignisvulpis/mozilla/firefox/media/mtransport -I/home/ignisvulpis/mozilla/firefox/media/webrtc/ -I/home/ignisvulpis/mozilla/firefox/media/webrtc/signaling/src/common/time_profiling -I/home/ignisvulpis/mozilla/firefox/media/webrtc/signaling/src/peerconnection -I/home/ignisvulpis/mozilla/firefox/ipc/chromium/src -I/home/ignisvulpis/mozilla/firefox/ipc/glue -I/home/ignisvulpis/mozilla/firefox/dom/bluetooth/bluetooth1 -I../../dist/include -I/home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dist/include/nspr -I/home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dist/include/nss -fPIC -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/UnifiedBindings3.o.pp -Wall -Wempty-body -Woverloaded-virtual -Wsign-compare -Wwrite-strings -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -Wno-uninitialized -DDEBUG -DTRACING -g -freorder-blocks -Os -fno-omit-frame-pointer /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings/UnifiedBindings3.cpp
 9:22.24 make[5]: *** [UnifiedBindings3.o] Error 1
 9:22.24 make[5]: *** Waiting for unfinished jobs....

I tried to cleanup and get rid of the "interface" stuff but without it I get the error message above.
https://github.com/AxelNennker/firefox_credentials/tree/master/interfaces


-Axel

Comment 3

[Boris Zbarsky [:bzbarsky]]

> This webidl triggers the include of "nsIDOMCredential.h".

Why did you remove the [NoInterfaceObject] from Credential?

If something appears on the RHS of "implements", it should be [NoInterfaceObject].

Comment 4

[Boris Zbarsky [:bzbarsky]]

In any case, comment 2 has nothing to do with this bug....  Given the IDL there the include _is_ needed, since Credential has both an interface object and a prototype object but appears on the rhs of "implements".

Comment 5

[Axel Nennker]

(In reply to Not doing reviews right now from comment #3)
> > This webidl triggers the include of "nsIDOMCredential.h".
> 
> Why did you remove the [NoInterfaceObject] from Credential?
> 
> If something appears on the RHS of "implements", it should be
> [NoInterfaceObject].

Probably because I still could not get my head around [NoIntercaceObject].
My reading of http://www.w3.org/TR/WebIDL/#NoInterfaceObject is that the interface just does not appear on window;

Which text in the TR implies the rule?: 
"If something appears on the RHS of "implements", it should be [NoInterfaceObject]."

My guess is that the rule is related to supplemental interfaces:
http://www.w3.org/TR/WebIDL/#dfn-supplemental-interface
But in my reading this says when not to use NoInterfaceObject but not when is should be.

Comment 6

[Axel Nennker]

(In reply to Not doing reviews right now from comment #4)
> In any case, comment 2 has nothing to do with this bug....  Given the IDL
> there the include _is_ needed, since Credential has both an interface object
> and a prototype object but appears on the rhs of "implements".

Does this mean that I have to describe Credential twice? Once in WebIDL and then again in idl?
https://github.com/AxelNennker/firefox_credentials/blob/master/interfaces/nsIDOMCredential.idl
(or write nsIDOMCredential.h myself)

And then write 
class Credential : nsIDOMCredential
{
void GetId(nsString& aRetVal) { aRetVal =  id; }

Probably again my missing understanding of the subject... sorry.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Which text in the TR implies the rule?: 

None.  It's allowed per WebIDL spec, and there is one API in the web platform that does it.  It's just bad API design and people shouldn't add more APIs like that.  If you _do_ decide to add an API like that, you will need to define a corresponding nsIDOM* for use in instanceof checks.

Again, none of that has anything to do with this bug.

> Does this mean that I have to describe Credential twice?

If you insist on using interfaces on the rhs of implements that have an interface object, then yes.  The IDL version can be an empty interface; all codegen cares about is being able to QI to it.  

Again, the right solution is to not have interfaces with an interface object that are on the rhs of "implements".  That conflates mixins and classes in an undesirable way.

Comment 8

[Axel Nennker]

Even when Credential is a NoInterfaceObject the "#include nsIDOMCredential.h" is needed.

-----

ignisvulpis@namenlos:~/mozilla/firefox$ cat dom/webidl/Credentials.webidl 
/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this file,
 * You can obtain one at http://mozilla.org/MPL/2.0/.
 */

dictionary CredentialData {
  DOMString id;
  DOMString name;
  USVString iconURL;
};

[NoInterfaceObject]
interface Credential {
  readonly attribute DOMString id;
  readonly attribute DOMString type;
};

dictionary PasswordCredentialData : CredentialData {
  DOMString password;
};

[Constructor(optional PasswordCredentialData data), Exposed=Window]
interface PasswordCredential {
  Promise<Response> send(USVString url);
};
PasswordCredential implements Credential;

dictionary CredentialRequestOptions {
  sequence<USVString> federations;
};
  
dictionary CredentialRequest {
  CredentialRequestOptions options;
  boolean suppressUI = false;
  sequence<DOMString> types;
};

[JSImplementation="@mozilla.org/credentials/navigatorcredentials;1", NavigatorProperty="credentials", NoInterfaceObject]
interface CredentialsContainer {
  Promise<Credential?> get(optional CredentialRequest request);
  Promise<Credential> store(optional PasswordCredential credential);
  Promise<void> requireUserMediation();
};
ignisvulpis@namenlos:~/mozilla/firefox$ 

------

 6:05.03 In file included from /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings/UnifiedBindings3.cpp:62:0:
 6:05.04 /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings/CredentialsBinding.cpp:15:30: fatal error: nsIDOMCredential.h: No such file or directory
 6:05.04  #include "nsIDOMCredential.h"
 6:05.04                               ^
 6:05.04 compilation terminated.

------

Why do I have to write any implementation of Credential anyway? 
Why would anyone have two different implementations for simple interfaces like Credential.
Maybe introduce [NoInterfaceObject, MozAutoGeneratedImplementation]

+++++++

Another question: Why does Mozilla have NavigatorProperty="credentials" instead of 
partial interface Navigator {
  readonly attribute CredentialContainer credentials;
};

and would'nt this be the way for the credential management spec to solve the extension point

partial interface CredentialContainer {
  Promise<Credential?> get(optional CredentialRequest request);
  Promise<void>requireUserMediation();
};
partial interface CredentialContainer {
    Promise<Credential> store(PasswordCredential credential);
}
partial interface CredentialContainer {
    Promise<Credential> store(FooCredential credential);
}

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Even when Credential is a NoInterfaceObject the "#include nsIDOMCredential.h" is needed.

Yes, that, finally, is this bug.  You should have tried with the patch here.


The rest of what you wrote has nothing to do with this bug.  Feel free to mail me directly if you have codegen questions, but this is not the right place for them.  I'm going to respond to comment 8, but that's it.

> Why do I have to write any implementation of Credential anyway? 

Because it's being passed as an argument (as I've said before, the spec is buggy here), so the codegen needs to know what that maps to on the C++ side.  Note that you don't necessarily need an implementation per se; e.g. it could all be pure virtual with all implementation living in the subclasses.

> Why would anyone have two different implementations for simple interfaces like
> Credential.

I'm not sure what you're asking here.  Please feel free to clarify over e-mail.

> Maybe introduce [NoInterfaceObject, MozAutoGeneratedImplementation]

I have no idea what the generated implementation would do here and hence whether it would be commonly used.  We do have something like this for DOM events, since that _is_ commonly enough used that it made sense to hook up implementation autogeneration.  Again, this bug is not the right place for this discussion.

> Another question: Why does Mozilla have NavigatorProperty="credentials"

Which Mozilla?  There is no NavigatorProperty="credentials" anywhere in our source code.  Again, nothing to do with this bug, feel free to send me mail.

> and would'nt this be the way for the credential management spec to solve

Yes, that would work fine.  As would using a union type.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 8597990 [details] [diff] [review]
Don't try to include stuff for a generated hasInstance hook if we have no interface object, since in that case we don't need the include

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

::: dom/bindings/Codegen.py
@@ +1774,5 @@
>          args = [Argument('JSContext*', 'cx'),
>                  Argument('JS::Handle<JSObject*>', 'obj'),
>                  Argument('JS::MutableHandle<JS::Value>', 'vp'),
>                  Argument('bool*', 'bp')]
> +        assert descriptor.interface.hasInterfaceObject()x

Drop that trailing 'x'?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Drop that trailing 'x'?

Er, yes, long since done locally since it totally fails to run that way.  ;)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13f7e6d0e909

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13f7e6d0e909