[Webkit-unassigned] [Bug 16401] [GTK] GObject/C DOM binding
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jul 12 12:57:58 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=16401
--- Comment #217 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> 2009-07-12 12:57:55 PDT ---
(In reply to comment #194)
> (From update of attachment 29670 [details])
> I concur with Mark's review, this is not ready to be landed. I have gone
> through the CodeGenerator and picked out a bunch of issues that really stick
> out, but in general, I can't really review this as is. I believe the best way
> forward is for you to pick one file (say Node.idl) and get code generation
> working for it or a subset of it.
that was what i was originally asked to do, back in .... mmmm... september.
it was already too late, by then, and it's definitely too late now. adam's
reply _also_ says "no".
the time for "one file" (Node.idl) was approximately 24 hours after i started,
in early august. but even then, as i looked at the dependencies it became
abundandly clear that just one file wasn't going to cut it.
so - we're well beyond that now. going "backwards" isn't going to happen.
i realise however that you do need to review this, in some way. and, given
that it's too big to review all at once, you _do_ need to have some level
of confidence that it actually works - and get a handle on it.
to do that, i recommend that you install pywebkitgtk (patched version)
and perhaps even try running pyjamas-desktop.
what you will then have is a code-base from which you can go "dang!"
and can play around.
also, i wrote a small (well, quite sophisticated) adapted version of
GtkLauncher:
http://lkcl.net/webkit/main.c
which you can also play with simply by replacing the existing GtkLauncher's
main.c
> The stripped down version of the code
> generator that can generate this one file will be a great jumping off point. I
> know this seems like a step backwards, but the current state of the patch is
> really not reviewable as is due to a lack of clarity in the code. The single
> patch is trying to do too many things at once.
it's _successfully_ doing many things at once, which you can demonstrate
to yourself by installing pywebkitgtk and pyjamas-desktop.
it's not my fault that this code hasn't been written at the same time as
the other code-generators (which have copyright timestamps dating back
to 2006).
so - forward it is.
> Here are some comments:
>
> > + if ($attribute->signature->extendedAttributes->{"Custom"} ||
> > + $attribute->signature->extendedAttributes->{"CustomGetter"} ||
> > + $attribute->signature->extendedAttributes->{"CustomSetter"}) {
> > + return 1;
> > + }
>
> [Custom] and related attributes are only used for implementing JS specific
> quirks and should not be needed for GObject bindings.
should not, but have been. detailed reply here:
https://bugs.webkit.org/show_bug.cgi?id=16401#c215
short answer: they stay until each of the custom attributes can be
reviewed on a case-by-case basis.
> > + if ($prop_type eq "stylesheets::MediaList" or
> > + $prop_type eq "CSSStyleDeclaration" or
> > + $prop_type eq "CSSVariablesDeclaration" or
> > + $prop_type eq "EventListener" or
> > + $prop_type =~ /Constructor$/ or
>
> Constructors are a JS only idiom.
i'm yet to be completely convinced, but am slowly coming round to it, that
constructors should not be needed [in glib/gobject bindings].
some of the DOM objects are _completely_ inaccessible.
XPathValidator is totally isolated and cannot be accessed, but mark
kindly pointed out a couple of weeks ago that there is a Document-based
access to the same functionality.
so that one's sorted.
however, TextDocument and XMLDocument are still utterly isolated.
... and yes, we have a pyjamas-desktop example which is still broken
because we can't create an XML Document, because there's no function
to access it.
also, because a TextDocument cannot be created, XMLHTTPRequest has had
to have its "open" function take a string parameter, and this alone
warrants the #define to create a separate IDL #ifdef for Gobject bindings.
so there is a lot left unanswered, and until it's answered for,
i'm reluctant to go ripping out references to "constructors" until
such time as it has been proven to my satisfaction that they are not
needed. at all.
> > +
> > +sub UsesManualToJSImplementation {
>
> ??? How is this related to GObject support?
not being funny or anything, but the answer is: don't know, and don't care.
much of this code was developed on a evolutionary-style rapid development
cycle that you will have difficulty believing in.
for eight weeks solid, i was making approximately 150 to 300 modifications
to CodeGeneratorGObject.pl and other source files _per day_, with over 250
simultaneous vi editor sessions open.
this development style, of "trusting" what i could cut and paste from
dozens of different sources, seems to have worked (as you can see from
pyjamas-desktop).
so - much of the code is, and will remain, "unexplained", like genetic code.
the summary of all that is, as i said: i don't know, and you shouldn't
care. if you do care, try taking it out, and see what happens. that's
what i did, and i'm through with doing that [cycle of development],
unless someone can show me a damn good justifiable reason for doing so.
> > + my $type = shift;
> > +
> > + # skipped out CanvasPixelArray - see JSCanvasPixelArrayCustom.cpp for why.
> > + return 1 if $type eq "Node" or $type eq "Document" or $type eq "HTMLCollection" or
> > + $type eq "SVGPathSeg" or $type eq "StyleSheet" or $type eq "CSSRule" or $type eq "CSSValue" or
> > + $type eq "Event" or $type eq "Element" or $type eq "Text";
> > + return 0;
> > +}
> > +
> > +sub WritePrivateHeader {
> > + my $object = shift;
> > + my $class = shift;
> > +
> > + my $interfaceName = $class->name;
> > + my $className = GetClassName($interfaceName);
> > + my $filename = "$outputDir/" . $className . "Private.h";
> > + my $guard = uc(decamelize($className)) . "_PRIVATE_H";
> > + my $parentClassName = GetParentClassName($class);
> > +
> > + my $hasLegacyParent = $class->extendedAttributes->{"LegacyParent"};
>
> LegacyParent is a JS only idiom.
see above. don't know don't care. it just works. it ain't broken.
feel free to fix it [later].
> > + my $hasRealParent = @{$class->parents} > 0;
> > + my $hasParent = $hasLegacyParent || $hasRealParent;
> > +
> > + open(PRIVHEADER, ">$filename") or die "Couldn't open file $filename for writing";
> > +
> > + print PRIVHEADER split("\r", $licenceTemplate);
> > + print PRIVHEADER "\n";
> > +
> > + my $text = << "EOF";
> > +#ifndef $guard
> > +#define $guard
> > +
> > +#include <glib-object.h>
> > +#include <webkit/${parentClassName}.h>
> > +#include "${interfaceName}.h"
> > +EOF
> > +
> > + print PRIVHEADER $text;
> > +
> > + print PRIVHEADER map { "#include \"$_\"\n" } sort keys(%hdrPropIncludes);
> > + print PRIVHEADER "\n" if keys(%hdrPropIncludes);
> > +
> > + $text = << "EOF";
> > +typedef struct _${className} ${className};
> > +
> > +namespace WebKit {
> > + ${className} *
> > + wrap${interfaceName}(WebCore::${interfaceName} *coreObject);
> > +
> > + WebCore::${interfaceName} *
> > + core(${className} *request);
> > +
> > +EOF
> > +
> > + print PRIVHEADER $text;
> > +
> > + if (${interfaceName} eq "DOMWindow" || (!$hasParent or $class->extendedAttributes->{"GenerateToJS"}) and
> > + !UsesManualToJSImplementation($interfaceName)) {
> > + $text = << "EOF";
>
> What is this trying to do?
that question really needed to be asked in september of 2008, some eight
months ago, when i had a full working knowledge of this code in my head.
if you want an answer now, you'll need to experiment (like i did).
> > +sub Uniquify {
> > + my $name = shift;
> > + my $avoid = shift;
> > + if ($name eq $avoid) {
> > + $name = $name . "2";
> > + }
>
> Please explain this.
i don't know what it is, and it's not in the patch that i'm going to
submit after reading these comments.
> > + if($attribute->signature->extendedAttributes->{"ConvertFromString"}) {
>
> This is a JS idiom.
yes. i found that it got in the way, and i appear to have deleted
it from the latest version.
it wasn't until i started delving into pyjamas-desktop in a heavy-duty
way that i found that some functions _require_ NULL etc. and that these
things got in the way.
so - out they went.
seemed to work, too, which is good :)
> > + # TODO: Add other conversion functions for different types. Current
> > + # IDLs only list longs.
> > + if($gtype eq "long") {
> > + $convert_fn = "";
> > + $post_convert_fn = ".toInt()";
> > + } else {
> > + die "Can't convert to type ${gtype}.";
> > + }
> > + }
> > +
>
> > + if ($gtype eq "int") {
> > + $txt_install_prop = << "EOF";
> > + G_MININT, /* min */
> > + G_MAXINT, /* max */
> > + 0, /* default */
> > +EOF
> > + } elsif ($gtype eq "boolean") {
> > + $txt_install_prop = << "EOF";
> > + FALSE, /* default */
> > +EOF
> > + } elsif ($gtype eq "float") {
> > + $txt_install_prop = << "EOF";
> > + G_MINFLOAT, /* min */
> > + G_MAXFLOAT, /* max */
> > + 0, /* default */
> > +EOF
> > + } elsif ($gtype eq "double") {
> > + $txt_install_prop = << "EOF";
> > + G_MINDOUBLE, /* min */
> > + G_MAXDOUBLE, /* max */
> > + 0, /* default */
> > +EOF
> > + } elsif ($gtype eq "uint64") {
> > + $txt_install_prop = << "EOF";
> > + 0, /* min */
> > + G_MAXUINT64, /* max */
> > + 0, /* default */
> > +EOF
> > + } elsif ($gtype eq "ulong") {
> > + $txt_install_prop = << "EOF";
> > + 0, /* min */
> > + G_MAXULONG, /* max */
> > + 0, /* default */
> > +EOF
> > + } elsif ($gtype eq "long") {
> > + $txt_install_prop = << "EOF";
> > + G_MINLONG, /* min */
> > + G_MAXLONG, /* max */
> > + 0, /* default */
> > +EOF
> > + } elsif ($gtype eq "uint") {
> > + $txt_install_prop = << "EOF";
> > + 0, /* min */
> > + G_MAXUINT, /* max */
> > + 0, /* default */
> > +EOF
> > + } elsif ($gtype eq "ushort") {
> > + $txt_install_prop = << "EOF";
> > + 0, /* min */
> > + G_MAXUINT16, /* max */
> > + 0, /* default */
> > +EOF
> > + } elsif ($gtype eq "uchar") {
> > + $txt_install_prop = << "EOF";
> > + G_MININT8, /* min */
> > + G_MAXINT8, /* max */
> > + 0, /* default */
> > +EOF
> > + } elsif ($gtype eq "char") {
> > + $txt_install_prop = << "EOF";
> > + 0, /* min */
> > + G_MAXUINT8, /* max */
> > + 0, /* default */
> > +EOF
> > + } elsif ($gtype eq "string") {
> > + $txt_install_prop = << "EOF";
> > + "", /* default */
> > +EOF
> > + } elsif ($gtype eq "object") {
> > + $txt_install_prop = << "EOF";
> > + WEBKIT_TYPE_${propGType}, /* gobject type */
> > +EOF
> > + }
> > + push(@txt_install_props, $txt_install_prop);
> > + $txt_install_prop = << "EOF";
> > + (GParamFlags)${gparamflag}));
> > +
> > +EOF
>
> This should probably be done in a more generic way using a hash.
yes. that would be nice. it should go on the TODO list, for after
when the patch has been landed. right now however, what's there
works.
so, many apologies sam - many of these questions are far far too late,
and others raise some nice issues which will help improve the maintainability
at a later date. and, unfortunately, some are out-of-date.
overall, however, there isn't anything that you've raised which warrants
any changes. they all fall into "too unsafe to consider doing;
were made by adam and so are irrelevant now; too late to answer".
the development process isn't "over", not by a long shot, but what's
there can be proven, empirically and indirectly, to "work", and, given
the level of complexity and the THREE levels of indirection between
the code and its [current and only] test environment, pyjamas-desktop,
fiddling with the code generator to do "tidyups" will prove to be a
significantly bad / time-consuming idea.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list