Bug 131299 – Change NavigatorContentUtils client ownership from port side to NavigatorContentUtils

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

As a step to convert from OwnPtr to std::unique_ptr, this patch convert OwnPtr to std::unique_ptr in NavigatorContentUtils.

[Gyuyoung Kim]

Created attachment 228727 [details]
WIP

[Gyuyoung Kim]

Created attachment 228729 [details]
Patch

[Gyuyoung Kim]

CC'ing Darin and Andersca. I try to convert from OwnPtr to std::unique_ptr again for NavigatorContentUtils.

[Darin Adler]

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.

[Darin Adler]

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.

[Gyuyoung Kim]

(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)

[Gyuyoung Kim]

Created attachment 228787 [details]
Patch

[Gyuyoung Kim]

(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.

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 228787 [details]
Patch

Clearing flags on attachment: 228787

Committed r166915: <http://trac.webkit.org/changeset/166915>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.