Bug 23452 – Add XHTMLMP support to Webkit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[yichao.yin]

the upcoming patch is for adding XHTMLMP support to Webkit. It can be enabled/disabled by ENABLE(XHTMLMP) and is disabled by default. It has just been verified for QT port so far.

[yichao.yin]

Created attachment 26890 [details]
Add XHTMLMP support initial patch

Initial patch

[Nikolas Zimmermann]

Adding Darin to the CC list, as I'd like to hear his opinion regarding HTMLNoScriptElement (especially the manual style resolution (aka. attach/recalcStyle implementation)). I've checked most of the other code, and it's looking quite nice. Though I think there may be a better solution regarding the attach/recalcStyle functions, that I'm overlooking - so calling out for Darin :-)

[yichao.yin]

Created attachment 27307 [details]
updated patch


Create the new patch against the night build of TOT.

Only two source files need updating. i.e. The XMLTokenizerQt.cpp and XMLTokenizerLibxml2.cpp are updated since they have been changed.

[Dave Hyatt]

Comment on attachment 27307 [details]
updated patch

The code here:

if (m_doc->isXHTMLDocument() && !m_hasDocTypeDeclaration) {

... bails if you don't have a DOCTYPE.  It only asks if you're an XHTML document, so if XHTMLMP is enabled, you'll start being more strict in your handling of XHTML documents that aren't also the XHTMLMP MIME type.  Is that what you really want?

The code in parseDTD and externalSubsetHandler had lines like:

(publicId == QLatin1String("-//WAPFORUM//DTD XHTML Mobile 1.0//EN"))) {

These were unconditionally removed.  Shouldn't those just be left alone but put inside #if !ENABLE(XHTMLMP)?

The new document method isXHTMLDocument() was added inside #if ENABLE(XHTMLMP).  It's ok for that to be outside of the #ifdef.

Can you explain what the NOSCRIPT changes are about? Why do you need an actual element for <noscript> in the XHTMLMP case?

Thanks!

[yichao.yin]

Hi Dave,
Thanks for your good comments. my reply is below.

(In reply to comment #4)
> (From update of attachment 27307 [details] [review])
> The code here:
> if (m_doc->isXHTMLDocument() && !m_hasDocTypeDeclaration) {
> ... bails if you don't have a DOCTYPE.  It only asks if you're an XHTML
> document, so if XHTMLMP is enabled, you'll start being more strict in your
> handling of XHTML documents that aren't also the XHTMLMP MIME type.  Is that
> what you really want?

You are right, we'd better add Document::isXHTMLMPDocument() to check this.


> The code in parseDTD and externalSubsetHandler had lines like:
> (publicId == QLatin1String("-//WAPFORUM//DTD XHTML Mobile 1.0//EN"))) {
> These were unconditionally removed.  Shouldn't those just be left alone but put
> inside #if !ENABLE(XHTMLMP)?

Yes, sorry to ignore it, will fix them. :-)

> The new document method isXHTMLDocument() was added inside #if ENABLE(XHTMLMP).
>  It's ok for that to be outside of the #ifdef.

Will replace this method with isXHTMLMPDocument().

> Can you explain what the NOSCRIPT changes are about? Why do you need an actual
> element for <noscript> in the XHTMLMP case?
> Thanks!

As per the specification: OMA-WAP-XHTMLMP-V1_1-20061020-A.pdf, XHTML Mobile profile is a strict subset of XHTML 1.1 [XHTML11]. It extends XHTML Basic [XHTMLBasic] to bring enhanced functionality to application authors, including "scripts within a document", and <noscript> is a new elment in the scripts module, which is used to describe an alternate presentation for use. It's defined in this DTD document:
http://www.w3.org/TR/xhtml-modularization/DTD/xhtml-script-1.mod

Usually, there are two situations the <noscript> should be processed.
1) when any <script> elements can't be excuted because of the language specified by its type attribute is invalid.
2) scripting is not supported by the user agent or is configured to not evaluate scripts by user.

I will upload the updated patch soon.
Thanks againg

[yichao.yin]

Created attachment 27342 [details]
updated patch


This patch contains the fix of issues that Dave points out.
The main change is replacing the method Document::isXHTMLDocument() with Document::isXHTMLMPDocument()

[Darin Adler]

Comment on attachment 27342 [details]
updated patch

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

[yichao.yin]

