[webkit-reviews] review denied: [Bug 27434] adding necessary functions and properties to Document IDL gobject bindings : [Attachment 33063] adds 3 essential features to gobject document idl

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Luke Kenneth Casson Leighton
<lkcl at lkcl.net>'s request for review:
Bug 27434: adding necessary functions and properties to Document IDL gobject
bindings
https://bugs.webkit.org/show_bug.cgi?id=27434

Attachment 33063: adds 3 essential features to gobject document idl
https://bugs.webkit.org/attachment.cgi?id=33063&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
> Index: WebCore/dom/Document.idl
> ===================================================================
> --- WebCore/dom/Document.idl	(revision 44473)
> +++ WebCore/dom/Document.idl	(working copy)
> @@ -154,7 +154,8 @@
>  
>		    attribute [ConvertNullToNullString] DOMString title;
>	   readonly attribute DOMString referrer;
> -#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> +#if (defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT) || \
> +	(defined(LANGUAGE_GOBJECT) && LANGUAGE_GOBJECT)
>		    attribute [ConvertNullToNullString] DOMString domain;
>  #else
>	   readonly attribute DOMString domain;

I wonder why the non-JavaScript case is marked as readonly. The HTML 5 spec
suggests it should not be.  It may be that we should consider removing the
#if's here.

> @@ -180,7 +181,8 @@
>  
>	   NodeList getElementsByName(in DOMString elementName);
>  
> -#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> +#if (defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT) || \
> +	(defined(LANGUAGE_GOBJECT) && LANGUAGE_GOBJECT)
>		    attribute [Custom] Location location;
>  #endif

This is flagged as being [Custom] but I don't see a gobject method implementing
this property anywhere in the patch.  Did you forget to include it?

> @@ -193,7 +195,8 @@
>	   Element	      elementFromPoint(in long x, in long y);
>  
>	   // Mozilla extensions
> -#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> +#if (defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT) || \
> +	(defined(LANGUAGE_GOBJECT) && LANGUAGE_GOBJECT)
>	   DOMSelection       getSelection();
>  #endif

The #if's should probably be removed from this one.  The method was added in
<http://trac.webkit.org/changeset/30515>, where the ChangeLog notes that it was
restricted to JS as it was only added for web compatibility.  Since then, the
getSelection attribute on Document has made its way in to the HTML 5
specification:
<http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#documents
>.