[Webkit-unassigned] [Bug 16401] [GTK] GObject/C DOM binding
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 12 21:42:03 PDT 2008
https://bugs.webkit.org/show_bug.cgi?id=16401
alp at nuanti.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #23367|review? |review-
Flag| |
------- Comment #28 from alp at nuanti.com 2008-09-12 21:42 PDT -------
(From update of attachment 23367)
This is looking good and needs a little more work before we can start to think
about getting it landed.
These are some of my comments from a first pass review, starting with the
trivial / easiest issues to fix.
Build fix:
The 'KJS' namespace was renamed to 'JSC' recently. Simple search-and-replace in
CodeGeneratorGObject.pm gets it building again.
Coding style:
void: No need for the 'void' in function prototypes like webkit_event_new(void)
except in publicly installed C headers. It doesn't matter if the auto-generated
C++ sources have the 'void' (since this lets you share the same prototype
string with the .h public headers and generated .cpp) but it shouldn't be used
in hand-written code.
includes: #include <glib-object.h> not #include "glib-object.h"
indentation: There are a few places (eg. webkitwebframe.cpp) where 4-space
indentation isn't adhered to. It's good to get into the habit of getting this
right whenever submitting code to the bug tracker so reviewers can get directly
to the technical stuff.
Trailing whitespace: There are needless spaces at the ends of lines in both
hand-written and auto-generated code that shouldn't be there.
Core API changes:
webkitevent.h/WebKitEvent: We try to avoid generic type names in the core
WebKit/GTK+ public API since 'WebKit.Event' will be ambiguous when 'Gdk.Event'
is imported in many OO language bindings. So this could become either
WebKitWebEvent, could go into the Gdom namespace or even somehow be merged with
GdomEvent (I haven't investigated enough to see if this last suggestion would
make sense though).
Special-cases for lower_case_conversion:
decamelize() is great, but sometimes we'll want to special-case certain
character sequences to avoid awkward function names. Right now we get
gdom_x_path_exception_to_string() but what we really want is
gdom_xpath_exception_to_string(). Some cases I noticed: XPath, WebKit,
gdom_htmlbr_element_get_type, gdom_htmld_list_element_get_type
_init functions vs. constructors:
There are some generated functions like gdom_wheel_event_init_wheel_event().
These should probably be mapped to GObject constructors, ie.
gdom_wheel_event_new().
G_PARAM_*:
webkitprivate.h defines WEBKIT_PARAM_READABLE and WEBKIT_PARAM_READWRITE
replacements for G_PARAM_READABLE and G_PARAM_READWRITE which tell GObject that
it doesn't need to copy the input strings to g_param_spec(). It's probably
worth reusing this (or copying equivalents into gdomprivate.h or similar).
Build system:
The large lists in these files should be auto-generated by the build system:
gdom.h
gdomdefines.h
GDOMHTMLElementWrapperFactory.cpp
(1) The GObject DOM build should be split out into a new automake intermediate
library separate from libwebkit_1_0_la and libWebCore_la and (2) the gdom
public headers should be installed under a new prefix in the destination
directory eg.:
libgdom_ladir = $(prefix)/include/webkit-1.0/gdom
libgdom_la_HEADERS = \
$(gdomgtk_h_api) \
WebKit/gtk/webkit/gdom.h
# Convenience libraries
noinst_LTLIBRARIES = \
libJavaScriptCore.la \
libWebCore.la \
libgdom.la
The rationale for this is twofold:
(1) It'll be helpful for both WebKit hackers and application developers to
immediately see the separation between the hand-written core WebKit/GTK+ API
(2) Other WebKit ports using GObject but not GTK+ or the WebKit/GTK+ API layer
will still be able to share the same DOM binding to some extent.
Private* C++ sources and headers:
As far as I can tell, the PrivateGdom*.cpp generated files are redundant. The
code generated into PrivateGdomElement.cpp could just as well go into
GdomElement.cpp for example, right?
The PrivateGdom*.h files also look fairly empty right now, but I imagine that
private headers for each DOM class could still end up being useful, depending
on how you go about merging the Private*.cpp implementations.
Getting rid of the Private*.cpp sources will help reduce compile time and
reduce the size of the lists of generated files in the build system. Do let me
know if I've got something wrong here -- there may be a good rationale for
these privates that I've missed.
Filenames:
This one is open to discussion, but I think the best naming convention for the
generated header files (and probably also the generated .cpp implementations)
is 'gdom-html-menu-element.h' instead of 'GdomHTMLMenuElement.h'. You can use
the same algorithm used in the code generator to generate these filenames. The
hyphens deviate from our existing headers a little but they're important for
clarity when it comes to the kind of type names you get in the DOM. I don't
think you need to change this yet since it'll be even more of a pain to
maintain the build system while working on the patch, but we'll need to
remember to re-visit this point later.
Exception codes:
The ExceptionCodes are currently discarded. At some point we'll want to store
these error codes (like the JS bindings do with setDOMException()) and allow
for retreival following the call. Again, this isn't a top priority and can be
dealt with when the other issues are fixed.
String management:
First off, you need to use WebCore::String::fromUTF8() rather than
WebCore::String() any place a const gchar* is passed in since GObject strings
deal in UTF-8.
There is a major problem with const gchar* as a return value. As far as I can
tell, GStringConvert() returns g_strdup()'ed strings which need to be manually
g_free()'ed by the caller. First of all, this code shouldn't be duplicated in
every generated file, but moreover, we try to return strings that the caller
doesn't have to free manually and gdom breaks that convention.
Potential ways of solving this:
(*) Keep around a single CString and replace it each time a function returns a
string. Not good because after two subsequent DOM function calls returning
strings, the first will become invalid.
(*) Simple GC. Keep around the CStrings or references to the g_strdup() result
and free them in an idle handler.
(*) AtomicStrings are probably easier to cache than Strings, so they could have
a more efficient special-case?
The good news is that most strings are accessed via GObject properties rather
than const gchar* returns, of which there are only 33:
$ grep -h -A1 'WEBKIT_API const gchar \*' DerivedSources/Gdom*.h | egrep -v --
'--|WEBKIT_API'
gdom_character_data_substring_data (GdomCharacterData *thiz, gulong offset,
gulong length);
gdom_css_primitive_value_get_string_value (GdomCSSPrimitiveValue *thiz);
gdom_css_style_declaration_get_property_value (GdomCSSStyleDeclaration *thiz,
const gchar *property_name);
gdom_css_style_declaration_remove_property (GdomCSSStyleDeclaration *thiz,
const gchar *property_name);
gdom_css_style_declaration_get_property_priority (GdomCSSStyleDeclaration
*thiz, const gchar *property_name);
gdom_css_style_declaration_item (GdomCSSStyleDeclaration *thiz, gulong index);
gdom_css_style_declaration_get_property_shorthand (GdomCSSStyleDeclaration
*thiz, const gchar *property_name);
gdom_css_variables_declaration_get_variable_value (GdomCSSVariablesDeclaration
*thiz, const gchar *variable_name);
gdom_css_variables_declaration_remove_variable (GdomCSSVariablesDeclaration
*thiz, const gchar *variable_name);
gdom_css_variables_declaration_item (GdomCSSVariablesDeclaration *thiz, gulong
index);
gdom_document_query_command_value (GdomDocument *thiz, const gchar *command);
gdom_dom_application_cache_item (GdomDOMApplicationCache *thiz, gulong index);
gdom_dom_core_exception_to_string (GdomDOMCoreException *thiz);
gdom_dom_selection_to_string (GdomDOMSelection *thiz);
gdom_dom_window_prompt (GdomDOMWindow *thiz, const gchar *message, const gchar
*default_value);
gdom_element_get_attribute (GdomElement *thiz, const gchar *name);
gdom_element_get_attribute_ns (GdomElement *thiz, const gchar *namespace_uri,
const gchar *local_name);
gdom_event_exception_to_string (GdomEventException *thiz);
gdom_html_anchor_element_to_string (GdomHTMLAnchorElement *thiz);
gdom_html_canvas_element_to_data_url (GdomHTMLCanvasElement *thiz, const gchar
*type);
gdom_media_list_item (GdomMediaList *thiz, gulong index);
gdom_node_lookup_prefix (GdomNode *thiz, const gchar *namespace_uri);
gdom_node_lookup_namespace_uri (GdomNode *thiz, const gchar *prefix);
gdom_range_exception_to_string (GdomRangeException *thiz);
gdom_range_to_string (GdomRange *thiz);
gdom_storage_key (GdomStorage *thiz, gulong index);
gdom_storage_get_item (GdomStorage *thiz, const gchar *key);
gdom_xml_http_request_exception_to_string (GdomXMLHttpRequestException *thiz);
gdom_xml_http_request_get_all_response_headers (GdomXMLHttpRequest *thiz);
gdom_xml_http_request_get_response_header (GdomXMLHttpRequest *thiz, const
gchar *header);
gdom_xml_serializer_serialize_to_string (GdomXMLSerializer *thiz, GdomNode
*node);
gdom_x_path_exception_to_string (GdomXPathException *thiz);
gdom_x_path_ns_resolver_lookup_namespace_uri (GdomXPathNSResolver *thiz, const
gchar *prefix);
If we can't figure out a smart approach for string management, we can use the
dumb GC approach for now and open a new bug for UTF-8 DOM string optimisations.
--
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