[Webkit-unassigned] [Bug 16401] [GTK] GObject/C DOM binding

Tue Sep 16 13:02:37 PDT 2008

https://bugs.webkit.org/show_bug.cgi?id=16401

------- Comment #48 from lkcl at lkcl.net  2008-09-16 13:02 PDT -------
> Can you please name the generated files with a format that matches
> our coding style?  "GdomBarInfo.h" doesn't match our naming conventions,
> and should probably be GDOMBarInfo.h.

 someone make a firm decision, tell me what to do,
 i'll go with it.  alp recommended gdom-xxx-xxx-xxx.h

> If these files are to be part of the WebKit GTK API

they're not part of the webkit gtk api.
they're entirely independent of the GTK api.  they _happen_ to be _used_ by
the GTK api, as a first example usage - and i am investigating how the QT
api should use them, too (pending someone writing a qt bindings).

they're "glib" bindings, not "gtk" bindings.

creation of a Glib object around both WebCore::Frame and WebCore::Page
will allow their use in other widget sets.

> Alp may be able to provide suggestions about what
> the right naming scheme is here.

 yep - he did - but they break the webkit coding conventions.

> The changes in FrameLoaderClientGtk.cpp look bogus.

 without that simple fix, any frames opened up in
 any page will instantly cause a segfault.

> Why are they in this patch?

because i need to get something working and useable,
the patch is 7,000 lines long, i'm too low on disk space
to have several copies of webkit checked out using git
to manage several revisions of the patch - one to send
to you and one to have something that actually "works".

it's the "working solution". nasty as hell, but it works.
if someone can fix #20403 i won't need that nasty fix.

> Why does GDOMObject.h have a commented out 'namespace WebCore'?

i believed it necessary at some point in the development cycle,
and was oscillating between it being there and not.

it's gone.

>  Why does it have a function named get_gobj_type which
> doesn't match our naming conventions?

pick a name - any name, i'll use it.

> The includes in webkitwebframe.cpp should be sorted per our style conventions.

done.  i think.

> They also shouldn't have the "gdom/" prefix.

 yeahh, i was going to convert to <gdom/NNNNN.h> to make it
 unequivocably clear that the glib bindings have absolutely
 nothing to do with being part of the gtk port.

>  The private header files should
be have Private as a suffix to match the convention used elsewhere in the
project (PrivateGdomDOMWindow.h -> GDOMWindowPrivate.h).  It's not clear to me
why some of the files have "Int" in their name.  Is that short for "Internal"? 

 yep.

> If so, the file should use that as a suffix in place of Private, such as
GDOMWindowInternal.h.

 ok. ... ahhh ... wait - i think that interferes with the pattern matching
 in WebCore/GNUMakefile.am - specifically:

    DerivedSources/gdom/PrivateGdom%.h: DerivedSources/gdom/Gdom%.cpp;

 etc.

 if someone can come up with a suitable pattern-match that works,
 without make thinking that the "*Internal*" files have make rules,
 and can come up with a suitable match also for the "*Private*.h"
 rules, then the names of the files can change.

> There are commented-out printf's in webkitwebevent.cpp which shouldn't be
> there.

gone.

> PrivateIntGdomEventTargetNode.h lacks a license header,

 whoops, sorted.

> and the Apple Inc. copyright line lacks the "All rights reserved." note that should be present. 

 then that means that somewhere there is another file
 missing it, from which the text was cut and paste.
 in amongst two weeks of very intense programming, i could
 not say which one it is.

> The file also uses strange formatting for the include guard.  Please see
> existing code for an example of the style that should be used.

it's auto-generated, it's perl, and it's easier to append "__INT_" than it is
to write yet another perl function which makes the perl code - already
unreadable - yet more complex.

if you _really_ want me to make the auto-generator more complex,
please state so clearly and i will be happy to do it.

> The code in GStringConvert.h lacks a license header.

added.

>  I'm not sure what the
> point of the GStringConvert class is.  It seems like it could easily be
> implemented using a simple overloaded function rather than the strange class +
> macro combination. 

 huh.  so it could :)

> The "extern C" around a C++ class doesn't make a lot of
sense either.

 yep - and it's not needed, either.  at some point, there was a bit of
 a mess involving extern "C"s... :)

