[Webkit-unassigned] [Bug 23452] Add XHTMLMP support to Webkit
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Feb 10 10:07:01 PST 2009
https://bugs.webkit.org/show_bug.cgi?id=23452
darin at apple.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #27342|review? |review-
Flag| |
------- Comment #7 from darin at apple.com 2009-02-10 10:07 PDT -------
(From update of attachment 27342)
> #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.
> +#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.
> +#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.
> +#if ENABLE(XHTMLMP)
> + // As for XHTMLMP document, we needn't update the style selector here
> + // to avoid the layout of <noscript> are changed incorrectly.
> + if (!isXHTMLMPDocument())
> +#endif
> m_doc->updateStyleSelector();
Sorry, I don't understand this comment. The language is garbled. I understand
that for <noscript> you don't need updateStyleSelector, but why isn't this
needed for text that's not in <noscript>? This needs a better comment at least.
> #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.
> +#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.
> +#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".
> @@ -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.
> +#if ENABLE(XHTMLMP)
> + m_hasDocTypeDeclaration = false;
> +#endif
This seems like a strange place to set this boolean. Is there a better
approach?
> -#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.
> 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.
> #include "HTMLLinkElement.h"
> +#if ENABLE(XHTMLMP)
> +#include "HTMLNames.h"
> +#include "HTMLScriptElement.h"
> +#endif
> #include "HTMLStyleElement.h"
Separate paragraph.
> +#if ENABLE(XHTMLMP)
> + if (m_doc->isXHTMLMPDocument() && !m_hasDocTypeDeclaration) {
> + handleError(fatal, "DOCTYPE declaration is missing, XHTMLMP document expects it.", lineNumber(), columnNumber());
> + break;
> + }
> +#endif
It's really unfortunate that so much XHTMLMP code has to go in the different
XML parsers. You should investigate refactoring so this code can be in shared
common code instead of being repeated twice.
> #include "HTMLNames.h"
> +#if ENABLE(XHTMLMP)
> +#include "HTMLNoScriptElement.h"
> +#endif
> #include "HTMLOListElement.h"
Separate paragraph.
> +void HTMLNoScriptElement::attach()
> +{
> + ASSERT(!renderer());
> + // In order to make the content within noscript node visible,
> + // we should attach render object to noscript node, then its children
> + // can share its render style to create their own render objects
> + RefPtr<Node> parent = this;
> + RenderObject* parentRenderer = 0;
> + while(parent = parent->parentNode()) {
> + if(parentRenderer = parent->renderer())
> + break;
> + }
> + ASSERT(parentRenderer);
> +
> + RefPtr<RenderStyle> style = RenderStyle::create();
> + style->inheritFrom(parentRenderer->style());
> +
> + RenderObject* ro = createRenderer(parentRenderer->renderArena(), style.get());
> + if (ro) {
> + setRenderer(ro);
> + renderer()->setStyle(style);
> + parentRenderer->addChild(renderer(), nextRenderer());
> + }
> +
> + ContainerNode::attach();
> +
> + // If no need to process <noscript>, we hide it
> + if (!document()->shouldProcessNoscriptElement()) {
> + style->setDisplay(NONE);
> + setChanged();
> + }
> +}
This seems like far too much code. There must be a way to inherit this code
from the default HTML element behavior.
> +void HTMLNoScriptElement::recalcStyle(StyleChange change)
> +{
> + if (!document()->shouldProcessNoscriptElement())
> + return;
> +
> + // If <noscript> needs processing, we make it visiable here.
> + ASSERT(renderer());
> + RefPtr<RenderStyle> style = renderer()->style();
> + ASSERT(style);
> + if (style->display() == NONE) {
> + style->setDisplay(INLINE);
> +
> + // create renderers for its children
> + if (hasChildNodes()) {
> + RenderObject* ro = 0;
> +
> + for (Node* n = firstChild(); n; n = n->traverseNextNode(this))
> + if (!n->renderer())
> + n->createRendererIfNeeded();
> + }
> + }
> +}
This is wrong. If display:none is set, then nothing should be displayed. I
think display:none should *not* be set for these elements, and this should not
be worked around in recalcStyle.
> + virtual bool checkDTD(const Node*);
> + virtual void attach();
> + virtual void recalcStyle(StyleChange);
> + bool childShouldCreateRenderer(Node*) const;
This last function should be marked virtual. I'd suggest making all these
overrides private, too, because of the "make everything as private as possible"
rule.
> +#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
Misspelling of JavaScript here.
I don't think this hack is the right way to fix this bug.
> +
> // 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[] = "";
Otherwise, this looks OK. Maybe an initial patch could leave out some of the
questionable parts so this can be mostly checked in.
--
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