Delete Notation because we don't use it
Created attachment 242377 [details] Patch
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 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
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
Isn't this removal a behavior change for the web, though? Notation nodes are in DOM Level 2.
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 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 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 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 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
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 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
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 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 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.
(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 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!
Created attachment 242454 [details] Patch
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.
Style is a false negative
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
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
Created attachment 242477 [details] Patch
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 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 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.
Created attachment 242500 [details] Patch
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.
style is a false negative. looks like gtk-wk2 is also a false negative.
Comment on attachment 242500 [details] Patch Looks ok
Comment on attachment 242500 [details] Patch Clearing flags on attachment: 242500 Committed r177297: <http://trac.webkit.org/changeset/177297>
All reviewed patches have been landed. Closing bug.