Summary: | Change NavigatorContentUtils client ownership from port side to NavigatorContentUtils | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||
Component: | New Bugs | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, bunhere, cdumez, commit-queue, darin, gyuyoung.kim, sergio | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2014-04-07 05:29:20 PDT
Created attachment 228727 [details]
WIP
Created attachment 228729 [details]
Patch
CC'ing Darin and Andersca. I try to convert from OwnPtr to std::unique_ptr again for NavigatorContentUtils. Comment on attachment 228729 [details]
Patch
This is not our normal idioms for client objects. It’s not all that common to have a client that is owned by the object it’s passed to. The real issue here isn’t use of std::unique_ptr at all. It’s changing the ownership model. I don’t see any strong reason to do this.
Comment on attachment 228729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228729&action=review > Source/WebCore/ChangeLog:3 > + Convert OwnPtr to std::unique_ptr in NavigatorContentUtils This is not relevant to converting OwnPtr to unique_ptr. It’s just a separate ownership change that need not be done. The only thing that involves OwnPtr is the misleading title of the bug. (In reply to comment #4) > (From update of attachment 228729 [details]) > This is not our normal idioms for client objects. It’s not all that common to have a client that is owned by the object it’s passed to. The real issue here isn’t use of std::unique_ptr at all. It’s changing the ownership model. I don’t see any strong reason to do this. Yes, right. My goal in this patch is to pass the client ownership from ewk_view to NavigatorContentUtils. Because "navigatorContentUtilsClient" of ewk_view's _Ewk_View_Private_Data is only being used to be passed to provideNavigatorContentUtilsTo(). There isn't any reason that _Ewk_View_Private_Data manages the navigatorContentUtilsClient client/ownership. That's why I wanna change ownership model. My original patch in Bug 130958 mentioned this, though there was wrong description :( (https://bugs.webkit.org/show_bug.cgi?id=130958#c0) Created attachment 228787 [details]
Patch
(In reply to comment #5) > (From update of attachment 228729 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228729&action=review > > > Source/WebCore/ChangeLog:3 > > + Convert OwnPtr to std::unique_ptr in NavigatorContentUtils > > This is not relevant to converting OwnPtr to unique_ptr. It’s just a separate ownership change that need not be done. The only thing that involves OwnPtr is the misleading title of the bug. I change the wrong bug title. If you have still concern, please let me know. Comment on attachment 228787 [details]
Patch
While this is not the client pattern we use elsewhere, there’s no real reason not to do it this way here.
Comment on attachment 228787 [details] Patch Clearing flags on attachment: 228787 Committed r166915: <http://trac.webkit.org/changeset/166915> All reviewed patches have been landed. Closing bug. |