Bug 139171

Summary: Delete Notation because we don't use it
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, dino, jonlee, rniwa, simon.fraser, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Patch
none
Patch none

Description Myles C. Maxfield 2014-12-01 19:21:12 PST
Delete Notation because we don't use it
Comment 1 Myles C. Maxfield 2014-12-01 19:23:34 PST
Created attachment 242377 [details]
Patch
Comment 2 WebKit Commit Bot 2014-12-01 19:25:05 PST
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.
Comment 3 Build Bot 2014-12-01 21:00:15 PST
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
Comment 4 Build Bot 2014-12-01 21:00:18 PST
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
Comment 5 Simon Fraser (smfr) 2014-12-01 21:26:02 PST
Isn't this removal a behavior change for the web, though? Notation nodes are in DOM Level 2.
Comment 6 Chris Dumez 2014-12-01 21:31:30 PST
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.
Comment 7 Chris Dumez 2014-12-01 21:38:17 PST
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.
Comment 8 Chris Dumez 2014-12-01 21:39:39 PST
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.
Comment 9 Chris Dumez 2014-12-01 21:43:28 PST
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.
Comment 10 Build Bot 2014-12-01 21:50:59 PST
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
Comment 11 Build Bot 2014-12-01 21:51:03 PST
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
Comment 12 Build Bot 2014-12-02 00:01:37 PST
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
Comment 13 Build Bot 2014-12-02 00:01:42 PST
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
Comment 14 Myles C. Maxfield 2014-12-02 11:54:54 PST
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.
Comment 15 Myles C. Maxfield 2014-12-02 12:08:37 PST
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.
Comment 16 Chris Dumez 2014-12-02 12:11:30 PST
(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,
    };
Comment 17 Myles C. Maxfield 2014-12-02 12:18:24 PST
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!
Comment 18 Myles C. Maxfield 2014-12-02 15:08:30 PST
Created attachment 242454 [details]
Patch
Comment 19 WebKit Commit Bot 2014-12-02 15:10:42 PST
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.
Comment 20 Myles C. Maxfield 2014-12-02 16:24:32 PST
Style is a false negative
Comment 21 Build Bot 2014-12-02 17:36:50 PST
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
Comment 22 Build Bot 2014-12-02 17:36:55 PST
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
Comment 23 Myles C. Maxfield 2014-12-02 20:12:58 PST
Created attachment 242477 [details]
Patch
Comment 24 Chris Dumez 2014-12-02 20:18:14 PST
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.
Comment 25 Myles C. Maxfield 2014-12-02 20:50:36 PST
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.
Comment 26 Chris Dumez 2014-12-02 20:56:57 PST
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.
Comment 27 Myles C. Maxfield 2014-12-03 10:45:50 PST
Created attachment 242500 [details]
Patch
Comment 28 WebKit Commit Bot 2014-12-03 10:50:04 PST
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.
Comment 29 Myles C. Maxfield 2014-12-03 13:01:01 PST
style is a false negative. looks like gtk-wk2 is also a false negative.
Comment 30 Kent Tamura 2014-12-08 13:36:20 PST
Comment on attachment 242500 [details]
Patch

Looks ok
Comment 31 WebKit Commit Bot 2014-12-15 11:39:54 PST
Comment on attachment 242500 [details]
Patch

Clearing flags on attachment: 242500

Committed r177297: <http://trac.webkit.org/changeset/177297>
Comment 32 WebKit Commit Bot 2014-12-15 11:39:59 PST
All reviewed patches have been landed.  Closing bug.