Bug 139171 – Delete Notation because we don't use it

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Delete Notation because we don't use it

[Myles C. Maxfield]

Created attachment 242377 [details]
Patch

[WebKit Commit Bot]

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


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


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

[Build Bot]

Comment on attachment 242377 [details]
Patch

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

New failing tests:
fast/dom/Window/get-set-properties.html
fast/dom/Window/window-lookup-precedence.html
js/dom/global-constructors-attributes.html
fast/dom/constants.html
fast/dom/dom-constructors.html
js/dom/dom-static-property-for-in-iteration.html
http/tests/security/cross-frame-access-put.html

[Build Bot]

Created attachment 242380 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Simon Fraser (smfr)]

Isn't this removal a behavior change for the web, though? Notation nodes are in DOM Level 2.

[Chris Dumez]

FYI, Chrome managed to remove it a while back:
https://code.google.com/p/chromium/issues/detail?id=405525

According to that bug, neither Firefox Nightly nor IE11 have the Notation interface.

The only web-exposed change is that window.Notation property is no longer visible as we don't use Notation anywhere in the API. Since Notation has no constants, it is fairly unlikely to be used out there IMHO.

[Chris Dumez]

Comment on attachment 242377 [details]
Patch

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

> Source/WebCore/dom/Node.h:141
> +        XPATH_NAMESPACE_NODE = 12,

This should probably remain at 13:
http://www.w3.org/TR/2003/CR-DOM-Level-3-XPath-20030331/xpath.html#XPathNamespace

> Source/WebCore/dom/Node.idl:-47
> -    const unsigned short      NOTATION_NODE                  = 12;

I missed this additional web exposed change in my earlier comment.

[Chris Dumez]

Comment on attachment 242377 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No tests because there is no behavior change.

The no behavior change part is not exactly true :)
That said, I don't think we need layout tests to un-expose properties.

[Chris Dumez]

Comment on attachment 242377 [details]
Patch

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

>> Source/WebCore/dom/Node.idl:-47
>> -    const unsigned short      NOTATION_NODE                  = 12;
> 
> I missed this additional web exposed change in my earlier comment.

Chrome actually kept that one. Firefox also still supports it. I haven't tested IE but I think we may want to keep this.

[Build Bot]

Comment on attachment 242377 [details]
Patch

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

New failing tests:
fast/dom/Window/get-set-properties.html
http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html
js/dom/global-constructors-attributes.html
fast/dom/constants.html
fast/dom/dom-constructors.html
js/dom/dom-static-property-for-in-iteration.html
fast/dom/Window/window-lookup-precedence.html
http/tests/security/cross-frame-access-put.html

[Build Bot]

Created attachment 242383 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

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

[Build Bot]

Comment on attachment 242377 [details]
Patch

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

New failing tests:
fast/dom/Window/get-set-properties.html
http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html
js/dom/global-constructors-attributes.html
fast/dom/constants.html
fast/dom/dom-constructors.html
js/dom/dom-static-property-for-in-iteration.html
fast/dom/Window/window-lookup-precedence.html
http/tests/security/cross-frame-access-put.html

[Build Bot]

Created attachment 242389 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

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

[Myles C. Maxfield]

Comment on attachment 242377 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No tests because there is no behavior change.
> 
> The no behavior change part is not exactly true :)
> That said, I don't think we need layout tests to un-expose properties.

Fixed.

>> Source/WebCore/dom/Node.h:141
>> +        XPATH_NAMESPACE_NODE = 12,
> 
> This should probably remain at 13:
> http://www.w3.org/TR/2003/CR-DOM-Level-3-XPath-20030331/xpath.html#XPathNamespace

Yup. I was too hasty with this.

>>> Source/WebCore/dom/Node.idl:-47
>>> -    const unsigned short      NOTATION_NODE                  = 12;
>> 
>> I missed this additional web exposed change in my earlier comment.
> 
> Chrome actually kept that one. Firefox also still supports it. I haven't tested IE but I think we may want to keep this.

Yeah; I think it makes sense to keep the unmodified IDL even if we don't support some part of it.

[Myles C. Maxfield]

Comment on attachment 242377 [details]
Patch

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

>>>> Source/WebCore/dom/Node.idl:-47
>>>> -    const unsigned short      NOTATION_NODE                  = 12;
>>> 
>>> I missed this additional web exposed change in my earlier comment.
>> 
>> Chrome actually kept that one. Firefox also still supports it. I haven't tested IE but I think we may want to keep this.
> 
> Yeah; I think it makes sense to keep the unmodified IDL even if we don't support some part of it.

Unfortunately, if this is present, our codegen step requires the relevant value in Node.h, and if the enum value is in Node.h, all the places where we switch over the NodeType have to include this value, which is what this patch is attempting to remove. Looks like I've got to take it out here as well.

[Chris Dumez]

