886755 - New DOM bindings enums in dictionaries which are not defined in the same file don't work

Status: RESOLVED FIXED

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

Description

[Jan Varga [:janv]]

Attachment: patch

For example:

PersistenceType.webidl:
enum PersistenceType { "permanent", "temporary" };

IDBFactory.webidl:
dictionary IDBOpenDBOptions
{
  long long version;
  PersistenceType storage;
};

IDBFactoryBinding.h:
...
struct IDBOpenDBOptions : public MainThreadDictionaryBase
{
  Optional<PersistenceType > mStorage;
  Optional<int64_t > mVersion;

private:
...

That doesn't compile since PersistenceType is not defined.
I'm attaching a simple patch that works, but it adds a useless "self" include for the cases where enums/dictionaries live in the same file.

Comment 1

[Jan Varga [:janv]]

Boris, could you help me with this ?

Comment 2

[Boris Zbarsky [:bzbarsky]]

The real basic problem here is that enums can't be forward-declared, right?

That said, why do you need to put them in different webidl files to start with?

Comment 3

[Jan Varga [:janv]]

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #2)
> The real basic problem here is that enums can't be forward-declared, right?

right

> 
> That said, why do you need to put them in different webidl files to start
> with?

Ehsan suggested to do it in his review comments for temp storage, bug 785884.
We will need the enum in different APIs, for example FileSystem API
http://lists.w3.org/Archives/Public/public-webapps/2013AprJun/0382.html

Comment 4

[Boris Zbarsky [:bzbarsky]]

Is it still true that the enum-class enums we use now cannot be forward-declared?

Comment 5

[Jan Varga [:janv]]

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #4)
> Is it still true that the enum-class enums we use now cannot be
> forward-declared?

interesting, I'll try to do it that way, thanks

Comment 6

[(no longer active)]

(In reply to comment #4)
> Is it still true that the enum-class enums we use now cannot be
> forward-declared?

We should be able to do that.  You can do that by editing mfbt/TypedEnum.h, and add a macro called MOZ_FORWARD_DECLARE_ENUM_CLASS and in the MOZ_HAVE_CXX11_STRONG_ENUMS case, have it expand to |enum class Name : Type;| and in the other case have it expand to |class Name;|.

I'd be happy to review a patch which does that.

Comment 7

[Jan Varga [:janv]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> (In reply to comment #4)
> > Is it still true that the enum-class enums we use now cannot be
> > forward-declared?
> 
> We should be able to do that.  You can do that by editing mfbt/TypedEnum.h,
> and add a macro called MOZ_FORWARD_DECLARE_ENUM_CLASS and in the
> MOZ_HAVE_CXX11_STRONG_ENUMS case, have it expand to |enum class Name :
> Type;| and in the other case have it expand to |class Name;|.
> 
> I'd be happy to review a patch which does that.

I have a patch that does exactly the same, I'm testing it here:
https://hg.mozilla.org/try/rev/df7c68aeb216

Comment 8

[Jan Varga [:janv]]

Attachment: patch v0.1

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 767753 [details] [diff] [review]
patch v0.1

Why are we still adding the #include now that we're forward-declaring things?

Comment 10

[Jan Varga [:janv]]

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #9)
> Comment on attachment 767753 [details] [diff] [review]
> patch v0.1
> 
> Why are we still adding the #include now that we're forward-declaring things?

the #include is now in the generated cpp file

when I remove it, I get this error:
/Users/varga/Sources/Mozilla/obj-ff-dbg/dom/bindings/IDBFactoryBinding.cpp:64:61: error: 
      use of undeclared identifier 'PersistenceTypeValues'; did you mean
      'PersistenceType'?
  ...temp.ref(), PersistenceTypeValues::strings, "PersistenceType", ...
                 ^~~~~~~~~~~~~~~~~~~~~
                 PersistenceType                                    
./IDBFactoryBinding.h:14:24: note: 'PersistenceType' declared here
MOZ_FORWARD_ENUM_CLASS(PersistenceType, uint32_t)
                       ^
../../dist/include/mozilla/TypedEnum.h:104:57: note: expanded from macro
      'MOZ_FORWARD_ENUM_CLASS'
#  define MOZ_FORWARD_ENUM_CLASS(Name, type) enum class Name : type;
                                                        ^
/Users/varga/Sources/Mozilla/obj-ff-dbg/dom/bindings/IDBFactoryBinding.cpp:64:61: error: 
      incomplete type 'mozilla::dom::PersistenceType' named in nested name
      specifier
  ...temp.ref(), PersistenceTypeValues::strings, "PersistenceType", ...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, in the .cpp file the #include makes perfect sense, yes!

Comment 12

[(no longer active)]

Comment on attachment 767753 [details] [diff] [review]
patch v0.1

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

r=me on the TypedEnum.h changes.  Please ask somebody else (khuey/peterv/smaug/Ms2ger) to review the codegen changes, as I know nothing about that code.  Thanks!

::: mfbt/TypedEnum.h
@@ +100,5 @@
>     * underlying type, so no extra check is needed.
>     */
> +#  define MOZ_BEGIN_ENUM_CLASS(Name, type)   enum class Name : type {
> +#  define MOZ_END_ENUM_CLASS(Name)           };
> +#  define MOZ_FORWARD_ENUM_CLASS(Name, type) enum class Name : type;

Nit: I think MOZ_FORWARD_DECLARE_ENUM_CLASS is a better name.

Also, please document the new macro above this block, perhaps at the end of the current documentation.  Thanks!

Comment 13

[Jan Varga [:janv]]

It seems that the forward declaration can't be used on platforms which don't support strong enums.

MOZ_FORWARD_DECLARE_ENUM_CLASS(PersistenceType, uint32_t)
Optional<PersistenceType > mStorage;

the macro translates to |class PersistenceType;| and the template fails to compile

https://tbpl.mozilla.org/?tree=Try&rev=984b5e717fb0
Am I interpreting it right?

I think we have to check MOZ_HAVE_CXX11_STRONG_ENUMS in the generated code and forward declare just the enum or include the entire file according to it.

Comment 14

[Andrew McCreight [:mccr8]]

Yeah, I tried to make forward declaration of enums work before, but I couldn't figure out any way to make it work on VS2010.

Comment 15

[Boris Zbarsky [:bzbarsky]]

We might as well do the #include in all cases, then, and just skip it if it would be the same file.  This probably involves telling CGHeaders which file it's for.

Comment 16

[(no longer active)]

Yeah, let's give up on the idea of forward declaring enums here.

Comment 17

[Jan Varga [:janv]]

Attachment: patch v0.2

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 770358 [details] [diff] [review]
patch v0.2

The checkin comment should probably be more like this:

  Bug 886755 - Include the correct binding header if a binding uses a WebIDL enumeration that's defined in a different .webidl file.

CGHeader.__init__ needs to document the new argument in its docstring.

Please document why the 'filename != prefix + ".h"' check is there.

r=me with that

Comment 19

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea62d12c0f1c

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea62d12c0f1c

Comment 21

[Max Vujovic]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #16)
> Yeah, let's give up on the idea of forward declaring enums here.

Do we have a way to forward declare enums defined with MOZ_BEGIN_ENUM_CLASS today?

I stumbled on this bug while looking for a way to do it.