Bug 130958 – [EFL][WK1] Apply std::unique_ptr<> to NavigatorContentUtilsClientEfl

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

[Gyuyoung Kim]

EFL and GTK ports have owned "navigatorContentUtilsClient" member variable. However, it is only used for passing a pointer of NavigatorContentUtilsClient. It is useless. To remove it, we need to change parameter type to OwnPtr|PassOwnPtr in NavigatorContentUtils classes. Besides OwnPtr|PassOwnPtr help to manage the client pointer more safely.

[Gyuyoung Kim]

Created attachment 228153 [details]
Patch for ews

[Gyuyoung Kim]

Comment on attachment 228153 [details]
Patch for ews

Child class of RefCountedSupplement can't use std::unique_ptr yet, because RefCountedSupplement still use PassRefPtr template. It would be good if we change it on new bug. Let me file a bug for it after landing this patch.

[Anders Carlsson]

Comment on attachment 228153 [details]
Patch for ews

The patch itself looks good but it should use std::unique_ptr instead of OwnPtr/PassOwnPtr!

[Gyuyoung Kim]

(In reply to comment #3)
> (From update of attachment 228153 [details])
> The patch itself looks good but it should use std::unique_ptr instead of OwnPtr/PassOwnPtr!

As I said, comment #2, we need to modify RefCountedSupplement to adjust std::unique_ptr. However, some classes are inherited it. So, I would like to change them with OwnPtr/PassPwnPtr first, then I would adjust the std::unique_ptr all together. Do you want change those at once ?

[Darin Adler]

(In reply to comment #2)
> Child class of RefCountedSupplement can't use std::unique_ptr yet, because RefCountedSupplement still use PassRefPtr template.

I don’t understand the connection between PassRefPtr and OwnPtr. Why can classes derived from RefCountedSupplement use OwnPtr safely? Shouldn’t it be RefPtr?

[Gyuyoung Kim]

Created attachment 228696 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #3)
> (From update of attachment 228153 [details])
> The patch itself looks good but it should use std::unique_ptr instead of OwnPtr/PassOwnPtr!

Ok, I apply std::unique_ptr to EFL port's implementation first before changing OwnPtr|PassOwnPtr in whole NavigatorContentUtils. It looks I need to have time to replace all.

[Gyuyoung Kim]

(In reply to comment #5)
> (In reply to comment #2)
> > Child class of RefCountedSupplement can't use std::unique_ptr yet, because RefCountedSupplement still use PassRefPtr template.
> 
> I don’t understand the connection between PassRefPtr and OwnPtr. Why can classes derived from RefCountedSupplement use OwnPtr safely? Shouldn’t it be RefPtr?

It looks there was misunderstanding. I knew we can't pass OwnPtr to RefPtr at that time. So, I thought that RefCountedSupplement needs to be modified as well. But, there seems to be no need to modify it. Sorry for noise.

[Darin Adler]

Comment on attachment 228696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228696&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:-263
> -    OwnPtr<WebCore::NavigatorContentUtilsClientEfl> navigatorContentUtilsClient;

This data member is still needed. It can be a std::unique_ptr, but we can’t just get rid of it.

> Source/WebKit/efl/ewk/ewk_view.cpp:687
> +    WebCore::provideNavigatorContentUtilsTo(priv->page.get(), std::make_unique<WebCore::NavigatorContentUtilsClientEfl>(smartData->self).get());

This is not going to work. It’s going to pass a pointer to provideNavigatorContentUtilsTo after deleting the NavigatorContentUtilsClientEfl object.

[Gyuyoung Kim]

Created attachment 228714 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #9)
> (From update of attachment 228696 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228696&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:-263
> > -    OwnPtr<WebCore::NavigatorContentUtilsClientEfl> navigatorContentUtilsClient;
> 
> This data member is still needed. It can be a std::unique_ptr, but we can’t just get rid of it.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:687
> > +    WebCore::provideNavigatorContentUtilsTo(priv->page.get(), std::make_unique<WebCore::NavigatorContentUtilsClientEfl>(smartData->self).get());
> 
> This is not going to work. It’s going to pass a pointer to provideNavigatorContentUtilsTo after deleting the NavigatorContentUtilsClientEfl object.

Oops, my mistake. thank you for pointing it out. Fixed.

[Gyuyoung Kim]

Created attachment 228715 [details]
Patch

[Darin Adler]

Comment on attachment 228715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228715&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:867
> +    priv->navigatorContentUtilsClient = nullptr;

This line of code is not needed. Calling "delete priv" below will take care of it.

[Gyuyoung Kim]

Created attachment 228718 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 228718 [details]
Patch for landing

Clearing flags on attachment: 228718

Committed r166866: <http://trac.webkit.org/changeset/166866>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.