[webkit-reviews] review granted: [Bug 120691] [DOM4] Have ProcessingInstruction inherit CharacterData : [Attachment 210593] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Christophe Dumez <dchris at gmail.com>'s
request for review:
Bug 120691: [DOM4] Have ProcessingInstruction inherit CharacterData
https://bugs.webkit.org/show_bug.cgi?id=120691

Attachment 210593: Patch
https://bugs.webkit.org/attachment.cgi?id=210593&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=210593&action=review


Looks OK except for two small issues. I’m going to say r=me but we should check
into these two things.

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:318
> - at interface DOMProcessingInstruction : DOMNode WEBKIT_VERSION_1_3
> + at interface DOMProcessingInstruction : DOMCharacterData WEBKIT_VERSION_1_3

I am not sure this is OK to change. Changing the inheritance could break OS X
apps. We should ask Tim Hatcher what his risk assessment is.

> Source/WebCore/dom/CharacterData.cpp:207
> +    if (nodeType() == PROCESSING_INSTRUCTION_NODE)
> +	   toProcessingInstruction(this)->checkStyleSheet();

This is unfortunate, because nodeType is a virtual function dispatch. I guess
this is not such a hot code path, though.

> Source/WebCore/dom/CharacterData.h:65
> +    String m_data;

Does ProcessingInstruction really need direct access to the data member? I
can’t tell why from the patch. Typically I’d prefer a small inline function
instead of exposing the data member.