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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 16 09:47:08 PDT 2008


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





------- Comment #46 from mrowe at apple.com  2008-09-16 09:47 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.  If these files are to be part of the WebKit GTK API
then they should match the GTK naming convention, which I believe does not use
upper-case letters at all.  Alp may be able to provide suggestions about what
the right naming scheme is here.

The changes in FrameLoaderClientGtk.cpp look bogus.  Why are they in this
patch?

Why does GDOMObject.h have a commented out 'namespace WebCore'?  Why does it
have a function named get_gobj_type which doesn't match our naming conventions?

The includes in webkitwebframe.cpp should be sorted per our style conventions. 
They also shouldn't have the "gdom/" prefix.  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"? 
If so, the file should use that as a suffix in place of Private, such as
GDOMWindowInternal.h.

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

PrivateIntGdomEventTargetNode.h lacks a license header, and the Apple Inc.
copyright line lacks the "All rights reserved." note that should be present. 
The file also uses strange formatting for the include guard.  Please see
existing code for an example of the style that should be used.

The code in GStringConvert.h lacks a license header.  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.  The "extern C" around a C++ class doesn't make a lot of
sense either.  The header includes in this file also need sorted.

Do the values for FEATURE_DEFINES_GDOM and FEATURE_DEFINES_JAVASCRIPT ever
differ?  If not, is there any point in having both?

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?

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.

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.



All in all, this is looking like a good start, but it needs a lot more polish
and refinement before it can be landed.  I've primarily commented on style
issues.  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.


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



More information about the webkit-unassigned mailing list