Bug 188747 – Refactoring: eliminate raw pointer usage in Fullscreen code

• Component: New Bugs
• Status: REOPENED

[Jer Noble]

Refactoring: eliminate raw pointer usage in Fullscreen code

[Jer Noble]

Created attachment 347511 [details]
Patch

[Jer Noble]

Created attachment 347515 [details]
Patch

[EWS Watchlist]

Comment on attachment 347515 [details]
Patch

Attachment 347515 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8919554

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 347526 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 347515 [details]
Patch

Attachment 347515 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8919903

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 347531 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 347515 [details]
Patch

Attachment 347515 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8919689

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 347533 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 347515 [details]
Patch

Attachment 347515 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8920159

New failing tests:
media/audio-playback-restriction-autoplay.html
webrtc/peer-connection-track-end.html
webrtc/video-autoplay.html
media/media-fullscreen-pause-inline.html
media/video-main-content-allow.html
media/video-crash-invisible-autoplay-display-none.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-video-rgba4444.html
webrtc/video-update-often.html
media/modern-media-controls/time-label/time-label-white-space-nowrap.html
media/audio-playback-restriction-play-muted.html
media/video-main-content-autoplay.html
imported/w3c/web-platform-tests/webrtc/simplecall.https.html
media/remote-control-command-is-user-gesture.html
media/remove-video-best-media-element-in-main-frame-crash.html
media/video-restricted-no-preload-metadata.html
media/video-restricted-no-preload-auto.html
media/video-main-content-allow-then-deny.html
media/muted-video-is-playing-audio.html
media/media-fullscreen-loop-inline.html
media/controls/ipad/close-page-with-picture-in-picture-video-assertion-failure.html
fast/canvas/webgl/oes-texture-half-float.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-video.html
webrtc/video-with-data-channel.html

[EWS Watchlist]

Created attachment 347537 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Ryosuke Niwa]

Comment on attachment 347515 [details]
Patch

Looks like a bunch of tests are failing. r- for that.

[Radar WebKit Bug Importer]

<rdar://problem/43541164>

[Jer Noble]

Created attachment 347841 [details]
Patch

[Jer Noble]

Created attachment 347847 [details]
Patch

[EWS Watchlist]

Comment on attachment 347847 [details]
Patch

Attachment 347847 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8949724

New failing tests:
media/controls/ipad/close-page-with-picture-in-picture-video-assertion-failure.html

[EWS Watchlist]

Created attachment 347870 [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.4

[Jer Noble]

Created attachment 347922 [details]
Patch

[Jer Noble]

Okay, I think this patch is ready for review.

[Jer Noble]

Created attachment 349452 [details]
Patch

[Alex Christensen]

Comment on attachment 349452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349452&action=review

> Source/WTF/wtf/WeakPtrContainer.h:56
> +    void removeNullMembers()

This probably doesn't need to be public.

> Source/WTF/wtf/WeakPtrContainer.h:73
> +            removeNullMembers();

This makes two passes.  This whole thing could be done inside of removeAllMatching and only have one pass.

[Alex Christensen]

Comment on attachment 349452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349452&action=review

> Source/WTF/wtf/WeakPtrContainer.h:34
> +class WeakPtrContainer {

WeakPtrSet?

[Jer Noble]

(In reply to Alex Christensen from comment #20)
> Comment on attachment 349452 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349452&action=review
> 
> > Source/WTF/wtf/WeakPtrContainer.h:56
> > +    void removeNullMembers()
> 
> This probably doesn't need to be public.

Well, given the next comment:

> > Source/WTF/wtf/WeakPtrContainer.h:73
> > +            removeNullMembers();
> 
> This makes two passes.  This whole thing could be done inside of
> removeAllMatching and only have one pass.

We can remove the 'removeNullMembers()' method entirely.

(In reply to Alex Christensen from comment #21)
> Comment on attachment 349452 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349452&action=review
> 
> > Source/WTF/wtf/WeakPtrContainer.h:34
> > +class WeakPtrContainer {
> 
> WeakPtrSet?

Nothing keeps the same WeakPtr from being added multiple times.

[EWS Watchlist]

Comment on attachment 349452 [details]
Patch

Attachment 349452 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9180030

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

Created attachment 349486 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349452 [details]
Patch

Attachment 349452 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9180004

New failing tests:
media/controls/ipad/close-page-with-picture-in-picture-video-assertion-failure.html

[EWS Watchlist]

Created attachment 349491 [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.13.4

[Jer Noble]

Created attachment 349784 [details]
Patch

[EWS Watchlist]

Comment on attachment 349784 [details]
Patch

Attachment 349784 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9218787

New failing tests:
media/restricted-audio-playback-with-document-gesture.html

[EWS Watchlist]

Created attachment 349795 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Jer Noble]

Created attachment 349838 [details]
Patch

[Alex Christensen]

Comment on attachment 349838 [details]
Patch

I don't want to encourage patches this big, but r=me

[Alex Christensen]

Don't break the 32-bit build

[Jer Noble]

Created attachment 351008 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 351008 [details]
Patch for landing

Clearing flags on attachment: 351008

Committed r236605: <https://trac.webkit.org/changeset/236605>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 190087

[Jer Noble]

Created attachment 351130 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 351130 [details]
Patch for landing

Clearing flags on attachment: 351130

Committed r236624: <https://trac.webkit.org/changeset/236624>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

media/controls/ipad/close-page-with-picture-in-picture-video-assertion-failure.html is consistently crashing at WebKit::PlaybackSessionInterfaceContext::isPictureInPictureSupportedChanged(bool) on iOS Simulator after this change:

https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Release%20WK2%20(Tests)/r236674%20(159)/media/controls/ipad/close-page-with-picture-in-picture-video-assertion-failure-crash-log.txt

[Jer Noble]

(In reply to Ryan Haddad from comment #40)
> media/controls/ipad/close-page-with-picture-in-picture-video-assertion-
> failure.html is consistently crashing at
> WebKit::PlaybackSessionInterfaceContext::
> isPictureInPictureSupportedChanged(bool) on iOS Simulator after this change:
> 
> https://build.webkit.org/results/
> Apple%20iOS%2012%20Simulator%20Release%20WK2%20(Tests)/r236674%20(159)/media/
> controls/ipad/close-page-with-picture-in-picture-video-assertion-failure-
> crash-log.txt

Filed follow-up bug #190170.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 190207

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #42)
> Re-opened since this is blocked by bug 190207

This was rolled out in https://trac.webkit.org/changeset/236748/webkit due to the crashes seen on the bots.

[Ryan Haddad]

*** Bug 190170 has been marked as a duplicate of this bug. ***