|
Description
Aishwarya Nirmal
2017-10-30 10:47:38 PDT
Created attachment 325362 [details]
Patch
Attachment 325362 [details] did not pass style-queue:
ERROR: Source/WebCore/JSHTMLMenuitemElement.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5]
Total errors found: 1 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 325362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325362&action=review > Source/WebCore/ChangeLog:7 > + I think it should be possible to write a simple test for this. I'll look for one to mimic. > Source/WebCore/JSHTMLMenuitemElement.h:1 > +/* This doesn't seem right. The generated file shouldn't be in the WebKit source tree, it should get generated into the build directory (and not show up in your patch). You may need help getting Xcode to do the right thing, it's not exactly straightforward. (In reply to Tim Horton from comment #3) > Comment on attachment 325362 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=325362&action=review > > > Source/WebCore/ChangeLog:7 > > + > > I think it should be possible to write a simple test for this. I'll look for > one to mimic. Maybe something in LayoutTests/fast/html? There are a bunch that end in -element that seem like sensible starting points. Created attachment 325364 [details]
Patch
(In reply to Tim Horton from comment #4) > (In reply to Tim Horton from comment #3) > > Comment on attachment 325362 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=325362&action=review > > > > > Source/WebCore/ChangeLog:7 > > > + > > > > I think it should be possible to write a simple test for this. I'll look for > > one to mimic. > > Maybe something in LayoutTests/fast/html? There are a bunch that end in > -element that seem like sensible starting points. Sounds good! I think I fixed the issue with the JSHTML file, so I'll look into creating some tests. Comment on attachment 325364 [details] Patch Attachment 325364 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5042101 New failing tests: imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html Created attachment 325368 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 325364 [details] Patch Attachment 325364 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5042110 New failing tests: imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html Created attachment 325370 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 325364 [details] Patch Attachment 325364 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5042140 New failing tests: imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html Created attachment 325371 [details]
Archive of layout-test-results from ews116 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 325364 [details] Patch Attachment 325364 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5042152 New failing tests: imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html Created attachment 325373 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 325364 [details]
Patch
r- since the patch is failing to build.
(In reply to Ryosuke Niwa from comment #15) > Comment on attachment 325364 [details] > Patch > > r- since the patch is failing to build. It's not, the test is failing. Aishwarya, you'll want to find out why imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical asserts that HTMLMenuItemElement *doesn't* exist :P (In reply to Tim Horton from comment #16) > (In reply to Ryosuke Niwa from comment #15) > > Comment on attachment 325364 [details] > > Patch > > > > r- since the patch is failing to build. > > It's not, the test is failing. > > Aishwarya, you'll want to find out why > imported/w3c/web-platform-tests/html/semantics/interactive-elements/ > contextmenu-historical asserts that HTMLMenuItemElement *doesn't* exist :P GTK+, Windows, and WPE seem to be failing to build? Have you confirmed that they build locally somehow? It builds locally for me. Looking at that file, imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical, it seems that there are some tests ensuring that HTMLMenuItemElement does not exist. We probably don't want to check for this anymore, so I can work on editing the test. (In reply to Aishwarya Nirmal from comment #18) > It builds locally for me. rniwa is right, something is up with the CMake build. I think it doesn't use DerivedSources.make, so maybe look for other files that mention an adjacent derived source to find out what mechanism it does use. Created attachment 325391 [details]
Patch
Comment on attachment 325391 [details] Patch Attachment 325391 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5044601 New failing tests: imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html Created attachment 325398 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 325391 [details] Patch Attachment 325391 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5044716 New failing tests: imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html Created attachment 325401 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 325391 [details] Patch Attachment 325391 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5044751 New failing tests: imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html Created attachment 325404 [details]
Archive of layout-test-results from ews115 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 325391 [details] Patch Attachment 325391 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5044815 New failing tests: imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html Created attachment 325406 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
The test is failing because it's expecting HTMLMenuItemElement to be not defined in the global scope. You have run "run-webkit-tests LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html --reset" to reset the expected results. This is because the feature had been removed from the HTML specification in https://github.com/whatwg/html/pull/2742. Comment on attachment 325391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325391&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). You need to describe what kind of API you're implementing and what code changes you're making for it. r- due to the lack of change log descriptions and the test failures. > Source/WebCore/ChangeLog:8 > + * CMakeLists.txt: Given the feature has been removed from the HTML specification, we should put this under a runtime flag. Created attachment 325482 [details]
Patch
Comment on attachment 325482 [details] Patch Attachment 325482 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5054553 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/sandbox_032.htm Created attachment 325501 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
The test that is failing seems to be unrelated to my changes. When I run run-webkit-tests LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/sandbox_032.htm --debug locally, the test passes. Comment on attachment 325482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325482&action=review > Source/WebCore/ChangeLog:12 > + is set to false by default. Nits: There are two trailing spaces here. > Source/WebCore/ChangeLog:13 > + Please add some references to old HTML specifications like https://www.w3.org/TR/2013/WD-html51-20130528/interactive-elements.html. > Source/WebKit/ChangeLog:8 > + Adds in the MenuItemEnabled flag and default value. Nit: A trailing space at the end. > Source/WebCore/html/HTMLMenuItemElement.idl:27 > +] interface HTMLMenuItemElement : HTMLElement { So the idea is to implement more attributes of https://www.w3.org/TR/2013/WD-html51-20130528/interactive-elements.html in the future patches? Yes! I will add those details to the change log. Created attachment 325633 [details]
Patch
Created attachment 325635 [details]
Patch
Comment on attachment 325635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325635&action=review > LayoutTests/fast/html/menuitem-element.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Please use <!DOCTYPE html> > LayoutTests/fast/html/menuitem-element.html:12 > + You either need to skip this test in TestExpectations or enable the feature using settings, etc... Created attachment 325768 [details]
Patch
Comment on attachment 325768 [details] Patch Attachment 325768 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5080207 New failing tests: fast/html/menuitem-element.html Created attachment 325776 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 325768 [details] Patch Attachment 325768 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5080229 New failing tests: fast/html/menuitem-element.html Created attachment 325778 [details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 325779 [details]
Patch
Comment on attachment 325779 [details] Patch Attachment 325779 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5081414 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting.https.html Created attachment 325795 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 325779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325779&action=review > Source/WebCore/ChangeLog:16 > + A specification for the menuitem element can be found at > + https://www.w3.org/TR/2013/WD-html51-20130528/interactive-elements.html#the-menuitem-element. > + More attributes of this element will be implemented in future patches. Should we be adding this? This is obsolete. More recent HTML specifications do not include this. See <https://html.spec.whatwg.org/#non-conforming-features>. Menuitem is noncorforming because Mozilla is the only implementor, but it does not seem to be entirely obsolete. The w3 folks seem to be willing to reconsider the removal of the menuitem if multiple browsers were to support it https://github.com/whatwg/html/issues/2929. We are aiming to use menuitem to represent the touch bar menu items for the touch bar web API. The API is experimental, so we are planning to use any information gathered to better inform a web standard for menuitem. (In reply to Aishwarya Nirmal from comment #49) > Menuitem is noncorforming because Mozilla is the only implementor, but it > does not seem to be entirely obsolete. The w3 folks seem to be willing to > reconsider the removal of the menuitem if multiple browsers were to support > it https://github.com/whatwg/html/issues/2929. OK, makes sense. Comment on attachment 325779 [details]
Patch
r=me since this patch can't possibly cause the service worker test failure.
Sorry for the delay. In the future, please also set cq? flag so that the committer can set cq+ flag to land the patch via commit queue.
Comment on attachment 325779 [details] Patch Clearing flags on attachment: 325779 Committed r224457: <https://trac.webkit.org/changeset/224457> All reviewed patches have been landed. Closing bug. |