>  The header includes in this file also need sorted.

 done

> Do the values for FEATURE_DEFINES_GDOM and FEATURE_DEFINES_JAVASCRIPT ever
differ?

 most definitely they do.

>  If not, is there any point in having both?

 most definitely there is.

 1) LANGUAGE_GOBJECT=1

 look at which functions are "custom"; look at which functions in
 the idls are defined for objc and which for 

 2) SVG.

 see #20586.  i'm _not_ adding svg supoprt until this patch is accepted.
 it's far too much additional work.

> GDOMHTMLElementWrapperFactory.cpp says that "THIS FILE IS DERIVED FROM AN
> AUTOMATICALLY GENERATED FILE."  what file is it derived from, and why is it
> being checked in if it is a generated file?

because it's not an auto-generated file.

if someone else - i.e. not me - would like to write some
perl code or other code which auto-generates that file,
they are welcome to do it.  i'm not a perl coder, but i'm
very good at cut/paste from examples of perl code.

> GDOMBinding.cpp has some FIXME's, TODO's and a
> lot of commented-out code that
> should be cleaned up before this can be landed.

 i'm leaving the TODOs in for future reference, for
 another patch, for additional functionality e.g. SVG.

 i _can_ remove the remainder of the FIXMEs and
 TODOs if you like but that would mean that future
 developers will find it more difficult to track down
 issues.

> Some of the IDL file changes do not look correct.
> I would think that the GObject bindings would want to match
> the behaviour of the Obj-C DOM bindings
> rather than JS in the cases that they differ.
> Many of these instances are due
> to behavioral peculiarities of JS, especially attributes such as
> ConvertNullToNullString and toString functions.

i'm.... i'll be absolutely brutally honest, here: i'm really
not ... how do i put this best...

imagine that there is a good way to say "no, i'm not going
to make any further changes, additions or enhancements or
even compliance to standards" without causing offense, and
then taking that as being said - repeatedly and clearly -
throughout the next few sentences.

i've reached my limit - unpaid work-wise - on amount of time and
effort to spend on adding extra functionality.

if you can find some money, somewhere, i'll do the additional work.

otherwise, the only circumstances under which i will make additional
changes is if there is a serious bug.  memory leak, simple coding
standards changes, style conventions etc - i've no problem with that.

i'm missing the use of "Location" property setting (because it's
a custom function) and i'm not even adding _that_, and i need it
in pyjamas-desktop, and i'm _still_ not going to add it, until
after this patch goes through.

a 7,500 line patch is enough.

i've done free software projects which resulted in nearly 150,000
lines of additional code being added - three years work, entirely
wasted (because of people's egos).  i'm not doing that again.

so - no more changes.  what's there "works".  if it's not
javascript standards compliant (because i #ifdef LANGUAGE_GOBJECT'd
in a function called open(CString) when LANGUAGE_JAVASCRIPT
does open(WebCore::class) - tough.  it works, it avoided many issues
which can be fixed...

_later_.

not now.

> All in all, this is looking like a good start,

 thanks.

> but it needs a lot more polish
> and refinement before it can be landed. 

 that's ok.  extra functionality, changes to various workarounds
 which can be planned for later patches - not ok.  sorry.

> I've primarily commented on style issues.

 very helpful, thank you.  i believe i've dealt with most
 of them as i've written this, and left 

> Hopefully Sam will be able to take a look at the
> implementation details once the patch is cleaned up
> and easier to follow, and Alp and other
> GTK developers can provide feedback on the new API.

 yaay.

 well, unfortunately, the auto-generator is always going
 to be a bit of a mess: i'm not a perl programmer, as i
 mentioned above, but i'm extremely good at cut/paste
 and "getting results" through extremely rapid development
 cycles.

 i'm sorry about the extra functionality thing.  i'm
 working on this for a personal project - unfunded - and
 am under extreme financial pressure.  many of the decisions
 made can be revisited and refined - but if it came to
 a choice between "significant extra functionality needed
 to land the patch" and "walking away", i walk away.
 there's enough extra work on the list already: i'm holding
 back _several_ bug reports and features until it's landed,
 simply because there's so much involved, i don't want to
 riddle the webkit database with stuff that could change.

 landing this patch - with functionality "as-is" - means
 that there's a stake in the ground to move forward from.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.