[Webkit-unassigned] [Bug 23452] Add XHTMLMP support to Webkit
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Feb 11 04:29:30 PST 2009
https://bugs.webkit.org/show_bug.cgi?id=23452
------- Comment #8 from yichao.yin at torchmobile.com.cn 2009-02-11 04:29 PDT -------
(In reply to comment #7)
> (From update of attachment 27342 [review])
> > #include "HTMLNames.h"
> > +#if ENABLE(XHTMLMP)
> > +#include "HTMLNoScriptElement.h"
> > +#endif
> > #include "HTMLStyleElement.h"
> Conditional includes should go in a separate paragraph after the unconditional
> includes.
Got it, will fix.
> > +#if ENABLE(XHTMLMP)
> > + if (settings())
> > + m_shouldProcessNoScriptElement = !settings()->isJavaScriptEnabled();
> > +#endif
> Member initialization is preferred over assignment. Also, this seems to leave
> m_shouldProcessNoScriptElement uninitialized if settings() is 0. Also, when can
> settings() be 0? I don't think this is correct.
I am not sure when the settings() will be 0. but I think we'd better use
ASSERT(settings()) to ensure save.
will remove the if segment to make sure m_shouldProcessNoScriptElement is
initialized. is that ok?
> > +#if ENABLE(XHTMLMP)
> > +bool Document::isXHTMLMPDocument() const
> > +{
> > + return (frame() && frame()->loader() && (frame()->loader()->responseMIMEType() == "application/vnd.wap.xhtml+xml" || frame()->loader()->responseMIMEType() == "application/xhtml+xml"));
> > +}
> > +#endif
> No need to null-check frame()->loader(), since that can never be 0. I don't
> understand why "application/xhtml+xml" means isXHTMLMP, and not isXHTML.
will remove the unnecessary null-check.
As for the MIMEType of "application/xhtml+xml" support is for following the
user agent conformance
requirement stated in the section 7.2 of specificatoin
OMA-WAP-XHTMLMP-V1_1-20061020-A.pdf.
This method is just used to judge what document is XHTMLMP document.
> > #include "HTMLLinkElement.h"
> > +#if ENABLE(XHTMLMP)
> > +#include "HTMLNames.h"
> > +#include "HTMLScriptElement.h"
> > +#endif
> > #include "HTMLStyleElement.h"
> Conditional includes should go in a separate paragraph after the unconditional
> includes.
will fix.
> > +#if ENABLE(XHTMLMP)
> > + //check if the DOCTYPE Declaration of XHTMLMP document exists
> > + if (m_doc->isXHTMLMPDocument() && !m_hasDocTypeDeclaration) {
> > + handleError(fatal, "The DOCTYPE declaration is missing, XHTMLMP document expects it.", lineNumber(), columnNumber());
> > + return;
> > + }
> > +#else
> > m_sawFirstElement = true;
> > +#endif
> This change to where m_sawFirstElement is set seems to be a mistake. You should
> instead move it down unconditionally to after you need it for your new code.
> We put spaces after the "//" in our comments.
> The wording of this error isn't consistent with the wording of other parsing
> errors. In particular "XHTMLMP document expects it" is awkward.
Sorry, I am not sure how to name the error is better. how about this one:
"DOCTYPE declaration lack." ?
> > +#if ENABLE(XHTMLMP)
> > + if (!m_sawFirstElement && isXHTMLMPDocument()) {
> > + m_sawFirstElement = true;
> > +
> > + // As per the section 7.1 of OMA-WAP-XHTMLMP-V1_1-20061020-A.pdf,
> > + // we should make sure that the root element MUST be 'html' and
> > + // ensure the name of the default namespace on the root elment 'html'
> > + // MUST be 'http://www.w3.org/1999/xhtml'
> > + if (localName != HTMLNames::htmlTag.localName()) {
> > + handleError(fatal, "The XHTMLMP document's root element is not a 'html' element.", lineNumber(), columnNumber());
> > + return;
> > + } else if (uri.isNull()) {
> > + m_defaultNamespaceURI = HTMLNames::xhtmlNamespaceURI;
> > + uri = m_defaultNamespaceURI;
> > + }
> > + } else if (!m_sawFirstElement)
> > + m_sawFirstElement = true;
> > +#endif
> It's strange to do if (!m_sawFirstElement) before setting m_sawFirstElement.
> You should just do that unconditionally, and outside the #if.
> We don't do "else" after "return".
Adding condition for the initialization of m_sawFirstElement is for avoiding to
affect the original logic.
but you are right, it is not necessary indeed here. Thanks.
> > @@ -797,6 +836,11 @@ void XMLTokenizer::endElementNs()
> > ASSERT(!m_pendingScript);
> > m_requestingScript = true;
> >
> > +#if ENABLE(XHTMLMP)
> > + if (element->isHTMLElement() && !static_cast<HTMLScriptElement*>(scriptElement)->shouldExecuteAsJavaScript())
> > + m_doc->setShouldProcessNoscriptElement(true);
> > + else {
> > +#endif
> > String scriptHref = scriptElement->sourceAttributeValue();
> > if (!scriptHref.isEmpty()) {
> > // we have a src attribute
> > @@ -813,6 +857,9 @@ void XMLTokenizer::endElementNs()
> > } else
> > m_view->frame()->loader()->executeScript(ScriptSourceCode(scriptElement->scriptContent(), m_doc->url(), m_scriptStartLine));
> >
> > +#if ENABLE(XHTMLMP)
> > + }
> > +#endif
> You should put the braces outside the #if, and indent the code.
> The check before casting scriptElement to HTMLScriptElement* is a little bit
> non-obvious. I think that function should be on ScriptElement not on
> HTMLScriptElement. It can simply return false for non-HTML script elements.
> That's more efficient than what you're doing here and also safer because it
> doesn't require a cast.
sounds better. but if that we have to change the code of ScriptElement and
SVGScriptElement.
I think it's not very good for this patch to touch much none-XHTMLMP code in
the WebCore. No?
> > +#if ENABLE(XHTMLMP)
> > + m_hasDocTypeDeclaration = false;
> > +#endif
> This seems like a strange place to set this boolean. Is there a better
> approach?
maybe XMLTokenizer::endDocument() is a better place.
> > -#if ENABLE(WML)
> > String extId = toString(externalID);
> > + String dtdName = toString(name);
> > +#if ENABLE(WML)
> This change will result in unused variable warnings for non-WML non-XHTMLMP
> builds. I think you need to do more #if here to get this right for all the
> cases.
No, for non-WML and non-XHTMLMP build, dtdName will also be used.
> > if (isWMLDocument()
> > && extId != "-//WAPFORUM//DTD WML 1.3//EN"
> > && extId != "-//WAPFORUM//DTD WML 1.2//EN"
> > @@ -962,8 +1013,20 @@ void XMLTokenizer::internalSubset(const
> > && extId != "-//WAPFORUM//DTD WML 1.0//EN")
> > handleError(fatal, "Invalid DTD Public ID", lineNumber(), columnNumber());
> > #endif
> > +#if ENABLE(XHTMLMP)
> > + if ((extId == "-//WAPFORUM//DTD XHTML Mobile 1.0//EN")
> > + || (extId == "-//WAPFORUM//DTD XHTML Mobile 1.1//EN")) {
> > + if (dtdName != HTMLNames::htmlTag.localName()) {
> > + handleError(fatal, "Invalid DOCTYPE declaration, XHTMLMP expects 'html' as root element.", lineNumber(), columnNumber());
> > + return;
> > + } else if (m_doc->isXHTMLMPDocument())
> > + setIsXHTMLMPDocument(true);
> >
> > - m_doc->addChild(DocumentType::create(m_doc, toString(name), toString(externalID), toString(systemID)));
> > + m_hasDocTypeDeclaration = true;
> > + }
> > +#endif
> No else after return.
will fix.
> > +
> > // http://bugs.webkit.org/show_bug.cgi?id=10854
> > // The frame's last ref may be removed and it can be deleted by checkCompleted(),
> > // so we'll add a protective refcount
> > @@ -3605,7 +3614,11 @@ void FrameLoader::addExtraFieldsToReques
> > }
> >
> > if (mainResource)
> > +#if ENABLE(XHTMLMP)
> > + request.setHTTPAccept("application/xml,application/vnd.wap.xhtml+xml,application/xhtml+xml;profile='http://www.wapforum.org/xhtml',text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5");
> > +#else
> > request.setHTTPAccept("application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5");
> > +#endif
> This is awkward. I suggest we use a constant at the top of the file and put the
> #if up there instead of having the if here.
> static const char acceptHeaderField[] = "";
Better idea. I will fix it.
Darin, Thanks a lot for your good comments.
Regarding the above comments I replied, I will upload the updated patch to fix
them later. And for the other comments, I will handle them tommorrow and keep
you updated.
--
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.
More information about the webkit-unassigned
mailing list