1103153 - Detect if a union type move from UnionTypes.h to another header or vice versa

Status: ASSIGNED

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

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 1

[Peter Van der Beken [:peterv]]

Comment on attachment 8526990 [details] [diff] [review]
v1

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

Bug 1068740 made it so that the location of generated code can be in a global file or a binding file, depending on the contents of WebIDL files. We're missing this information in the dependency checks which leads to needing clobbers sometimes wen a WebIDL file changes. In particular, if A.webidl has a union type (which I'll name Foo) which is not used anywhere else we generate the code for that type in ABinding.h/cpp. If someone then adds a use of Foo to B.webidl we generate the code for that type in a global file (UnionTypes.h/cpp). At that point we need to rebuild ABinding.h/cpp so it doesn't contain the code for that type anymore (or we have duplicate code), but A.webidl hasn't changed. A similar issue is when we then remove the use from B.webidl again.

The attached patch uses some state in WebIDLCodegenManagerState to record which WebIDL files contain which of these types, and when we regenerate a binding it detects addition/removals of the types and regenerates all the binding files for WebIDL files that contain a type that was added/removed. Is this an acceptable solution? If so, should we store this state under a special key so it's clear that that key is reserved for Codegen.py?

Comment 2

[Peter Van der Beken [:peterv]]

BTW, another solution that Boris and I talked about was moving all these types into their own header. I've got this somewhat working (putting union and dictionary types in their own header and all enums in a header too), but I don't think we can really make this work because it means that the list of exported files depends on parsing the WebIDL files, and we need that list before we even preprocess the WebIDL files.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8526990 [details] [diff] [review]
v1

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

This seems OK. Although I don't have enough context into the codegen side of things to assess what's going on with header files. My expertise is with mozwebidlcodegen.

Some tests would be nice. mozwebidlcodegen is extensively tested to ensure we don't break the build.

::: dom/bindings/Codegen.py
@@ +1238,5 @@
>      than one WebIDL file.
>      """
>      return config.unionsPerFilename.get(webIDLFile, [])
>  
> +def ExtraChangedFiles(webIDLFile, config, oldState, newState):

For bonus points, you should change the naming in this file to match Python convention. CamelCase is typically reserved for class names. extra_changed_files is what you'll find in most Python.

::: dom/bindings/mozwebidlcodegen/__init__.py
@@ +235,5 @@
>             file.
>          3. If an output file is missing, ensure it is present by performing
>             necessary regeneration.
>          """
> +        from Codegen import ExtraChangedFiles

This should be at module-level, not function-level.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Peter, this is causing people to need to clobber... Do you mind driving this in?

Comment 6

[Jonathan Kew [:jfkthame]]

<ping>

This can result in a fair bit of wasted time (and a degree of frustration...), it'd be awesome if we could get it resolved.

Comment 7

[Peter Van der Beken [:peterv]]

I'll try to pick this up again.