[webkit-reviews] review denied: [Bug 23452] Add XHTMLMP support to Webkit : [Attachment 27559] updated patch fixed some issues Darin pointed out

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> 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 27559: updated patch fixed some issues Darin pointed out
https://bugs.webkit.org/attachment.cgi?id=27559&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +#if ENABLE(XHTMLMP)
> +    ASSERT(settings());
> +    m_shouldProcessNoScriptElement = !settings()->isJavaScriptEnabled();
> +#endif

I don't think this assertion is helpful. We don't want to assert ever pointer
just before we dereference it.

But I also don't know if settings() is guaranteed to be non-zero here or not.
Is it guaranteed?

> +    return (frame()->loader()->responseMIMEType() ==
"application/vnd.wap.xhtml+xml" || frame()->loader()->responseMIMEType() ==
"application/xhtml+xml");

Please leave out the parentheses.

> +    }
> +#endif
> +   m_sawFirstElement = true;

Indentation is wrong here.

> +#if ENABLE(XHTMLMP)
> +static const char* const defaultHttpAcceptHeader =
"application/xml,application/vnd.wap.xhtml+xml,application/xhtml+xml;profile='h
ttp://www.wapforum.org/xhtml',text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=
0.5";
> +#else
> +static const char* const defaultHttpAcceptHeader =
"application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/p
ng,*/*;q=0.5";
> +#endif

This is good. static const char defaultHTTPAcceptHeader[] would be even better,
I think. WebKit style capitalizes the acronym "HTTP", and doesn't use "Http".
Or you can leave it out of this name altogether. I don't think it's that
confusing to call this just an "accept header" locally in this file.

> +#if PLATFORM(QT) && ENABLE(XHTMLMP)
> +    // FIXME: Qt is flawed
> +    // This hacking is for fixing one bug of javasrcipt in XHTMLMP document,

> +    // i.e: if alert() method is contained directly in <script> element, 
> +    // it will cause QtLauncher crash when frame loader execute it.
> +    if (m_isRunningScript && m_frame->document() &&
m_frame->document()->isXHTMLMPDocument())
> +	   return;
> +#endif

We need to figure out the real problem and fix this in a better way.