<rdar://problem/46194315> macOS: WebKit1 does not handle occlusion changes
Created attachment 357595 [details] Patch
<rdar://problem/46194315>
Comment on attachment 357595 [details] Patch Attachment 357595 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10462029 Number of test failures exceeded the failure limit.
Created attachment 357598 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 357595 [details] Patch Attachment 357595 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10462188 Number of test failures exceeded the failure limit.
Created attachment 357601 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 357613 [details] Patch
Comment on attachment 357613 [details] Patch Attachment 357613 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10463796 Number of test failures exceeded the failure limit.
Created attachment 357619 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 357613 [details] Patch Attachment 357613 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10464283 Number of test failures exceeded the failure limit.
Created attachment 357626 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 357633 [details] Patch
Comment on attachment 357633 [details] Patch Attachment 357633 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10466514 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/register-closed-window.https.html
Created attachment 357645 [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.13.6
Created attachment 357649 [details] Patch
The iOS failure looks like noise. The code is #ifdefed on iOS. I reuploaded to give EWS another chance. The results looks like DRT is dumping the layout instead of the text version. Chris, if you can't review this, who reviews WebKit1 patches nowadays? :)
Comment on attachment 357649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357649&action=review r=me with question. > Source/WebKitLegacy/mac/WebView/WebViewData.h:214 > + BOOL windowOcclusionDetectionEnabled; So we want this off by default? Why?
(In reply to Chris Dumez from comment #17) > View in context: > https://bugs.webkit.org/attachment.cgi?id=357649&action=review > > r=me with question. > > > Source/WebKitLegacy/mac/WebView/WebViewData.h:214 > > + BOOL windowOcclusionDetectionEnabled; > > So we want this off by default? Why? Because I am dumb. I bet I broke the patch when I added DRT support.
Created attachment 357746 [details] Patch
> > Source/WebKitLegacy/mac/WebView/WebViewData.h:214 > > + BOOL windowOcclusionDetectionEnabled; > > So we want this off by default? Why? Thanks so much for the good review Chris. You saved my ass. I verified this patch by hand after the DRT changes.
Thanks for the review.
Comment on attachment 357746 [details] Patch Clearing flags on attachment: 357746 Committed r239417: <https://trac.webkit.org/changeset/239417>
All reviewed patches have been landed. Closing bug.
I think this might have caused 2 API tests to time out: Timeout TestWebKitAPI.FullscreenZoomInitialFrame.WebKit TestWebKitAPI.PageVisibilityStateWithWindowChanges.WebKit
(In reply to Chris Dumez from comment #24) > I think this might have caused 2 API tests to time out: > Timeout > > TestWebKitAPI.FullscreenZoomInitialFrame.WebKit > TestWebKitAPI.PageVisibilityStateWithWindowChanges.WebKit Are those not tested by the bots? I'll have a look today
(In reply to Benjamin Poulain from comment #25) > (In reply to Chris Dumez from comment #24) > > I think this might have caused 2 API tests to time out: > > Timeout > > > > TestWebKitAPI.FullscreenZoomInitialFrame.WebKit > > TestWebKitAPI.PageVisibilityStateWithWindowChanges.WebKit > > Are those not tested by the bots? > > I'll have a look today No, still no API test ews...
I'm going to roll this out so that we can get the bots back to green in the meantime.
Reverted r239417 for reason: Introduced two API test failures on macOS. Committed r239453: <https://trac.webkit.org/changeset/239453>
Created attachment 357898 [details] Patch
Looks like running tests with occlusion is a big fragile. The tests appear under other windows and the timing of visibility change become unpredictable. I disabled occlusion support. I see WebKit2 also needed that in a couple of places :(
Comment on attachment 357898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357898&action=review > Tools/TestWebKitAPI/mac/WebKitAgnosticTest.mm:90 > + // The tests can be run concurrently. In that case, window can occlude each other and change visibility results. I thought we did not run API tests concurrently. The Window would likely not be visible on screen though.
(In reply to Chris Dumez from comment #31) > Comment on attachment 357898 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357898&action=review > > > Tools/TestWebKitAPI/mac/WebKitAgnosticTest.mm:90 > > + // The tests can be run concurrently. In that case, window can occlude each other and change visibility results. > > I thought we did not run API tests concurrently. The Window would likely not > be visible on screen though. I assumed we could from: --child-processes=CHILD_PROCESSES Number of processes to run in parallel.
Comment on attachment 357898 [details] Patch Clearing flags on attachment: 357898 Committed r239496: <https://trac.webkit.org/changeset/239496>