Bug 120691 – [DOM4] Have ProcessingInstruction inherit CharacterData

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Merge https://chromium.googlesource.com/chromium/blink/+/f4784150a3cc4b972fe6b2ea8c2d823106351f10

Have ProcessingInstruction inherit CharacterData as per the latest DOM4
specification:
http://dom.spec.whatwg.org/#processinginstruction

Previously, ProcessingInstruction was inheriting from Node as per DOM
Level 3.

Firefox already follows DOM4 but not IE10.
This change leads to less code duplication.

[Chris Dumez]

I can work on merging this tomorrow.

[Chris Dumez]

Created attachment 210587 [details]
Patch

[Build Bot]

Comment on attachment 210587 [details]
Patch

Attachment 210587 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1696631

[Build Bot]

Comment on attachment 210587 [details]
Patch

Attachment 210587 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1692820

[Chris Dumez]

Created attachment 210591 [details]
Patch

[Build Bot]

Comment on attachment 210591 [details]
Patch

Attachment 210591 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1690847

[Chris Dumez]

Created attachment 210593 [details]
Patch

[WebKit Commit Bot]

Please wait for approval from [e-mail] (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.

[Timothy Hatcher]

Comment on attachment 210593 [details]
Patch

This is fine for the ObjC API.

[Darin Adler]

Comment on attachment 210593 [details]
Patch

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
> -@interface DOMProcessingInstruction : DOMNode WEBKIT_VERSION_1_3
> +@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.

[Chris Dumez]

Comment on attachment 210593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210593&action=review

>> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:318
>> +@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.

Tim said it was OK in https://bugs.webkit.org/show_bug.cgi?id=120691#c9

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

I'll check.

[Timothy Hatcher]

Comment on attachment 210593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=210593&action=review

>>> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:318
>>> +@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.
> 
> Tim said it was OK in https://bugs.webkit.org/show_bug.cgi?id=120691#c9

It is OK because DOMCharacterData has a data method and it still inherits from DOMNode.

[Chris Dumez]

Created attachment 211013 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 211013 [details]
Patch

Clearing flags on attachment: 211013

Committed r155340: <http://trac.webkit.org/changeset/155340>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.