Bug 27436 – gobject bindings need access to keyCode on KeyboardEvents!

• Status: RESOLVED
• Resolution: LATER
• Component: WebKit Misc.

[Luke Kenneth Casson Leighton]

this is part of the split of #16401 into smaller patches
as part of an agreement recommended by david.

take this lightly, please, but whoever thought that you could do language bindings without having access to keyCodes ... honestly, it's just so daft!  and, if there _are_ workarounds that get at the same information... honestly, why make developers' lives damn awkward.  just publish the same consistent API and be done with it, for goodness sake! :)

anyway: it can be seen from the IDL itself that the exposed initKeyboardEvent function doesn't match up with the W3C DOM spec, so any arguments made - and there have been several, by reviewers - that Webkit language bindings stick religiously to the W3C spec are moot.

[Luke Kenneth Casson Leighton]

Created attachment 33065 [details]
enables access to keyCode and charCode properties and W3C-compliant initKeyboardEvent function under gobject language bindings

[Eric Seidel (no email)]

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.

[Luke Kenneth Casson Leighton]

(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.

[Luke Kenneth Casson Leighton]

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?

[Luke Kenneth Casson Leighton]

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.

[Mark Rowe (bdash)]

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.

[Eric Seidel (no email)]

Comment on attachment 33193 [details]
comments out only the 2nd initKeyboardEvent function on LANGUAGE_GOBJECT

ChangeLog name:
[phone]  lkcl <[e-mail]>

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.

[Luke Kenneth Casson Leighton]

(In reply to comment #7)
> (From update of attachment 33193 [details])
> ChangeLog name:
> [phone]  lkcl <[e-mail]>
> 
> 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.

[Luke Kenneth Casson Leighton]

Created attachment 33963 [details]
fixes tabs in CHANGELOG, alters date from original patch creation date (2008) to current date

[Eric Seidel (no email)]

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.

[Mark Rowe (bdash)]

As I've outlined in bug 27435, my concerns about the handling of overloaded methods have yet to be addressed.

[Mark Rowe (bdash)]

(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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Mark Rowe (bdash)]

(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.

[Eric Seidel (no email)]

(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?

[Mark Rowe (bdash)]

(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.

[Luke Kenneth Casson Leighton]

(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.

[Luke Kenneth Casson Leighton]

(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.

[Luke Kenneth Casson Leighton]

(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.

[Martin Robinson]

This isn't going to happen now, but maybe we can think about it when we rework the GObject DOM bindings.