(In reply to comment #15)
> Comment on attachment 242377 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242377&action=review
> 
> >>>> Source/WebCore/dom/Node.idl:-47
> >>>> -    const unsigned short      NOTATION_NODE                  = 12;
> >>> 
> >>> I missed this additional web exposed change in my earlier comment.
> >> 
> >> Chrome actually kept that one. Firefox also still supports it. I haven't tested IE but I think we may want to keep this.
> > 
> > Yeah; I think it makes sense to keep the unmodified IDL even if we don't support some part of it.
> 
> Unfortunately, if this is present, our codegen step requires the relevant
> value in Node.h, and if the enum value is in Node.h, all the places where we
> switch over the NodeType have to include this value, which is what this
> patch is attempting to remove. Looks like I've got to take it out here as
> well.

Blink dealt with it by moving the deprecated values to a new enum:
enum NodeType {
        ELEMENT_NODE = 1,
        ATTRIBUTE_NODE = 2,
        TEXT_NODE = 3,
        CDATA_SECTION_NODE = 4,
        PROCESSING_INSTRUCTION_NODE = 7,
        COMMENT_NODE = 8,
        DOCUMENT_NODE = 9,
        DOCUMENT_TYPE_NODE = 10,
        DOCUMENT_FRAGMENT_NODE = 11,
    };

    // Entity, EntityReference, Notation, and XPathNamespace nodes are impossible to create in Blink.
    // But for compatibility reasons we want these enum values exist in JS, and this enum makes the bindings
    // generation not complain about ENTITY_REFERENCE_NODE being missing from the implementation
    // while not requiring all switch(NodeType) blocks to include this deprecated constant.
    enum DeprecatedNodeType {
        ENTITY_REFERENCE_NODE = 5,
        ENTITY_NODE = 6,
        NOTATION_NODE = 12,
        XPATH_NAMESPACE_NODE = 13,
    };

[Myles C. Maxfield]

Comment on attachment 242377 [details]
Patch

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

>>>>>> Source/WebCore/dom/Node.idl:-47
>>>>>> -    const unsigned short      NOTATION_NODE                  = 12;
>>>>> 
>>>>> I missed this additional web exposed change in my earlier comment.
>>>> 
>>>> Chrome actually kept that one. Firefox also still supports it. I haven't tested IE but I think we may want to keep this.
>>> 
>>> Yeah; I think it makes sense to keep the unmodified IDL even if we don't support some part of it.
>> 
>> Unfortunately, if this is present, our codegen step requires the relevant value in Node.h, and if the enum value is in Node.h, all the places where we switch over the NodeType have to include this value, which is what this patch is attempting to remove. Looks like I've got to take it out here as well.
> 
> Blink dealt with it by moving the deprecated values to a new enum:
> enum NodeType {
>         ELEMENT_NODE = 1,
>         ATTRIBUTE_NODE = 2,
>         TEXT_NODE = 3,
>         CDATA_SECTION_NODE = 4,
>         PROCESSING_INSTRUCTION_NODE = 7,
>         COMMENT_NODE = 8,
>         DOCUMENT_NODE = 9,
>         DOCUMENT_TYPE_NODE = 10,
>         DOCUMENT_FRAGMENT_NODE = 11,
>     };
> 
>     // Entity, EntityReference, Notation, and XPathNamespace nodes are impossible to create in Blink.
>     // But for compatibility reasons we want these enum values exist in JS, and this enum makes the bindings
>     // generation not complain about ENTITY_REFERENCE_NODE being missing from the implementation
>     // while not requiring all switch(NodeType) blocks to include this deprecated constant.
>     enum DeprecatedNodeType {
>         ENTITY_REFERENCE_NODE = 5,
>         ENTITY_NODE = 6,
>         NOTATION_NODE = 12,
>         XPATH_NAMESPACE_NODE = 13,
>     };

That's a good solution!

[Myles C. Maxfield]

Created attachment 242454 [details]
Patch

[WebKit Commit Bot]

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


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


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

[Myles C. Maxfield]

Style is a false negative

[Build Bot]

Comment on attachment 242454 [details]
Patch

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

New failing tests:
http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html

[Build Bot]

Created attachment 242468 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

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

[Myles C. Maxfield]

Created attachment 242477 [details]
Patch

[Chris Dumez]

Comment on attachment 242477 [details]
Patch

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

> Source/WebCore/dom/NodeFilter.idl:-47
> -    const unsigned long       SHOW_NOTATION                  = 0x00000800;

Both Firefox and Chrome still support this constant.

[Myles C. Maxfield]

Comment on attachment 242477 [details]
Patch

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

>> Source/WebCore/dom/NodeFilter.idl:-47
>> -    const unsigned long       SHOW_NOTATION                  = 0x00000800;
> 
> Both Firefox and Chrome still support this constant.

Given that notations will never exist, I don't think it's useful to be able to filter on them. However, it's probably beneficial to split this patch up, and move the SHOW_NOTATION part out to another patch.

[Chris Dumez]

Comment on attachment 242477 [details]
Patch

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

>>> Source/WebCore/dom/NodeFilter.idl:-47
>>> -    const unsigned long       SHOW_NOTATION                  = 0x00000800;
>> 
>> Both Firefox and Chrome still support this constant.
> 
> Given that notations will never exist, I don't think it's useful to be able to filter on them. However, it's probably beneficial to split this patch up, and move the SHOW_NOTATION part out to another patch.

It is not a matter of being useful or not. The fact is that we would be the first web-browser to un-expose NodeFilter.SHOW_NOTATION. It may be risky. I am less worried about un-exposing window.Notation because several other browsers managed to do it without breaking content.

[Myles C. Maxfield]

Created attachment 242500 [details]
Patch

[WebKit Commit Bot]

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


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


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

[Myles C. Maxfield]

style is a false negative. looks like gtk-wk2 is also a false negative.

[Kent Tamura]

Comment on attachment 242500 [details]
Patch

Looks ok

[WebKit Commit Bot]

Comment on attachment 242500 [details]
Patch

Clearing flags on attachment: 242500

Committed r177297: <http://trac.webkit.org/changeset/177297>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.