RESOLVED CONFIGURATION CHANGED188747
Refactoring: eliminate raw pointer usage in Fullscreen code 
https://bugs.webkit.org/show_bug.cgi?id=188747
Summary Refactoring: eliminate raw pointer usage in Fullscreen code
Jer Noble
Reported 2018-08-20 10:38:39 PDT
Refactoring: eliminate raw pointer usage in Fullscreen code
Attachments
Patch (99.26 KB, patch)
2018-08-20 10:58 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch (101.29 KB, patch)
2018-08-20 11:28 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews102 for mac-sierra (2.66 MB, application/zip)
2018-08-20 12:16 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.04 MB, application/zip)
2018-08-20 12:38 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews113 for mac-sierra (3.49 MB, application/zip)
2018-08-20 12:42 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (3.30 MB, application/zip)
2018-08-20 13:43 PDT, EWS Watchlist 		no flags
Details
Patch (149.62 KB, patch)
2018-08-22 13:52 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch (151.00 KB, patch)
2018-08-22 14:22 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.42 MB, application/zip)
2018-08-22 16:21 PDT, EWS Watchlist 		no flags
Details
Patch (164.99 KB, patch)
2018-08-23 08:18 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch (166.52 KB, patch)
2018-09-11 14:19 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.39 MB, application/zip)
2018-09-11 16:15 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.49 MB, application/zip)
2018-09-11 16:42 PDT, EWS Watchlist 		no flags
Details
Patch (169.25 KB, patch)
2018-09-14 11:27 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.17 MB, application/zip)
2018-09-14 13:22 PDT, EWS Watchlist 		no flags
Details
Patch (171.11 KB, patch)
2018-09-14 17:37 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch for landing (170.99 KB, patch)
2018-09-27 14:55 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch for landing (170.94 KB, patch)
2018-09-28 15:48 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Show Obsolete (14) View All Add attachment proposed patch, testcase, etc.
Jer Noble
Comment 1 2018-08-20 10:58:16 PDT
Created attachment 347511 [details] Patch
Jer Noble
Comment 2 2018-08-20 11:28:50 PDT
Created attachment 347515 [details] Patch
EWS Watchlist
Comment 3 2018-08-20 12:16:28 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 4 2018-08-20 12:16:30 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 5 2018-08-20 12:38:30 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 6 2018-08-20 12:38:31 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 7 2018-08-20 12:42:38 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 8 2018-08-20 12:42:40 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 9 2018-08-20 13:43:33 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 10 2018-08-20 13:43:34 PDT Comment hidden (obsolete)
Ryosuke Niwa
Comment 11 2018-08-20 19:33:42 PDT
Comment on attachment 347515 [details] Patch Looks like a bunch of tests are failing. r- for that.
Radar WebKit Bug Importer
Comment 12 2018-08-20 19:34:21 PDT
<rdar://problem/43541164>
Jer Noble
Comment 13 2018-08-22 13:52:13 PDT
Created attachment 347841 [details] Patch
Jer Noble
Comment 14 2018-08-22 14:22:27 PDT
Created attachment 347847 [details] Patch
EWS Watchlist
Comment 15 2018-08-22 16:21:48 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 16 2018-08-22 16:21:50 PDT Comment hidden (obsolete)
Jer Noble
Comment 17 2018-08-23 08:18:39 PDT
Created attachment 347922 [details] Patch
Jer Noble
Comment 18 2018-08-23 09:29:36 PDT
Okay, I think this patch is ready for review.
Jer Noble
Comment 19 2018-09-11 14:19:29 PDT
Created attachment 349452 [details] Patch
Alex Christensen
Comment 20 2018-09-11 15:23:48 PDT
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 21 2018-09-11 15:27:10 PDT
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
Comment 22 2018-09-11 15:34:25 PDT
(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 23 2018-09-11 16:15:12 PDT
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
Comment 24 2018-09-11 16:15:14 PDT
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 25 2018-09-11 16:42:17 PDT
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
Comment 26 2018-09-11 16:42:19 PDT
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
Comment 27 2018-09-14 11:27:17 PDT
Created attachment 349784 [details] Patch
EWS Watchlist
Comment 28 2018-09-14 13:22:20 PDT
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
Comment 29 2018-09-14 13:22:22 PDT
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
Comment 30 2018-09-14 17:37:21 PDT
Created attachment 349838 [details] Patch
Alex Christensen
Comment 31 2018-09-17 16:44:07 PDT
Comment on attachment 349838 [details] Patch I don't want to encourage patches this big, but r=me
Alex Christensen
Comment 32 2018-09-17 16:45:01 PDT
Don't break the 32-bit build
Jer Noble
Comment 33 2018-09-27 14:55:31 PDT
Created attachment 351008 [details] Patch for landing
WebKit Commit Bot
Comment 34 2018-09-28 11:39:59 PDT
Comment on attachment 351008 [details] Patch for landing Clearing flags on attachment: 351008 Committed r236605: <https://trac.webkit.org/changeset/236605>
WebKit Commit Bot
Comment 35 2018-09-28 11:40:01 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 36 2018-09-28 13:35:11 PDT
Re-opened since this is blocked by bug 190087
Jer Noble
Comment 37 2018-09-28 15:48:10 PDT
Created attachment 351130 [details] Patch for landing
WebKit Commit Bot
Comment 38 2018-09-28 16:31:37 PDT
Comment on attachment 351130 [details] Patch for landing Clearing flags on attachment: 351130 Committed r236624: <https://trac.webkit.org/changeset/236624>
WebKit Commit Bot
Comment 39 2018-09-28 16:31:39 PDT
All reviewed patches have been landed. Closing bug.
Ryan Haddad
Comment 40 2018-10-01 12:48:37 PDT
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
Comment 41 2018-10-01 15:19:46 PDT
(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
Comment 42 2018-10-02 10:04:48 PDT
Re-opened since this is blocked by bug 190207
Ryan Haddad
Comment 43 2018-10-02 10:11:46 PDT
(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
Comment 44 2018-10-02 10:12:46 PDT
*** Bug 190170 has been marked as a duplicate of this bug. ***
Frédéric Wang Nélar
Comment 45 2026-10-20 02:00:31 PDT
> + Two sources of raw pointers in the Fullscreen code: > + - Model classes (PlaybackSessionModel and VideoFullscreenModel) aren't ref-able, so > + they are passed around as raw references. > + - Observer classes (PlaybackSessionModelClient and VideoFullscreenModelClient, and > + VideoFullscreenChangeObserver) are also passed around as raw pointers, but shouldn't > + be ref-able. I think the changes in the patch description are essentially obsolete since these classes can now use smart pointers: - PlaybackSessionModel and PlaybackSessionModelClient: https://searchfox.org/wubkat/rev/4419a857fd628dde9bc8e2cbc8625be40f46ca70/Source/WebCore/platform/cocoa/PlaybackSessionModel.h#65 https://searchfox.org/wubkat/rev/4419a857fd628dde9bc8e2cbc8625be40f46ca70/Source/WebCore/platform/cocoa/PlaybackSessionModel.h#157 - VideoFullscreenChangeObserver was merged into VideoFullscreenModel in https://commits.webkit.org/265195@main - VideoFullscreenModel and VideoFullscreenModelClient (renamed in https://commits.webkit.org/268850@main): https://searchfox.org/wubkat/rev/4419a857fd628dde9bc8e2cbc8625be40f46ca70/Source/WebCore/platform/cocoa/VideoPresentationModel.h#56 https://searchfox.org/wubkat/rev/4419a857fd628dde9bc8e2cbc8625be40f46ca70/Source/WebCore/platform/cocoa/VideoPresentationModel.h#118 The patch is quite big (as commented by Alex) and the code has changed a lot, so it's a bit difficult to know whether some parts are still relevant. It seems the change to close-page-with-picture-in-picture-video-assertion-failure.html can still be taken though.
Note You need to log in before you can comment on or make changes to this bug.