[Webkit-unassigned] [Bug 23452] Add XHTMLMP support to Webkit

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun May 17 22:42:23 PDT 2009


https://bugs.webkit.org/show_bug.cgi?id=23452





------- Comment #31 from yichao.yin at torchmobile.com.cn  2009-05-17 22:42 PDT -------
(In reply to comment #28)
> (From update of attachment 30271 [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.


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