Bug 121543 - [GTK] Some of DOM bindings macros are misnamed
Summary: [GTK] Some of DOM bindings macros are misnamed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 133724
  Show dependency treegraph
 
Reported: 2013-09-18 00:37 PDT by Tomas Popela
Modified: 2014-06-24 23:55 PDT (History)
13 users (show)

See Also:


Attachments
Patch (107.92 KB, patch)
2014-06-23 05:33 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff
Patch (107.92 KB, patch)
2014-06-23 06:42 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Popela 2013-09-18 00:37:15 PDT
Some of DOM bindings macros are misnamed.

WEBKIT_DOM_HTMLBR_ELEMENT should be WEBKIT_DOM_HTML_BR_ELEMENT
WEBKIT_DOM_HTMLLI_ELEMENT should be WEBKIT_DOM_HTML_LI_ELEMENT
WEBKIT_DOM_HTMLHR_ELEMENT should be WEBKIT_DOM_HTML_HR_ELEMENT
WEBKIT_DOM_HTMLU_LIST_ELEMENT should be WEBKIT_DOM_HTML_U_LIST_ELEMENT
WEBKIT_DOM_HTMLD_LIST_ELEMENT should be WEBKIT_DOM_HTML_D_LIST_ELEMENT
WEBKIT_DOM_HTMLO_LIST_ELEMENT should be WEBKIT_DOM_HTML_O_LIST_ELEMENT

The same is for _IS_ macros (ie. WEBKIT_DOM_IS_HTML_BR_ELEMENT not HTMLBR_ELEMENT).

Looks like generator is doing something wrong with elements that have their tag name with two or less characters.

We should do something similar like in webkit_dom_element_get_id vs. webkit_dom_html_element_get_id case, because these macros are part of the public API.
Comment 1 Carlos Garcia Campos 2013-09-18 00:45:32 PDT
We have some exceptions in the function decamelize, but not all cases are covered. While working on bug #121538 I also noticed an inconsistency in the macros for the WebKitDOMObject and all others, see:

WEBKIT_TYPE_DOM_OBJECT
WEBKIT_IS_DOM_OBJECT
WEBKIT_IS_DOM_OBJECT_CLASS

While all other generated objects have WEBKIT_DOM as the prefix, for example:

WEBKIT_TYPE_DOM_ATTR
WEBKIT_DOM_IS_ATTR
WEBKIT_DOM_IS_ATTR_CLASS

So, we are always using WEBKIT as the prefix for TYPE macros, but WEBKIT_DOM for IS macros except for WebKitDOMObject that we use WEBKIT. 

I think we should fix all these inconsistencies and provide backwards compatibility macros in WebKitDOMCustom.
Comment 2 Carlos Garcia Campos 2014-06-16 04:33:25 PDT
(In reply to comment #1)
> We have some exceptions in the function decamelize, but not all cases are covered. While working on bug #121538 I also noticed an inconsistency in the macros for the WebKitDOMObject and all others, see:
> 
> WEBKIT_TYPE_DOM_OBJECT
> WEBKIT_IS_DOM_OBJECT
> WEBKIT_IS_DOM_OBJECT_CLASS
> 
> While all other generated objects have WEBKIT_DOM as the prefix, for example:
> 
> WEBKIT_TYPE_DOM_ATTR
> WEBKIT_DOM_IS_ATTR
> WEBKIT_DOM_IS_ATTR_CLASS
> 
> So, we are always using WEBKIT as the prefix for TYPE macros, but WEBKIT_DOM for IS macros except for WebKitDOMObject that we use WEBKIT. 
> 
> I think we should fix all these inconsistencies and provide backwards compatibility macros in WebKitDOMCustom.

To make it clear, since we are using WebKitDOM as our namespace I think we should rename all TYPE macros as WEBKIT_DOM_TYPE_FOO and WebKitDOMObject macros as well. Unless I'm wrong and the intention was not to use WebKitDOM as the namespace. Maybe Gustavo or Martin know it.
Comment 3 Martin Robinson 2014-06-16 08:49:31 PDT
(In reply to comment #2)

> To make it clear, since we are using WebKitDOM as our namespace I think we should rename all TYPE macros as WEBKIT_DOM_TYPE_FOO and WebKitDOMObject macros as well. Unless I'm wrong and the intention was not to use WebKitDOM as the namespace. Maybe Gustavo or Martin know it.

I cannot remember why it was designed this way.
Comment 4 Gustavo Noronha (kov) 2014-06-19 13:36:52 PDT
I think originally the namespace was supposed to be WebKit only, even for the DOM bindings, but in practice it ended up being WebKitDOM, so I think this fix would make sense.
Comment 5 Tomas Popela 2014-06-23 05:33:25 PDT
Created attachment 233599 [details]
Patch
Comment 6 Carlos Garcia Campos 2014-06-23 06:32:28 PDT
Comment on attachment 233599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233599&action=review

Great job! Thank you very much. This looks good to me. Martin, Gustavo, any objection?

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1419
> -    return WEBKIT_DOM_${clsCaps}(g_object_new(WEBKIT_TYPE_DOM_${clsCaps}, "core-object", coreObject, NULL));
> +    return WEBKIT_DOM_${clsCaps}(g_object_new(WEBKIT_DOM_TYPE_${clsCaps}, "core-object", coreObject, NULL));

We can take advantage we are changing this line to use nullptr instead of NULL.
Comment 7 Tomas Popela 2014-06-23 06:39:53 PDT
(In reply to comment #6)
> (From update of attachment 233599 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233599&action=review
> 
> Great job! Thank you very much. This looks good to me. Martin, Gustavo, any objection?
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1419
> > -    return WEBKIT_DOM_${clsCaps}(g_object_new(WEBKIT_TYPE_DOM_${clsCaps}, "core-object", coreObject, NULL));
> > +    return WEBKIT_DOM_${clsCaps}(g_object_new(WEBKIT_DOM_TYPE_${clsCaps}, "core-object", coreObject, NULL));
> 
> We can take advantage we are changing this line to use nullptr instead of NULL.

No problem. Updated patch will follow.
Comment 8 Tomas Popela 2014-06-23 06:42:42 PDT
Created attachment 233602 [details]
Patch
Comment 9 Martin Robinson 2014-06-24 11:59:49 PDT
Comment on attachment 233602 [details]
Patch

I haven't reviewed this, but I support making the names consistent.
Comment 10 Carlos Garcia Campos 2014-06-24 23:23:49 PDT
Comment on attachment 233602 [details]
Patch

Ok
Comment 11 WebKit Commit Bot 2014-06-24 23:55:13 PDT
Comment on attachment 233602 [details]
Patch

Clearing flags on attachment: 233602

Committed r170422: <http://trac.webkit.org/changeset/170422>
Comment 12 WebKit Commit Bot 2014-06-24 23:55:18 PDT
All reviewed patches have been landed.  Closing bug.