|
Description
Emilio Cobos Álvarez
2017-05-11 12:23:10 PDT
Created attachment 309749 [details]
Patch
Comment on attachment 309749 [details] Patch Attachment 309749 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3720160 New failing tests: imported/w3c/web-platform-tests/innerText/getter.html Created attachment 309778 [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 309749 [details] Patch Attachment 309749 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3720040 New failing tests: imported/w3c/web-platform-tests/innerText/getter.html Created attachment 309779 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Antti implemented this feature so he should be reviewing your patches related to display: contents. Created attachment 309782 [details]
Patch
(In reply to Ryosuke Niwa from comment #6) > Antti implemented this feature so he should be reviewing your patches > related to display: contents. Sounds good, thanks! One of the first steps should be to import the tests from https://github.com/w3c/web-platform-tests/tree/master/css/css-display-3 Created attachment 309796 [details]
Patch
Created attachment 309797 [details]
Patch
Err, sorry for the noise, I can't land the patch myself yet. Last version has the "Reviewed by..." line, if somebody could land it for me that'd be awesome. (In reply to Antti Koivisto from comment #9) > One of the first steps should be to import the tests from > > https://github.com/w3c/web-platform-tests/tree/master/css/css-display-3 Yeah, I have patches locally for this, which I'll submit for review tomorrow, probably. Thanks for the review Antti! Comment on attachment 309797 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309797&action=review > Source/WebCore/ChangeLog:11 > + Add a RuntimeEnabledFeature for display: contents, defaulted to false. > + > + The "defaulted to false" is not only because there are spec issues, but because > + I ran the WPT suite, and there was a fair amount of crashes and messed render > + trees. > + > + https://bugs.webkit.org/show_bug.cgi?id=171984 > + > + Reviewed by Antti Koivisto. Change log here is really messed up. The bug title should be followed by a bug URL, followed by a blank line, then a long description. Comment on attachment 309797 [details] Patch Attachment 309797 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3721629 New failing tests: imported/w3c/web-platform-tests/innerText/getter.html Created attachment 309824 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 309797 [details] Patch Attachment 309797 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3721583 New failing tests: imported/w3c/web-platform-tests/innerText/getter.html Created attachment 309825 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Is there any reason this need to be a RuntimeEnabledFeature rather than a Setting? Created attachment 310084 [details]
Patch
(In reply to Ryosuke Niwa from comment #13) > Comment on attachment 309797 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309797&action=review > > > Source/WebCore/ChangeLog:11 > > + Add a RuntimeEnabledFeature for display: contents, defaulted to false. > > + > > + The "defaulted to false" is not only because there are spec issues, but because > > + I ran the WPT suite, and there was a fair amount of crashes and messed render > > + trees. > > + > > + https://bugs.webkit.org/show_bug.cgi?id=171984 > > + > > + Reviewed by Antti Koivisto. > > Change log here is really messed up. > > The bug title should be followed by a bug URL, followed by a blank line, > then a long description. Thanks for the heads-up, prepare-ChangeLog posted the whole git commit description before the url field, and no script complained about it, so I just assumed it was fine. I updated and amended it. (In reply to Sam Weinig from comment #18) > Is there any reason this need to be a RuntimeEnabledFeature rather than a > Setting? From a quick look RuntimeEnabledFeatures it what's usually used for web features, but happy to change it if there's a good reason to. Comment on attachment 310084 [details] Patch Attachment 310084 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3738112 New failing tests: imported/w3c/web-platform-tests/innerText/getter.html Created attachment 310085 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 310084 [details] Patch Attachment 310084 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3738128 New failing tests: imported/w3c/web-platform-tests/innerText/getter.html Created attachment 310086 [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
So I have no clue why the test is failing in OSX. I guess the test runner is not picking up the flag? Presumably I'm missing some WebView.mm bits or something? Not sure how should I proceed given I don't own a Mac. I guess I'll try some more monkey-see-monkey-do based on other preferences, probably I need to add it to the EXPERIMENTAL_FEATURES? I'll flag for review again once I get it working. Oh, and regarding Settings vs. RuntimeEnabledFeatures, it looks to me per [1] that RuntimeEnabledFeatures should be the right place for it, but maybe I'm missing something. [1]: https://lists.webkit.org/pipermail/webkit-dev/2017-January/028678.html Created attachment 310088 [details]
WIP
Created attachment 310089 [details]
WIP
Created attachment 310090 [details]
WIP
Created attachment 310092 [details]
Patch
Ok, that seemed to help, could I get the patch re-reviewed please? Thanks a lot in advance! Comment on attachment 310092 [details] Patch Clearing flags on attachment: 310092 Committed r216974: <http://trac.webkit.org/changeset/216974> All reviewed patches have been landed. Closing bug. Reverted r216974 for reason: Revision caused consistent timeouts on all platforms. Committed r216981: <http://trac.webkit.org/changeset/216981> The Builds and history: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2FinnerText%2Fgetter.html https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/builds/1602 https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/builds/1177 https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/1544 https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Debug%20WK2%20(Tests)/builds/1416 https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Release%20WK2%20(Tests)/builds/1523 The test are timing out on both wk1 and wk2, Release and Debug, of all macOS, iOS Simulator, and GTK Thanks for reopening Matt. I can repro after rebasing my checkout. I've bisected and it's caused by https://trac.webkit.org/changeset/216972/webkit. I still don't know enough about that code to know whether that's correct or the bug lies somewhere else, but I guess I'll need to come up with a fix... Err, I meant https://trac.webkit.org/changeset/216966/webkit I'm still debugging it, and it's fascinating. The change mentioned in the previous comment looks correct to me, and indeed the test passes in isolation. It seems all related to the following <svg> container in the getter.html. If I remove the SVG container (and the svg tests), it just works. If I remove only the svg tests, but I leave the svg container, the second display: contents test times out. Created attachment 310869 [details]
Patch
(In reply to Emilio Cobos Álvarez from comment #39) > Created attachment 310869 [details] > Patch I shouldn't have really uploaded this yet, sorry about that. It's just the same patch rebased, but apparently webkit-patch upload -g HEAD~ (where HEAD~ was the patch for bug 172443) did the wrong thing here. Anyway this should be good to go if/when bug 172443 lands. Comment on attachment 310869 [details] Patch Attachment 310869 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3794436 Number of test failures exceeded the failure limit. Created attachment 310874 [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 310869 [details] Patch Attachment 310869 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3794511 Number of test failures exceeded the failure limit. Created attachment 310878 [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 310869 [details] Patch Attachment 310869 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3794504 Number of test failures exceeded the failure limit. Created attachment 310880 [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 310869 [details] Patch Attachment 310869 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3794488 New failing tests: fast/forms/input-text-paste-maxlength.html fast/forms/input-text-maxlength.html accessibility/ios-simulator/speak-selection-content.html fast/forms/textarea-maxlength.html editing/text-iterator/count-matches-in-form.html editing/text-iterator/find-string-on-flat-tree.html fast/forms/number/number-validity-badinput.html fast/forms/input-value-sanitization.html Created attachment 310881 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 310911 [details]
Patch
Comment on attachment 310911 [details] Patch Attachment 310911 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3796056 New failing tests: imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html Created attachment 310921 [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
Comment on attachment 310911 [details] Patch Attachment 310911 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3796144 New failing tests: imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html Created attachment 310925 [details]
Archive of layout-test-results from ews102 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 310911 [details] Patch Attachment 310911 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3796129 New failing tests: imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html Created attachment 310928 [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 310911 [details] Patch Attachment 310911 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3796169 New failing tests: imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html Created attachment 310933 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
(In reply to Build Bot from comment #57) > Created attachment 310933 [details] > Archive of layout-test-results from ews126 for ios-simulator-wk2 > > The attached test failures were seen while running run-webkit-tests on the > ios-sim-ews. > Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6 Nothing like making your own patch turn red because of a test written by you :-) (https://github.com/w3c/web-platform-tests/pull/5830) Created attachment 310953 [details]
Patch
Comment on attachment 310953 [details]
Patch
Hi Antti, could I get this re-reviewed/relanded?
This is the same patch as before, just rebased, and with some expectations updated for a newly imported test too.
Comment on attachment 310953 [details] Patch Clearing flags on attachment: 310953 Committed r217273: <http://trac.webkit.org/changeset/217273> All reviewed patches have been landed. Closing bug. (In reply to WebKit Commit Bot from comment #61) > Comment on attachment 310953 [details] > Patch > > Clearing flags on attachment: 310953 > > Committed r217273: <http://trac.webkit.org/changeset/217273> It seems that imported/w3c/web-platform-tests/innerText/getter.html is hitting a flaky assertion failure on macOS and iOS after this change: https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r217275%20(1295)/results.html (In reply to Ryan Haddad from comment #63) > (In reply to WebKit Commit Bot from comment #61) > > Comment on attachment 310953 [details] > > Patch > > > > Clearing flags on attachment: 310953 > > > > Committed r217273: <http://trac.webkit.org/changeset/217273> > > It seems that imported/w3c/web-platform-tests/innerText/getter.html is > hitting a flaky assertion failure on macOS and iOS after this change: > > https://build.webkit.org/results/ > Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r217275%20(1295)/results.html Ugh, so that's a |Text| instance being removed, and it seems to be failing the !renderer() assertion, which looks plausible. I'll try to look into it during the next few days, but I'd prefer not to revert this patch if it's not too much of a priority, because the patch just enables testing support for a feature that is conditionally enabled right now, and all these issues indicate that there are potentially a lot of other related bugs lying around I think. Additionally, this test started failing on Windows: https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r217488%20(816)/imported/w3c/web-platform-tests/innerText/getter-pretty-diff.html |