Bug 158642

Summary: Automatically manage ChromeClient lifetime
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: NEW    
Severity: Normal CC: ahmad.saleem792, andersca, bfulgham
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch bfulgham: review+

Alex Christensen
Reported 2016-06-10 16:56:07 PDT
Automatically manage ChromeClient lifetime
Attachments
Patch (51.20 KB, patch)
2016-06-10 17:01 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (65.35 KB, patch)
2016-06-10 21:13 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (65.77 KB, patch)
2016-06-10 22:05 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (67.90 KB, patch)
2016-06-10 22:14 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (68.51 KB, patch)
2016-06-10 23:06 PDT, Alex Christensen 		bfulgham: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Alex Christensen
Comment 1 2016-06-10 17:01:28 PDT
Created attachment 281063 [details] Patch
Alex Christensen
Comment 2 2016-06-10 21:13:19 PDT
Created attachment 281074 [details] Patch
Alex Christensen
Comment 3 2016-06-10 22:05:57 PDT
Created attachment 281077 [details] Patch
Alex Christensen
Comment 4 2016-06-10 22:14:18 PDT
Created attachment 281078 [details] Patch
Alex Christensen
Comment 5 2016-06-10 23:06:56 PDT
Created attachment 281080 [details] Patch
Darin Adler
Comment 6 2016-06-11 12:27:13 PDT
Comment on attachment 281080 [details] Patch The original idea was that a ChromeClient could be like a delegate in Cocoa programming. It didn’t always need to be a separate object and one existing object could be the ChromeClient and that same object could also be, say, the FrameLoaderClient. I don’t know if changing this is a big improvement. There’s a bit of a “rearranging the deck chairs” feeling for me in this patch. Could you talk with Anders and Sam, at least, about it?
Anders Carlsson
Comment 7 2016-06-11 15:56:20 PDT
(In reply to comment #6) > Comment on attachment 281080 [details] > Patch > > The original idea was that a ChromeClient could be like a delegate in Cocoa > programming. It didn’t always need to be a separate object and one existing > object could be the ChromeClient and that same object could also be, say, > the FrameLoaderClient. I don’t know if changing this is a big improvement. > There’s a bit of a “rearranging the deck chairs” feeling for me in this > patch. Could you talk with Anders and Sam, at least, about it? I've talked to Alex about this - originally we did this because on Windows, the WebView class acted as a bunch of clients. This has now changed, and the pattern of having separate objects for separate clients is used everywhere. I think having explicit lifetimes is more clear than all those uses of "delete this" everywhere.
Alex Christensen
Comment 8 2016-06-11 16:58:35 PDT
PageConfiguration has a potpourri of different lifetime management techniques. If they're all refcounted, then they could all be Ref's, and if they're all the same object then they could all be Ref's of the exact same object. Would refcounting all the clients be a good way to go?
Brent Fulgham
Comment 9 2016-07-07 12:58:53 PDT
Comment on attachment 281080 [details] Patch Looks good to me! r=me, but you need a WebKit2 owner to double-check the small WK2 change you made. Maybe you are a WK2 owner now and can just do the final approval?
Brent Fulgham
Comment 10 2016-07-07 12:59:21 PDT
(In reply to comment #9) > Comment on attachment 281080 [details] > Patch > > Looks good to me! r=me, but you need a WebKit2 owner to double-check the > small WK2 change you made. Maybe you are a WK2 owner now and can just do the > final approval? If you aren't a WK2 owner, remove the r+ and get Brady/Alexey/Anders/Sam to review!
Ahmad Saleem
Comment 11 2022-10-16 04:58:03 PDT
It seems that this r+ patch didn't landed as checked with "bugID" on Webkit GitHub. Do we need this now? Thanks!
Note You need to log in before you can comment on or make changes to this bug.