Closed
Bug 1470325
Opened 6 years ago
Closed 6 years ago
WebIDL `interface FooBinding` generated code name-conflicts with generated `interface Foo` code
Categories
(Core :: DOM: Bindings (WebIDL), enhancement, P3)
Core
DOM: Bindings (WebIDL)
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: jgilbert, Assigned: jgilbert)
References
Details
Attachments
(3 files)
The WebGPU sketch IDL includes `WebGPUBuffer` and `WebGPUBufferBinding` among other pairs.
Assignee | ||
Comment 1•6 years ago
|
||
As I posted to dev-platform, I took a stab at switching from dom::%sBinding to dom::bindings::%s, and I think I'm almost there aside from a testcase that has a New And Exciting name collision: > 1:54.21 In file included from /Users/jgilbert/dev/mozilla/objs/gecko5/js/xpconnect/src/Unified_cpp_js_xpconnect_src0.cpp:11: > 1:54.21 In file included from /Users/jgilbert/dev/mozilla/gecko5/js/xpconnect/src/Sandbox.cpp:59: > 1:54.21 In file included from /Users/jgilbert/dev/mozilla/objs/gecko5/dist/include/mozilla/dom/UnionConversions.h:69: > 1:54.21 /Users/jgilbert/dev/mozilla/objs/gecko5/dist/include/mozilla/dom/TestInterfaceJSMaplikeSetlikeIterableBinding.h:744:78: error: must use 'class' tag to refer to type 'TestInterfaceMaplike' in this scope > 1:54.21 Set(mozilla::dom::TestInterfaceMaplikeObject* self, const nsAString& aKey, TestInterfaceMaplike& aValue, ErrorResult& aRv); > 1:54.21 ^ > 1:54.21 class > 1:54.21 /Users/jgilbert/dev/mozilla/objs/gecko5/dist/include/mozilla/dom/TestInterfaceJSMaplikeSetlikeIterableBinding.h:660:11: note: class 'TestInterfaceMaplike' is hidden by a non-type declaration of 'TestInterfaceMaplike' here > 1:54.21 namespace TestInterfaceMaplike { > 1:54.21 ^ Since this declaration is within namespace `dom::bindings::TestInterfaceMapLikeObject`, `TestInterfaceMapLike` either needs a class/typename annotation or to be properly namespaced. I played around with Codegen.py a bunch but couldn't see an easy way to do this. Thoughts? Current changes: https://github.com/jdashg/gecko-cinn/commits/bindings-namespace
Flags: needinfo?(kyle)
Updated•6 years ago
|
Flags: needinfo?(kyle)
Priority: -- → P3
Comment 2•6 years ago
|
||
Oops. Forgot to unmark the ni? resolve on this. I'm still trying to figure out what the best way to do this is so re-ni?'ing myself until that's done.
Flags: needinfo?(kyle)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
Well, that's good news: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61bb683d37b27b518d1cd6271428a1e0e67a4643&selectedJob=184842574
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8987725 [details] Bug 1470325 - Update Codegen.py to emit Foo_Binding instead of FooBinding. - https://reviewboard.mozilla.org/r/252984/#review259760 lgtm but I want to get bz's input since the namespace idea was already discussed with him. I'm fine with underscores and think they'll be less confusing than the namespaces and possible collisions, just want to make sure I'm not missing anything. We'll probably need to update the WebIDL MDN page too, once it lands. ::: dom/bindings/Codegen.py:1747 (Diff revision 1) > > def objectMovedHook(descriptor, hookName, obj, old): > assert descriptor.wrapperCache > return fill(""" > if (self) { > - UpdateWrapper(self, self, ${obj}, ${old}); > + UpdateWrapper(self, self, ${obj}, ${old}); nit: Accidental whitespace removal?
Attachment #8987725 -
Flags: review?(kyle) → review+
Updated•6 years ago
|
Flags: needinfo?(kyle)
Attachment #8987725 -
Flags: review?(bzbarsky)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•6 years ago
|
||
So the problem with the dom::bindings::Foo proposal was that then you get name collisions with the dom::Foo implementation class when actually inside the dom::bindings namespace? For the specific example of TestInterfaceMapLike, we're running into a problem because we use the same code for example codegen and callback codegen and the former wants pretty-looking types so we strip off "mozilla::dom::" when we can (see bug 1000675). That's pretty simple to fix and limit the stripping to example codegen if we want.
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8987725 [details] Bug 1470325 - Update Codegen.py to emit Foo_Binding instead of FooBinding. - https://reviewboard.mozilla.org/r/252984/#review259800 r=me. I guess this is fewer chars than "bindings::" and definitely won't cause name collisions... until someone adds an interface with '_' in it, I guess.
Attachment #8987725 -
Flags: review?(bzbarsky) → review+
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8987726 [details] Bug 1470325 - s/FooBinding/Foo_Binding/g - https://reviewboard.mozilla.org/r/252986/#review259814
Attachment #8987726 -
Flags: review?(kyle) → review+
Comment 13•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9) > So the problem with the dom::bindings::Foo proposal was that then you get > name collisions with the dom::Foo implementation class when actually inside > the dom::bindings namespace? > > For the specific example of TestInterfaceMapLike, we're running into a > problem because we use the same code for example codegen and callback > codegen and the former wants pretty-looking types so we strip off > "mozilla::dom::" when we can (see bug 1000675). That's pretty simple to fix > and limit the stripping to example codegen if we want. Yeah, also having poked at it some while working on this, I'm not sure how obvious it'll be to other devs that you need something from the binding::X namespace now when using class X, versus having a completely different X_Binding class. X_Binding isn't exactly pretty (and does break with style in general), but it's slightly more obviously than binding::X, and hopefully more difficult to screw up.
Assignee | ||
Comment 14•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11) > Comment on attachment 8987725 [details] > Bug 1470325 - Update Codegen.py to emit Foo_Binding instead of FooBinding. - > > https://reviewboard.mozilla.org/r/252984/#review259800 > > r=me. I guess this is fewer chars than "bindings::" and definitely won't > cause name collisions... until someone adds an interface with '_' in it, I > guess. I'm hoping that a webidl interface 'Foo_Binding' wouldn't be acceptable web-style-wise, so we shouldn't have to deal with this, yeah?
Comment 15•6 years ago
|
||
> I'm hoping that a webidl interface 'Foo_Binding' wouldn't be acceptable web-style-wise
No guarantees of that, sadly. Especially for cases that are not web-exposed names like NoInterfaceObject interfaces, callbacks, dictionaries, etc.
Comment 16•6 years ago
|
||
Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba24316f5d84 Update Codegen.py to emit Foo_Binding instead of FooBinding. - r=bz,qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/a97feb8161b7 s/FooBinding/Foo_Binding/g - r=qdot
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ba24316f5d84 https://hg.mozilla.org/mozilla-central/rev/a97feb8161b7
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•