[webkit-reviews] review denied: [Bug 20393] Add WML support to WebKit : [Attachment 24709] WebCore touched by WML

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> 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 24709: WebCore touched by WML
https://bugs.webkit.org/attachment.cgi?id=24709&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Patch looks great, almost r+. Some minors issues:

DOMImplementation.cpp
> +
> +#if ENABLE(WML)
> +    if ((type == "text/vnd.wap.wml")
> +#if ENABLE(WBXML)
> +	   || (type == "application/vnd.wap.wmlc")
> +#endif
> +	   )

Extra parenthesis around (type == ..) are superflous. Please remove.

XMLTokenizer.h:
> +	   Document *document() const { return m_doc; }
> +

Should read "Document* document()..."

HTMLSelectElement:
>  void HTMLSelectElement::setMultiple(bool multiple)
>  {
> +#if ENABLE(WML)
> +    m_multiple = multiple;
> +#else
>      setAttribute(multipleAttr, multiple ? "" : 0);
> +#endif
>  }

When enabling WML, this would affect HTML as well, is altering m_multiple
really desired? When looking through the code, m_multiple is set through
parseMappedAttribute, and in general most HTMLElement derived classes exposing
setXXXValue / xxxValue functions are supposed to modify the corresponding
attribute values through setAttribute. Alterin m_multiple violates this
pattern. What's the reason?

MainResourceLoader.cpp:
> +#if ENABLE(WBXML)
> +    if (length && m_response.mimeType() == "application/vnd.wap.wmlc") {
> +	   printf(">>>>>>>>>>>>>  decode wbxml document\n");
> +	   printf(">>>>>>>>>>>>>  decode wbxml document\n");
> +	   printf(">>>>>>>>>>>>>  decode wbxml document\n");
> +	   WB_UTINY *wbxml = (WB_UTINY*)data;
> +	   WB_ULONG wbxml_len = length;
> +	   WB_ULONG xml_len = 0;
> +	   WB_UTINY *xml = 0;
> +	   WBXMLError ret = WBXML_OK; 
> +	   WBXMLGenXMLParams params;
> + 
> +	   // Init Default Parameters
> +	   params.lang = WBXML_LANG_UNKNOWN;
> +	   params.gen_type = WBXML_GEN_XML_INDENT;
> +	   params.indent = 1;
> +	   params.keep_ignorable_ws = FALSE;
> + 
> +	   ret = wbxml_conv_wbxml2xml_withlen(wbxml, wbxml_len, &xml, &xml_len,
&params);
> +	   if (ret != WBXML_OK)
> +	       return;
> +	   data = (char*)xml;
> +	   length = xml_len;
> +    }
> +#endif
That looks hackish overall. Please move stars -> "WB_UTINY *wbxml" ->
"WB_UTINY* wbxml".
C style casts should be changed to static/reinterpret cast.

Anything else looks sane!