[webkit-reviews] review denied: [Bug 20393] Add WML support to WebKit : [Attachment 22813] Patch to implement WML

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied George Staikos <staikos at kde.org>'s
request for review:
Bug 20393: Add WML support to WebKit
https://bugs.webkit.org/show_bug.cgi?id=20393

Attachment 22813: Patch to implement WML
https://bugs.webkit.org/attachment.cgi?id=22813&action=edit

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
I think in general this patch is too large as-is to review. Therefore I only
looked at coding style issues and not at what it exactly does. Maybe the patch
can be split up in wml specific, changes to the non-wml code and tests?

The coding style does not seem consistent, which is wrong per definition. I
assume the webkit style must be followed, which means all if(, for( and while(
uses should be if (, for ( and while (,
Here are some coding style issues:

Example of if( use where if ( should be used:

>+
>+void WMLAElement::defaultEventHandler(Event* evt) 
>+{
>+    bool shouldHandle = false;
>+    if(evt->type() == clickEvent) {
>+	  shouldHandle = true;

Example of while( use where while ( should be used:

>+void WMLCardElement::registerDoElement(WMLDoElement* e)
>+{
>+    if(e) {
>+	  Vector<WMLDoElement*>::iterator it = m_doVecs.begin();
>+	  WMLDoElement* doe = NULL;
>+	  while(it != m_doVecs.end()) {

Example of for( use where for ( should be used:

>+
>+    String templDoName;
>+    String cardDoName;
>+    WMLDoElement* templDo = 0;
>+    for(; templIt != templDoNameVecs.end(); ++templIt){
>+	  templDo = *templIt;

Put { on a new line after functions:

>+void WMLTemplateElement::setHasIntrEvent(bool has) {
>+    m_hasIntrEvent = has;
>+}
>+
>+bool WMLTemplateElement::hasIntrEvent() {
>+    return m_hasIntrEvent;
>+}

In general my advise is to code all sources to match webkit coding style. Until
then I think it makes sense to r- this, so that is what I do now.