[webkit-reviews] review denied: [Bug 23452] Add XHTMLMP support to Webkit : [Attachment 27560] patch for other change

[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 27560: patch for other change 
https://bugs.webkit.org/attachment.cgi?id=27560&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +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.