Bug 149348 – Drop support for legacy EntityReference DOM Node type

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

[Chris Dumez]

Drop support for legacy EntityReference DOM Node type.

EntityReference has been dropped from the DOM specification:
- https://dom.spec.whatwg.org/#dom-core-changes

EntityReference is not supported in Firefox:
- https://developer.mozilla.org/en-US/docs/Web/API/EntityReference
- https://developer.mozilla.org/en-US/docs/Web/API/NodeIterator.expandEntityReferences
- https://developer.mozilla.org/en-US/docs/Web/API/TreeWalker.expandEntityReferences

Chrome dropped support for EntityReference a while back (May 2013):
- EntityReference: https://code.google.com/p/chromium/issues/detail?id=226628
- NodeFilter / TreeWalker.expandEntityReferences: https://groups.google.com/a/chromium.org/d/msg/blink-dev/-ZO3eja4maA/86T13XJwQpUJ / https://src.chromium.org/viewvc/blink?view=rev&revision=185771

[Chris Dumez]

This is not supported in Internet Explorer either:
https://msdn.microsoft.com/library/ff974819(v=vs.85).aspx (Remarks section)

[Chris Dumez]

Created attachment 261509 [details]
WIP Patch

[WebKit Commit Bot]

Attachment 261509 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 261509 [details]
WIP Patch

Attachment 261509 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/183983

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 261511 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 261512 [details]
Patch

[WebKit Commit Bot]

Attachment 261512 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:21:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WebCore/ChangeLog:22:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 261515 [details]
Patch

[WebKit Commit Bot]

Attachment 261515 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 261519 [details]
Patch

[WebKit Commit Bot]

Attachment 261519 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 261519 [details]
Patch

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

