Bug 188747 - Refactoring: eliminate raw pointer usage in Fullscreen code
Summary: Refactoring: eliminate raw pointer usage in Fullscreen code
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
: 190170 (view as bug list)
Depends on: 190087 190207
Blocks: 190170
  Show dependency treegraph
 
Reported: 2018-08-20 10:38 PDT by Jer Noble
Modified: 2018-10-02 10:12 PDT (History)
8 users (show)

See Also:


Attachments
Patch (99.26 KB, patch)
2018-08-20 10:58 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (101.29 KB, patch)
2018-08-20 11:28 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch (151.00 KB, patch)
2018-08-22 14:22 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch (166.52 KB, patch)
2018-09-11 14:19 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch for landing (170.99 KB, patch)
2018-09-27 14:55 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (170.94 KB, patch)
2018-09-28 15:48 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2018-08-20 10:38:39 PDT
Refactoring: eliminate raw pointer usage in Fullscreen code
Comment 1 Jer Noble 2018-08-20 10:58:16 PDT
Created attachment 347511 [details]
Patch
Comment 2 Jer Noble 2018-08-20 11:28:50 PDT
Created attachment 347515 [details]
Patch
Comment 3 EWS Watchlist 2018-08-20 12:16:28 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2018-08-20 12:16:30 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2018-08-20 12:38:30 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-08-20 12:38:31 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-08-20 12:42:38 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-08-20 12:42:40 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2018-08-20 13:43:33 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-08-20 13:43:34 PDT Comment hidden (obsolete)
Comment 11 Ryosuke Niwa 2018-08-20 19:33:42 PDT
Comment on attachment 347515 [details]
Patch

Looks like a bunch of tests are failing. r- for that.
Comment 12 Radar WebKit Bug Importer 2018-08-20 19:34:21 PDT
<rdar://problem/43541164>
Comment 13 Jer Noble 2018-08-22 13:52:13 PDT
Created attachment 347841 [details]
Patch
Comment 14 Jer Noble 2018-08-22 14:22:27 PDT
Created attachment 347847 [details]
Patch
Comment 15 EWS Watchlist 2018-08-22 16:21:48 PDT Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-08-22 16:21:50 PDT Comment hidden (obsolete)
Comment 17 Jer Noble 2018-08-23 08:18:39 PDT
Created attachment 347922 [details]
Patch
Comment 18 Jer Noble 2018-08-23 09:29:36 PDT
Okay, I think this patch is ready for review.
Comment 19 Jer Noble 2018-09-11 14:19:29 PDT
Created attachment 349452 [details]
Patch
Comment 20 Alex Christensen 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.
Comment 21 Alex Christensen 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?
Comment 22 Jer Noble 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.
Comment 23 EWS Watchlist 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
Comment 24 EWS Watchlist 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
Comment 25 EWS Watchlist 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
Comment 26 EWS Watchlist 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
Comment 27 Jer Noble 2018-09-14 11:27:17 PDT
Created attachment 349784 [details]
Patch
Comment 28 EWS Watchlist 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
Comment 29 EWS Watchlist 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
Comment 30 Jer Noble 2018-09-14 17:37:21 PDT
Created attachment 349838 [details]
Patch
Comment 31 Alex Christensen 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
Comment 32 Alex Christensen 2018-09-17 16:45:01 PDT
Don't break the 32-bit build
Comment 33 Jer Noble 2018-09-27 14:55:31 PDT
Created attachment 351008 [details]
Patch for landing
Comment 34 WebKit Commit Bot 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>
Comment 35 WebKit Commit Bot 2018-09-28 11:40:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 WebKit Commit Bot 2018-09-28 13:35:11 PDT
Re-opened since this is blocked by bug 190087
Comment 37 Jer Noble 2018-09-28 15:48:10 PDT
Created attachment 351130 [details]
Patch for landing
Comment 38 WebKit Commit Bot 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>
Comment 39 WebKit Commit Bot 2018-09-28 16:31:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Ryan Haddad 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
Comment 41 Jer Noble 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.
Comment 42 WebKit Commit Bot 2018-10-02 10:04:48 PDT
Re-opened since this is blocked by bug 190207
Comment 43 Ryan Haddad 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.
Comment 44 Ryan Haddad 2018-10-02 10:12:46 PDT
*** Bug 190170 has been marked as a duplicate of this bug. ***