Bug 192821 – <rdar://problem/46194315> macOS: WebKit1 does not handle occlusion changes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

<rdar://problem/46194315> macOS: WebKit1 does not handle occlusion changes

[Benjamin Poulain]

Created attachment 357595 [details]
Patch

[Benjamin Poulain]

<rdar://problem/46194315>

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Benjamin Poulain]

Created attachment 357613 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Benjamin Poulain]

Created attachment 357633 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Benjamin Poulain]

Created attachment 357649 [details]
Patch

[Benjamin Poulain]

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? :)

[Chris Dumez]

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?

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Created attachment 357746 [details]
Patch

[Benjamin Poulain]

> > 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.

[Benjamin Poulain]

Thanks for the review.

[WebKit Commit Bot]

Comment on attachment 357746 [details]
Patch

Clearing flags on attachment: 357746

Committed r239417: <https://trac.webkit.org/changeset/239417>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

I think this might have caused 2 API tests to time out:
Timeout

    TestWebKitAPI.FullscreenZoomInitialFrame.WebKit
    TestWebKitAPI.PageVisibilityStateWithWindowChanges.WebKit

[Benjamin Poulain]

(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

[Chris Dumez]

(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...

[Ryan Haddad]

I'm going to roll this out so that we can get the bots back to green in the meantime.

[Ryan Haddad]

Reverted r239417 for reason:

Introduced two API test failures on macOS.

Committed r239453: <https://trac.webkit.org/changeset/239453>

[Benjamin Poulain]

Created attachment 357898 [details]
Patch

[Benjamin Poulain]

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 :(

[Chris Dumez]

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.

[Benjamin Poulain]

(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.

[WebKit Commit Bot]

Comment on attachment 357898 [details]
Patch

Clearing flags on attachment: 357898

Committed r239496: <https://trac.webkit.org/changeset/239496>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.