| Summary: | gobject bindings need access to keyCode on KeyboardEvents! | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Luke Kenneth Casson Leighton <lkcl> |
| Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED LATER | ||
| Severity: | Normal | CC: | ap, eric, mrobinson, mrowe |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | PC | ||
| OS: | OS X 10.5 | ||
| Bug Depends on: | |||
| Bug Blocks: | 16401 | ||
| Attachments: | |||
|
Description
Luke Kenneth Casson Leighton
2009-07-19 16:08:07 PDT
Created attachment 33065 [details]
enables access to keyCode and charCode properties and W3C-compliant initKeyboardEvent function under gobject language bindings
Please look back into the history of this line. I expect this was added to avoid Obj-C generation and shoudl be changed to !objc now. (In reply to comment #2) > Please look back into the history of this line. I expect this was added to > avoid Obj-C generation and shoudl be changed to !objc now. ack. same as #27437 then, basically. willdo. ah... looking at this a little closer, i believe that the reason why i commented out the initKeyboardEvent function on GOBJECT bindings would have been because you can't, in c, have two functions with the same name. and the gobject bindings auto-generator would have created two gdom_keyboard_event_init_keyboard_event() functions, because of the two initKeyboardEvent entries in the IDL file. so i simply #ifdef'd one of them out. stupidly, i actually managed to #ifdef out keyCode and charCode as well! so, duh, thank you for drawing my attention to this! anyway - i'll submit a revised patch which has !LANGUAGE_OBJC around keyCode and charCode.... errr... are you sure? i'll submit it for your attention, you let me know if it should be !LANGUAGE_OBJC or !LANGUAGE_JAVASCRIPT ok? Created attachment 33193 [details]
comments out only the 2nd initKeyboardEvent function on LANGUAGE_GOBJECT
probably a good idea to look at #27437 this likely needs discussion, i'm happy to submit necessary patches to keep Obj-C happy but don't have the expertise to say what is what, here - just let me know what needs to be done. i do know however that you can't have two versions of the same c function which is why one of the initKeyboardEvent functions _has_ to be #ifdef'd out.
I think this ties in to the more general issue of how to handle overloaded methods in a C-based bindings. Perhaps we should decide on a more general solution to this problem rather than #if'ing throughout the IDLs. Comment on attachment 33193 [details] comments out only the 2nd initKeyboardEvent function on LANGUAGE_GOBJECT ChangeLog name: +2008-11-30 lkcl <lkcl@lkcl.net> Tabs in ChangeLog. I think Mark wanted a different solution for c bindings in general, but in this change looks fine overall to me. r- for the ChangeLog issues. (In reply to comment #7) > (From update of attachment 33193 [details]) > ChangeLog name: > +2008-11-30 lkcl <lkcl@lkcl.net> > > Tabs in ChangeLog. yes. sorry. found that out a bit late - have corrected most of them in the [15] patches submitted, must have missed this one. will redo. > I think Mark wanted a different solution for c bindings in general, in each instance where the discussion comes up, i mention that there is no issue. the current solution has been proven to work, and, because of the violation of the W3C DOM spec in this case (to very sensibly add support for Alt-Gr) this is pretty much the _only_ case where the [clashing] function needs to be #ifdef'd out. more details have been given in other bugreports [several times]. > but in this > change looks fine overall to me. great. > r- for the ChangeLog issues. will fix. Created attachment 33963 [details]
fixes tabs in CHANGELOG, alters date from original patch creation date (2008) to current date
Comment on attachment 33963 [details]
fixes tabs in CHANGELOG, alters date from original patch creation date (2008) to current date
Assuming Mark's concerns with this general approach were resolved (as you noted above), then this looks fine to me.
As I've outlined in bug 27435, my concerns about the handling of overloaded methods have yet to be addressed. (In reply to comment #8) > > I think Mark wanted a different solution for c bindings in general, Yes, I'm after a more general solution than picking a single function and #if'ing the rest out. As I've mentioned in bug 27435, there are other situations in which this is necessary. Since a general solution would obviate the need for #if'ing in IDL files I would prefer to see it explored before we go landing changes that add #if's. Comment on attachment 33963 [details]
fixes tabs in CHANGELOG, alters date from original patch creation date (2008) to current date
r- per mark's above comments.
I think you should try to enlist one of the longer-term WebKitGtk contributers to help you with these patches. You're hitting a lot of road-blocks at once. Much of that is likely due to uploading 15 "first time" patches at once. So you end up hitting similar combinations of starter-mistakes with each. In this case, Mark is asking for a re-architecture of how we do autogeneration. That's a little much to expect of a new contributer, but certainly possible. Especially if you engage other senior members of the project. In this case, it may be too much to ask, because our auto-gen system is a pile of ugly perl which several of us have wanted to re-write for years. Eventually one of us will get around to re-writting it in c++ or python. I think it would be a relatively easy change to add some sort of ALLOW_ARGUMENT_OVERLOADS or similar define which encapsulated !JAVASCRIPT and GOBJECT. That wouldn't involve re-writing the world, but it also wouldn't be poluting our IDL files more. (In reply to comment #14) > I think it would be a relatively easy change to add some sort of > ALLOW_ARGUMENT_OVERLOADS or similar define which encapsulated !JAVASCRIPT and > GOBJECT. That wouldn't involve re-writing the world, but it also wouldn't be > poluting our IDL files more. Except for the small issue that JavaScript DOM bindings support overloaded functions. (In reply to comment #15) > (In reply to comment #14) > > I think it would be a relatively easy change to add some sort of > > ALLOW_ARGUMENT_OVERLOADS or similar define which encapsulated !JAVASCRIPT and > > GOBJECT. That wouldn't involve re-writing the world, but it also wouldn't be > > poluting our IDL files more. > > Except for the small issue that JavaScript DOM bindings support overloaded > functions. Sounds like either I have the ! reversed in my example text above, or I'm misunderstanding the issue. Are you saying that it would not be good/possible/sufficient to replace the proposed !JAVASCRIPT && GOBJECT && ... with a single define? (In reply to comment #16) > Sounds like either I have the ! reversed in my example text above, or I'm > misunderstanding the issue. Are you saying that it would not be > good/possible/sufficient to replace the proposed !JAVASCRIPT && GOBJECT && ... > with a single define? In some places that overloading is used, such as XHR, the IDL contains a single version of the method that is annotated with [Custom] to indicate that a custom JS implementation of the binding for that method will be provided. That custom implementation typically introspects the arguments that were provided and dispatches to the appropriate method on the wrapped DOM object. In most cases, functions that are #if'd in the IDLs to exclude them from JavaScript are not done so for a reason related to overloading, so changing them to be wrapped by a define related to overloading would not be appropriate. In the case of initKeyboardEvent it's not clear why one variant is excluded from JavaScript. The variant without altGraph was added in <http://trac.webkit.org/changeset/16277/trunk/WebCore/dom/KeyboardEvent.idl> by Sam without much explanation. (In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > I think it would be a relatively easy change to add some sort of > > > ALLOW_ARGUMENT_OVERLOADS or similar define which encapsulated !JAVASCRIPT and > > > GOBJECT. That wouldn't involve re-writing the world, but it also wouldn't be > > > poluting our IDL files more. > > > > Except for the small issue that JavaScript DOM bindings support overloaded > > functions. > > Sounds like either I have the ! reversed in my example text above, or I'm > misunderstanding the issue. Are you saying that it would not be > good/possible/sufficient to replace the proposed !JAVASCRIPT && GOBJECT && ... > with a single define? see https://bugs.webkit.org/show_bug.cgi?id=27435#c29 and previous. overloaded functions = same function with different numbers of arguments. think: "the usual c / c++ problem, basically". XULrunner have run into exactly the same problem: https://bug459452.bugzilla.mozilla.org/attachment.cgi?id=342670 https://bugzilla.mozilla.org/show_bug.cgi?id=502234 except there, the users have c++ as the native API, and the python bindings are direct to that. any mess-ups are due to implementation flaws (as can be seen from the above two bugs) and precedence given to javascript rather than any particularly good reason. MSHTML does two tricks: * adds rolling numbers onto the overloaded functions. burden is passed to users to work out which one to use. * and, also, i thiiink what they do is create different COM interfaces and then create a coclass merging them all together. IDispatch can cope with telling users about the interfaces and their functions (at run-time) and in this way, dynamic use of the MSHTML interface simply passes the burden onto the users. both of which are greeeaat. but. seriously. see https://bugs.webkit.org/show_bug.cgi?id=27435#c29 - the conclusion is that i don't believe it's that big a deal. even if what small API changes are required happen in 6 months or a year. (In reply to comment #14) > I think you should try to enlist one of the longer-term WebKitGtk contributers > to help you with these patches. *sigh*. sadly, they've all said "we'd love to, but we don't have time". please don't spend time reading any of the 300 comments in #16401, it won't help :) > You're hitting a lot of road-blocks at once. well, that's better than hitting one and only one, for eight months straight. > Much of that is likely due to uploading 15 "first time" patches at once. So > you end up hitting similar combinations of starter-mistakes with each. yep. caught lots of them. cancelled review on ones where i remember. > In this > case, Mark is asking for a re-architecture of how we do autogeneration. That's > a little much to expect of a new contributer, but certainly possible. *sssss*... i'm not the person to ask to do that. not on something that's at the core of the project. and, also, i think it would be a good idea to get the gobject bindings in, first, _before_ making any significant changes. in that way, all the bindings can be considered - and taken into consideration - all at once. if one of them is left out, then... > In this case, it may be too much to ask, because our auto-gen system is a pile > of ugly perl which several of us have wanted to re-write for years. i did as best i could a hybrid cut/paste job. > Eventually > one of us will get around to re-writting it in c++ or python. > > I think it would be a relatively easy change to add some sort of > ALLOW_ARGUMENT_OVERLOADS or similar define which encapsulated !JAVASCRIPT and > GOBJECT. That wouldn't involve re-writing the world, but it also wouldn't be > poluting our IDL files more. mmm.... can't give an opinion - bit tired, and also i don't quite follow [need context]. i can say however that the mozilla approach is to simply add extra IDL attributes, and they have a "no" prefix to indicate !. e.g. [noscript] means "exclude from both javascript and xpcom when generating bindings" which they freely admit is a broken idea. as you can see from this: https://bug459452.bugzilla.mozilla.org/attachment.cgi?id=342670 this is an attempt to fix the issue by adding an [optional_arg] IDL attribute, and then the number of arguments is inserted as an additional argument into the c++ code. i don't like it. and, as mentioned here: https://bugs.webkit.org/show_bug.cgi?id=27435#c29 i believe that this whole issue is moot, i.e. from a "practical" software engineering perspective, a whole boat-load of complexity could end up getting added for very little extra benefit / ROI. evidence to the contrary, i _do_ subscribe to the "good enough" software engineering principle - it's just that with webkit-gobject and pyjamas-desktop the bar's a bit damn high. (In reply to comment #12) > (In reply to comment #8) > > > I think Mark wanted a different solution for c bindings in general, > > Yes, I'm after a more general solution than picking a single function and > #if'ing the rest out. As I've mentioned in bug 27435, there are other > situations in which this is necessary. [answered there] > Since a general solution would obviate > the need for #if'ing in IDL files I would prefer to see it explored before we > go landing changes that add #if's. * a general solution would require across-the-board changes [to CodeGenerator.pm], including to CodeGeneratorGObject.pm. if they're willing to do that. * that means that someone would need to look at code that's in webkit svn _and_ code that's not. if they're willing to do that. * it could be months before an updated solution is discussed and ready [if in fact one is needed at all - see #27435 answer] on balance, therefore, i would consider it a wiser move to tolerate #ifdefs, warts and all, land the CodeGeneratorGObject and _then_ look at all of the CodeGenerators all at once. if it's _really_ a problem to have an "un-finalised" c-based gobject API out there, then there are plenty of ways to stop people from using it whilst matters are discussed, whilst also not impeding people who are willing to use what's there, regardless. putting in a #ifdef DO_NOT_USE_GOBJECT_API_ONLY_AT_OWN_RISK_NOT_FINALISED_YET is the most obvious one. This isn't going to happen now, but maybe we can think about it when we rework the GObject DOM bindings. |