[webkit-reviews] review denied: [Bug 23452] Add XHTMLMP support to Webkit : [Attachment 30271] Updated patch for code change to support XHTMLMP

[bugzilla-daemon at webkit.org]

George Staikos <staikos at kde.org> has denied yichao.yin
<yichao.yin at torchmobile.com.cn>'s request for review:
Bug 23452: Add XHTMLMP support to Webkit
https://bugs.webkit.org/show_bug.cgi?id=23452

Attachment 30271: Updated patch for code change to support XHTMLMP
https://bugs.webkit.org/attachment.cgi?id=30271&action=review

------- Additional Comments from George Staikos <staikos at kde.org>

> Index: WebCore/dom/Node.h
> ===================================================================
> --- WebCore/dom/Node.h	(revision 43615)
> +++ WebCore/dom/Node.h	(working copy)
> @@ -425,9 +425,9 @@ public:
>  
>      virtual void willRemove();
>      void createRendererIfNeeded();
> -    PassRefPtr<RenderStyle> styleForRenderer();
> +    virtual PassRefPtr<RenderStyle> styleForRenderer();

   Can we avoid this change somehow?  Or at least can we make it only virtual
if XHTMLMP is enabled?	Ideally avoid though.


>  void XMLTokenizer::internalSubset(const xmlChar* name, const xmlChar*
externalID, const xmlChar* systemID)
> @@ -959,8 +1007,9 @@ void XMLTokenizer::internalSubset(const 
>      }
>      
>      if (m_doc) {
> -#if ENABLE(WML)
>	   String extId = toString(externalID);
> +	   String dtdName = toString(name);
> +#if ENABLE(WML)


   I think we should not unguard those two lines.  If they need to be enabled
for XHTMLMP then we should guard them for both XHTMLMP and WML, not open them
for all platforms.


> +#if ENABLE(XHTMLMP)
> +    else if ((publicId == QLatin1String("-//WAPFORUM//DTD XHTML Mobile
1.1//EN"))
> +		|| (publicId == QLatin1String("-//WAPFORUM//DTD XHTML Mobile
1.0//EN"))) {

   Too many parentheses here.



I think the rest is fine, though in Document I would probably change the "if
(settings()) ..." to:
m_shouldProcessNoScriptElement = settings() &&
!settings()->isJavaScriptEnabled();