(In reply to comment #7)
> (From update of attachment 27342 [details] [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.

[yichao.yin]

Created attachment 27559 [details]
updated patch fixed some issues Darin pointed out


This patch just fixed part of the issues Darin points out.
including what I replied to promise to fix in comments #8.
As for the left, I will deal with them tomorrow.

[yichao.yin]

Created attachment 27560 [details]
patch for other change 


This patch contains all the other code change for XHTMLMP except the changes in the above patch 27559 which fixes some issues Darin raised.

[yichao.yin]

Created attachment 27561 [details]
Layout tests for XHTMLMP

[Darin Adler]

Comment on attachment 27559 [details]
updated patch fixed some issues Darin pointed out

> +#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='http://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/png,*/*;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.

[Darin Adler]

(In reply to comment #8)
> > > +#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?

As I said in my review, I don't think the assert is valuable. If we need a null check, we can include one, but if not we don't want to assert the pointer. It doesn't really add much.

When I said "member initialization", I meant this syntax:

    , m_shouldProcessNoScriptElement(xxx)

As opposed to assignment.

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

I don't get it. How can the same browser support both XHTML and XHTMLMP then? I think this is highly suspect.

> Sorry, I am not sure how to name the error is better. how about this one:
> "DOCTYPE declaration lack." ?

Maybe someone else, perhaps Hyatt, can help you with this naming.

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

Yes.

> I think it's not very good for this patch to touch much none-XHTMLMP code in
> the WebCore. No?

I understand your desire to not touch other code, but that's not a good strategy long term. We shouldn't add things without considering the overall design. The change I suggested has very little, or no, cost.

I think it's absolutely fine to touch the non-XHTMLMP code if it makes things cleaner. Ideally, though, we should do that in a separate small patch first to prepare, and not do it as part of a large XHTMLMP patch.

[Darin Adler]

Comment on attachment 27560 [details]
patch for other change 

> +class HTMLNoScriptElement : public HTMLElement {
> +public:
> +    HTMLNoScriptElement(const QualifiedName&, Document*);
> +    ~HTMLNoScriptElement();
> +
> +    virtual bool checkDTD(const Node*);
> +    virtual void attach();
> +    virtual void recalcStyle(StyleChange);
> +    bool childShouldCreateRenderer(Node*) const;
> +};

~HTMLNoScriptElement and childShouldCreateRenderer should both be marked virtual. All these functions should be private. I said most of this last time. Please either do it or explain why you're not doing it, next time.

> +void HTMLNoScriptElement::attach()

What happened to my comment about sharing more code here? I don't understand why you need these large functions for HTMLNoScriptElement::attach and recalcStyle, since the behavior is the same as generic HTML elements, such as <span>, when we're in the appropriate script mode. There should be more shared code and less pasted here.

> +    while(parent = parent->parentNode()) {
> +        if(parentRenderer = parent->renderer())
> +            break;
> +    }

Formatting mistakes here. Need space after while and if.

[Darin Adler]

Comment on attachment 27561 [details]
Layout tests for XHTMLMP

Need to add a Skipped list to make sure we skip this for platforms that don't implement XHTMLMP (all current platforms).

[yichao.yin]

(In reply to comment #12)
> (From update of attachment 27559 [details] [review])
> > +#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?

Seems no guarantee for it, so that we'd better add null-check for it and initialize it by assignment.


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

I will investigate it further.

All of the other issues in comments#12, I will fix them in the upcoming patch.

[yichao.yin]

(In reply to comment #13)

> I don't get it. How can the same browser support both XHTML and XHTMLMP then? I
> think this is highly suspect.

In fact, Document::isXHTMLMPDocument() is used to recognize the XHTMLMP doucment by MIMEtype.
XMLTokenizer::isXHTMLMPDocument() will be used to distinguish XHTMLMP and XHTML by m_isXHTMLMPDocument,
which will be reset according to the public ID of DOCTYPE declaration, so that browser can support XHTML
and XHTMLMP at the same time.

> > Sorry, I am not sure how to name the error is better. how about this one:
> > "DOCTYPE declaration lack." ?
> Maybe someone else, perhaps Hyatt, can help you with this naming.
This is a problem. seems I have to keep this one until got a better one. I will ask Hyatt for help.

> > > 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. 
> Yes.
> > I think it's not very good for this patch to touch much none-XHTMLMP code in
> > the WebCore. No?
> I understand your desire to not touch other code, but that's not a good
> strategy long term. We shouldn't add things without considering the overall
> design. The change I suggested has very little, or no, cost.
> I think it's absolutely fine to touch the non-XHTMLMP code if it makes things
> cleaner. Ideally, though, we should do that in a separate small patch first to
> prepare, and not do it as part of a large XHTMLMP patch.

You are right, I will do it.

[yichao.yin]

(In reply to comment #14)
> (From update of attachment 27560 [details] [review])
> > +class HTMLNoScriptElement : public HTMLElement {
> > +public:
> > +    HTMLNoScriptElement(const QualifiedName&, Document*);
> > +    ~HTMLNoScriptElement();
> > +
> > +    virtual bool checkDTD(const Node*);
> > +    virtual void attach();
> > +    virtual void recalcStyle(StyleChange);
> > +    bool childShouldCreateRenderer(Node*) const;
> > +};
> ~HTMLNoScriptElement and childShouldCreateRenderer should both be marked
> virtual. All these functions should be private. I said most of this last time.
> Please either do it or explain why you're not doing it, next time.

Sorry to bother you, I just fixed part of the issues you figured out, and splited up the old large patch yesterday.  I have planned to fix the left part today, so that you find nothing change in the patch 27560. 
the upcoming patch will contains the fix for this issue. :)


> > +void HTMLNoScriptElement::attach()
> What happened to my comment about sharing more code here? I don't understand
> why you need these large functions for HTMLNoScriptElement::attach and
> recalcStyle, since the behavior is the same as generic HTML elements, such as
> <span>, when we're in the appropriate script mode. There should be more shared
> code and less pasted here.

According to the spec, the <noscript> should not be executed until encounterred a failure to execute <script>. While parsing the XHTMLMP document, parser doesn't know if need to process a <noscript> unless it founds the type of some <script> element is invalid. The problem 
is we will lose the <noscript> if we do nothing when parser encounters one <noscript>, so I think we should create a renderer for <noscript> after it is added to DOM tree. But the default attach() can't do this because lack of some condtions. So I did this by myself in HTMLNoScriptElement::attach().  And in order to avoid unnecessary creation of the renderer of children of <noscript> if the <noscript> is not expected to be handled, I set the display of noscript style to NONE. If we found <noscript> need processing later on, we should restore the display value of this <noscript> and creating renderers for its children at this time. I implemented this in HTMLNoscriptElement::recalcStyle().

Of course, your comments are good. the attach() function shoud share the HTML code as much as possible. Now I implement it by making Node:styleForRenderer() virtual function, and override it in HTMLNoScriptElement. I have tried my best to avoid to touch the original code of WebKit much too before, but obviously, I'd better do this sometimes. :)


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

The content of <noscript> has an opportunity to be renderered by this way according to my design. did you have any other good suggestions for this?

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

will fix it.

> +#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


> > I don't think this hack is the right way to fix this bug.

In fact, this bug is not XHTMLMP specific. It's common for XHTML document. At least it is for QT platform. So I think we can leave this issue for this XHTMLMP patch. we can file a new bug to track it. ok?

[yichao.yin]

Created attachment 27593 [details]
updated patch about layout tests for XHTMLMP


The tests are just runned on QT platform so far, so that I have added Skip list for none-QT platform to this patch.

[yichao.yin]

Created attachment 27594 [details]
patch about configure files


This patch contains the changes of configure files for diffrent platform

[yichao.yin]

Created attachment 27596 [details]
patch about bug fix relevant to Darin's comments


This patch covers the fix of issues mentioned in my comments #16, #17 and #18

[yichao.yin]

Created attachment 27597 [details]
patch about scriptelment change


According to the comments of Darin, I add shouldExecuteAsJavaScript() to class ScriptElement.
The change is not XHTMLMP specific. but it can make the implemenation of XHTMLMP better.

[yichao.yin]

Created attachment 27598 [details]
patch contains the other changes has reviewed

[yichao.yin]

Created attachment 30268 [details]
Updated layout tests for XHTMLMP

To proceed with this bug about XHTLMP support for WebKit

This patch is created for layout tests of XHTMLMP

[yichao.yin]

Created attachment 30269 [details]
Updated patch for configure file changes

[yichao.yin]

Created attachment 30270 [details]
Updated patch for ScriptElement change

Change ScriptElement to support XHTMLMP in a better way

[yichao.yin]

Created attachment 30271 [details]
Updated patch for code change to support XHTMLMP

the primary code change to support XHTMLMP

[George Staikos]

Comment on attachment 30271 [details]
Updated patch for code change to support XHTMLMP


> 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();

[George Staikos]

Comment on attachment 30270 [details]
Updated patch for ScriptElement change

Looks okay to me pending corrections in the main patch.

[George Staikos]

One more comment: Please add appropriate copyright messages to the files where new code for XHTMLMP was added.

[yichao.yin]

(In reply to comment #28)
> (From update of attachment 30271 [details] [review])
> > 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.

Yichao: Yeah, we should add ENABLE(XHTMLMP) for it at least.
Making styleForRenderer() virtual can let us to have better chance to specify the proper style for the renderer of HTMLNoScriptElement when needed. If we won't make is virtual, we have to do some hacks in Node::styleForRenderer() to specify the proper style for HTMLNoScriptElement node. This will also cause performance impact -- the unecessary check will be done for every element node which is not HTMLNoScriptElement.  

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

Yichao: When both of XML and XHTMLMP is disabled, the two lines will make us to pay the cost of two additional String object. Ok, I will guard them with proper Macro.


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

Yichao: Will fix it.

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

Yichao: That's better! will fix it.

[yichao.yin]

(In reply to comment #30)
> One more comment: Please add appropriate copyright messages to the files where
> new code for XHTMLMP was added.

Ok, I will add them in the upcoming patch

[yichao.yin]

Created attachment 30438 [details]
Latest Updated patch for code change

This patch contains the fixes for those issues pointed out by George.
In addtion, it also includes changing some copyright information for updated codes, changing xml DOCTYPE declaration lose error message, etc

[Simon Fraser (smfr)]

Both the Changelog and the commit message need to reference this bug, and did not.

[yichao.yin]

Created attachment 30461 [details]
Latest Updated patch for configure file changes

Add bug# in the ChangeLog for tracing convenience

[yichao.yin]

Created attachment 30462 [details]
Latest updated patch for ScriptElement change

Add bug# in the ChangeLog for tracing convenience

[yichao.yin]

Created attachment 30463 [details]
Latest updated patch for code changes

Add bug# in the ChangeLog for tracing convenience

[George Staikos]

This is all checked in but I will leave it open until we address the virtual function changes.

[Laszlo Gombos]

*** Bug 25870 has been marked as a duplicate of this bug. ***

[yichao.yin]

Created attachment 30490 [details]
Latest Updated patch for code changes


This patch fixes the virtual method issue.

[George Staikos]

Comment on attachment 30490 [details]
Latest Updated patch for code changes

Sorry, the previous diff is already applied.  you'll need to make something that builds on it now.

[yichao.yin]

(In reply to comment #41)
> (From update of attachment 30490 [details] [review])
> Sorry, the previous diff is already applied.  you'll need to make something
> that builds on it now.

Yes, I have found it and building the latest version of WebKit, I will update the patch later

[yichao.yin]

Created attachment 30492 [details]
Patch for fixing the virtual function issue

This patch changes the implemenation of HTMLNoScriptElement to avoid using virtual function.

[George Staikos]

Comment on attachment 30492 [details]
Patch for fixing the virtual function issue

By popular request, let's de-virtualize.

[Simon Fraser (smfr)]

Comment on attachment 30492 [details]
Patch for fixing the virtual function issue

> Index: WebCore/dom/Node.cpp
> ===================================================================

> +#if ENABLE(XHTMLMP)
> +        if (localName() == HTMLNames::noscriptTag.localName())
> +            return document()->styleSelector()->styleForElement(static_cast<HTMLNoScriptElement*>(this), 0, false);
> +#endif
>          return document()->styleSelector()->styleForElement(static_cast<Element*>(this));

This would be cleaner as:

bool allowSharing = true;
#if ENABLE(XHTMLMP)
  if (localName() == HTMLNames::noscriptTag.localName())
    allowSharing = false; // comment explaining why noscript is so special
#endif
  return document()->styleSelector()->styleForElement(static_cast<Element*>(this), 0, allowSharing);

[George Staikos]

Added in Simon's changes and committed in 43913.  I think this closes off this bug report, and any other issues should be filed separately if and when they come up.