> Source/WebCore/dom/Document.idl:99
> -    [ObjCLegacyUnnamedParameters, RaisesException, NewObject] NodeIterator createNodeIterator(Node root,
> +#if defined(LANGUAGE_OBJECTIVE_C) && LANGUAGE_OBJECTIVE_C
> +    [ObjCLegacyUnnamedParameters, RaisesException] NodeIterator createNodeIterator(Node root,

Why are you removing NewObject?

> Source/WebCore/dom/Document.idl:103
> -    [ObjCLegacyUnnamedParameters, RaisesException, NewObject] TreeWalker createTreeWalker(Node root,
> +    [ObjCLegacyUnnamedParameters, RaisesException] TreeWalker createTreeWalker(Node root,

Ditto.

> Source/WebCore/dom/EntityReference.h:29
> +class EntityReference : public ContainerNode {

This is no longer final??

[Chris Dumez]

Created attachment 261532 [details]
Patch

[Chris Dumez]

Created attachment 261534 [details]
Patch

[WebKit Commit Bot]

Attachment 261534 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 261519 [details]
Patch

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

>> Source/WebCore/dom/Document.idl:99
>> +    [ObjCLegacyUnnamedParameters, RaisesException] NodeIterator createNodeIterator(Node root,
> 
> Why are you removing NewObject?

[NewObject] has no effect on ObjC bindings, only JS ones.

>> Source/WebCore/dom/EntityReference.h:29
>> +class EntityReference : public ContainerNode {
> 
> This is no longer final??

It can no longer be final because it is abstract.

[Chris Dumez]

Still working on getting GTK and Windows building...

[Chris Dumez]

Created attachment 261553 [details]
Patch

[WebKit Commit Bot]

Attachment 261553 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Will try again to fix the GTK build.

[Chris Dumez]

Created attachment 261592 [details]
Patch

[WebKit Commit Bot]

Attachment 261592 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 261592 [details]
Patch

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

> Source/WebCore/bindings/gobject/webkitdom.symbols:-71
> -WebKitDOMEntityReference* webkit_dom_document_create_entity_reference(WebKitDOMDocument*, const gchar*, GError**)

This is not correct, it breaks the API. I'm quite busy today, but I'll try to find some time to provide a proper fix. If you are in a hurry, feel free to land any patch that doesn't break the build and I'll fix the API in a follow up when I find the time. Thanks again for trying to keep GTK port working :-)

[Chris Dumez]

(In reply to comment #23)
> Comment on attachment 261592 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261592&action=review
> 
> > Source/WebCore/bindings/gobject/webkitdom.symbols:-71
> > -WebKitDOMEntityReference* webkit_dom_document_create_entity_reference(WebKitDOMDocument*, const gchar*, GError**)
> 
> This is not correct, it breaks the API. I'm quite busy today, but I'll try
> to find some time to provide a proper fix. If you are in a hurry, feel free
> to land any patch that doesn't break the build and I'll fix the API in a
> follow up when I find the time. Thanks again for trying to keep GTK port
> working :-)

I see. I wrongly assumed it was OK to alter the generated GObject API as long as it made sense. I will upload another iteration that attempts to keep the corresponding GObject API, like I did not ObjC.

[Chris Dumez]

Created attachment 261659 [details]
Patch

[WebKit Commit Bot]

Attachment 261659 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Thanks a bunch for helping. The API is divided into stable and unstable portions. If it's stable API, it can be deprecated but we have to keep it functional. Unstable API like WebKitDOMHTMLMediaElement requires defining WEBKIT_DOM_USE_UNSTABLE_API to get the declarations, is completely undocumented, and can be broken willy-nilly.

The stable API is documented here: http://webkitgtk.org/reference/webkitdomgtk/stable/index-all.html

[Chris Dumez]

Created attachment 261661 [details]
Patch

[WebKit Commit Bot]

Attachment 261661 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

(In reply to comment #27)
> Thanks a bunch for helping. The API is divided into stable and unstable
> portions. If it's stable API, it can be deprecated but we have to keep it
> functional.

Well, not necessarily functional, if the implementation is removed from WebCore we can keep the function in the API, mark it a deprecated, leave it as a no-op and maybe show a warning to let the user know the function is a no-op because it has been removed from the DOM spec.

[Carlos Garcia Campos]

Chris, I won't have time until Thursday, so don't worry about GTK+ port as long as it builds, I'll fix any API break on Thursday. Thanks again.

[Chris Dumez]

(In reply to comment #31)
> Chris, I won't have time until Thursday, so don't worry about GTK+ port as
> long as it builds, I'll fix any API break on Thursday. Thanks again.

Well, as you can see it doesn't anymore and I have no idea why :)

[Chris Dumez]

Created attachment 261663 [details]
Patch

[WebKit Commit Bot]

Attachment 261663 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 261668 [details]
Patch

[WebKit Commit Bot]

Attachment 261668 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 261688 [details]
Patch

[WebKit Commit Bot]

Attachment 261688 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Created attachment 261716 [details]
Patch

[Chris Dumez]

Comment on attachment 261716 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:293
> +    # FIXME: This is skipped because I don't know how to fix the build any better way.

Thanks for the help Michael! It was hard to get this building without being able to build the GTK port locally.

[WebKit Commit Bot]

Attachment 261716 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Node.h:136:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

I was watching the GTK EWS take upwards of an hour to process your patches, and realized that was just not going to work. :(

Anyway, I don't know how to fix this properly either, but after a few tries, I added this in SkipFunction in CodeGeneratorGObject.pm:

    # FIXME: This is skipped because I don't know how to fix the build any better way.
    # https://bugs.webkit.org/show_bug.cgi?id=149348
    if ($functionName eq "webkit_dom_document_create_entity_reference") {
        return 1;
    }

And now the build works for me. I will have to bully Carlos into doing some DOM bindings training, because right now our bus factor is 1.

[WebKit Commit Bot]

Comment on attachment 261716 [details]
Patch

Clearing flags on attachment: 261716

Committed r190120: <http://trac.webkit.org/changeset/190120>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

(In reply to comment #42)
> I was watching the GTK EWS take upwards of an hour to process your patches,
> and realized that was just not going to work. :(
> 
> Anyway, I don't know how to fix this properly either, but after a few tries,
> I added this in SkipFunction in CodeGeneratorGObject.pm:
> 
>     # FIXME: This is skipped because I don't know how to fix the build any
> better way.
>     # https://bugs.webkit.org/show_bug.cgi?id=149348
>     if ($functionName eq "webkit_dom_document_create_entity_reference") {
>         return 1;
>     }
> 
> And now the build works for me. I will have to bully Carlos into doing some
> DOM bindings training, because right now our bus factor is 1.

Proper fix landed in r190199

[Chris Dumez]

(In reply to comment #45)
> (In reply to comment #42)
> > I was watching the GTK EWS take upwards of an hour to process your patches,
> > and realized that was just not going to work. :(
> > 
> > Anyway, I don't know how to fix this properly either, but after a few tries,
> > I added this in SkipFunction in CodeGeneratorGObject.pm:
> > 
> >     # FIXME: This is skipped because I don't know how to fix the build any
> > better way.
> >     # https://bugs.webkit.org/show_bug.cgi?id=149348
> >     if ($functionName eq "webkit_dom_document_create_entity_reference") {
> >         return 1;
> >     }
> > 
> > And now the build works for me. I will have to bully Carlos into doing some
> > DOM bindings training, because right now our bus factor is 1.
> 
> Proper fix landed in r190199

Thanks a lot Carlos!

[Radar WebKit Bug Importer]

<rdar://problem/22882599>