[Webkit-unassigned] [Bug 20393] Add WML support to WebKit

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20393


zimmermann at kde.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24709|review?                     |review-
               Flag|                            |




------- Comment #26 from zimmermann at kde.org  2008-11-12 10:51 PDT -------
(From update of attachment 24709)
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!


-- 
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.