Bug 131299 - Change NavigatorContentUtils client ownership from port side to NavigatorContentUtils
Summary: Change NavigatorContentUtils client ownership from port side to NavigatorCont...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-07 05:29 PDT by Gyuyoung Kim
Modified: 2014-04-08 00:14 PDT (History)
7 users (show)

See Also:


Attachments
WIP (6.53 KB, patch)
2014-04-07 05:31 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (6.48 KB, patch)
2014-04-07 05:36 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (9.44 KB, patch)
2014-04-07 18:25 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2014-04-07 05:29:20 PDT
As a step to convert from OwnPtr to std::unique_ptr, this patch convert OwnPtr to std::unique_ptr in NavigatorContentUtils.
Comment 1 Gyuyoung Kim 2014-04-07 05:31:36 PDT
Created attachment 228727 [details]
WIP
Comment 2 Gyuyoung Kim 2014-04-07 05:36:42 PDT
Created attachment 228729 [details]
Patch
Comment 3 Gyuyoung Kim 2014-04-07 05:39:01 PDT
CC'ing Darin and Andersca. I try to convert from OwnPtr to std::unique_ptr again for NavigatorContentUtils.
Comment 4 Darin Adler 2014-04-07 13:29:33 PDT
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 5 Darin Adler 2014-04-07 13:31:05 PDT
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.
Comment 6 Gyuyoung Kim 2014-04-07 18:05:17 PDT
(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)
Comment 7 Gyuyoung Kim 2014-04-07 18:25:04 PDT
Created attachment 228787 [details]
Patch
Comment 8 Gyuyoung Kim 2014-04-07 18:26:03 PDT
(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 9 Darin Adler 2014-04-07 23:09:55 PDT
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 10 WebKit Commit Bot 2014-04-08 00:14:47 PDT
Comment on attachment 228787 [details]
Patch

Clearing flags on attachment: 228787

Committed r166915: <http://trac.webkit.org/changeset/166915>
Comment 11 WebKit Commit Bot 2014-04-08 00:14:56 PDT
All reviewed patches have been landed.  Closing bug.