Change Notification constructor to take an IDL dictionary instead of a WebCore::Dictionary
Created attachment 278375 [details] Patch
Comment on attachment 278375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278375&action=review > Source/WebCore/ChangeLog:8 > + * Modules/notifications/DOMWindowNotifications.idl: Simplified conditionals. This change looks good, but it also looks like the GTK build failure is related to this. GTK has NOTIFICATIONS on but LEGACY_NOTIFICATIONS off. EFL has both off. GTK would probably compile without this change.
Comment on attachment 278375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278375&action=review >> Source/WebCore/ChangeLog:8 >> + * Modules/notifications/DOMWindowNotifications.idl: Simplified conditionals. > > This change looks good, but it also looks like the GTK build failure is related to this. GTK has NOTIFICATIONS on but LEGACY_NOTIFICATIONS off. EFL has both off. GTK would probably compile without this change. It’s true that this change broke the GTK build. The issue, though, is that the GObject bindings generation script doesn’t correctly handle #include generation when code uses [Conditional], not so much the particular configuration. EFL builds because it doesn’t generate GObject bindings. I guess I will roll that change out and land without it. I’m increasingly worried about how the Objective-C and GObject bindings are holding us back, for little benefit. There are lots of new DOM APIs that we can’t express in those bindings. I wonder if there is a way we can freeze them and make them less of a maintenance burden.
(In reply to comment #3) > I’m increasingly worried about how the Objective-C and GObject bindings are > holding us back, for little benefit. There are lots of new DOM APIs that we > can’t express in those bindings. I wonder if there is a way we can freeze > them and make them less of a maintenance burden. ObjC bindings are only accessible from WebKit1, right? Are they accessible from within the InjectedBundle? Why do we even have gobject bindings when gtk only supports WebKit2?
(In reply to comment #4) > ObjC bindings are only accessible from WebKit1, right? I think that's right. > Are they accessible from within the InjectedBundle? I don't think they are. > Why do we even have gobject bindings when gtk only supports WebKit2? We need to find out the answer to this. Who would know the answer?
(In reply to comment #5) > (In reply to comment #4) > > ObjC bindings are only accessible from WebKit1, right? > > I think that's right. > > > Are they accessible from within the InjectedBundle? > > I don't think they are. > > > Why do we even have gobject bindings when gtk only supports WebKit2? > > We need to find out the answer to this. Who would know the answer? KaL or mcatanzaro in cc? I thought they were exposed via InjectedBundle but I am not sure.
(In reply to comment #6) > KaL or mcatanzaro in cc? > I thought they were exposed via InjectedBundle but I am not sure. The GObject bindings are indeed exposed via InjectedBundle. They're used by many applications and they're a critical advantage over Chromium. We don't want to hold back your development, though. Are there other issues we need to address besides "GObject bindings generation script doesn’t correctly handle #include generation when code uses [Conditional]" (which sounds fixable)? Frankly, the bindings generation is rather arcane. Ideally there would be a quick and *easy* way to skip an entire problematic IDL file. Then you'd be able to proceed here without waiting for us, breaking only a subset of our bindings and not our entire build, and we (well, Carlos, hehe ;) could come along and fix the missing bindings later. Let's wait for Carlos to comment; he understands these bindings much better than I do.
(In reply to comment #7) > We don't want to hold back your development, though. Are there other issues > we need to address besides "GObject bindings generation script doesn’t > correctly handle #include generation when code uses [Conditional]" (which > sounds fixable)? Yes, there are lots of issues. Some of the up and coming issues will be support for default values for function arguments, optional arguments, improved handling for nullable arguments (I did a stopgap hack for this), enumeration types, dictionary types, callback function types, union types, promise types, and USVString. Also handling of new way of dealing with DOM exceptions. We are planning on making them part of the return value instead of a separate ExceptionCode in argument. > Frankly, the bindings generation is rather arcane. No question about it. > Ideally there would be a quick and *easy* way to skip an entire problematic > IDL file. Some variation on [Conditional] would be a good way to do that, I suppose. Might want an easy to do this for individual attributes and functions as well.
Committed r200607: <http://trac.webkit.org/changeset/200607>
(In reply to comment #8) > (In reply to comment #7) > > We don't want to hold back your development, though. Are there other issues > > we need to address besides "GObject bindings generation script doesn’t > > correctly handle #include generation when code uses [Conditional]" (which > > sounds fixable)? > > Yes, there are lots of issues. Some time ago we defined a set of DOM API that we consider "stable" and the rest is considered "unstable". We have a script that the bots run to detect API breaks in our stable DOM bindings API, they are usually changes in the type of parameters or methods that used to raise exceptions or the other way around. This made the maintenance of the DOM bindings easier for us, but also for the other ports that don't need to worry about breaking our bindings. It's ok for us if you break the bindings, or the API, as long as it builds. So, I think the best solution would be to define easy ways in our port to skip the generation of anything in the bindings, from a single method to the whole idl file. > Some of the up and coming issues will be support for default values for > function arguments, optional arguments, improved handling for nullable > arguments (I did a stopgap hack for this), enumeration types, dictionary > types, callback function types, union types, promise types, and USVString. Some of those features could be implemented in the GObject DOM bindings, I guess they will not affect our "stable" API. We just need to ensure you guys can work on that without having to worry about the GObject bindings. > Also handling of new way of dealing with DOM exceptions. We are planning on > making them part of the return value instead of a separate ExceptionCode in > argument. > > > Frankly, the bindings generation is rather arcane. > > No question about it. > > > Ideally there would be a quick and *easy* way to skip an entire problematic > > IDL file. > > Some variation on [Conditional] would be a good way to do that, I suppose. > Might want an easy to do this for individual attributes and functions as > well. The conditional issue sounds like a bug because we have code in our script to deal with conditional features.
Looking at the GTK build fix in r200614 (thank you very much Alex) it seems the problem was not in the bindings after all. In any case, I was thinking that we could add a build flag, something like ENABLE(GOBJECT_DOM_UNSTABLE_API) or something like that, that you can easily disable (just changing a #define in config.h instead of dealing with build files) if something is broken. Then, I'll fix the issue and enable it again.
(In reply to comment #10) > The conditional issue sounds like a bug because we have code in our script > to deal with conditional features. The problem in the GObject bindings handling of conditional features is that it makes the contents of the file conditional in the proper way, but does not make the #includes conditional in the proper way. CodeGeneratorJS.pm bindings has an approach to this that separate tracks the conditional for each include, but this is not done in CodeGeneratorGObject.pm. Sometimes I wish much more of the code was shared.
There is at least one file that has an #if GOBJECT driven by this issue and it was the same issue I ran into here.