Summary: | [EFL][WK1] Apply std::unique_ptr<> to NavigatorContentUtilsClientEfl | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||
Component: | New Bugs | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bunhere, cdumez, commit-queue, darin, gyuyoung.kim, sergio | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2014-03-30 23:00:46 PDT
Created attachment 228153 [details]
Patch for ews
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.
Comment on attachment 228153 [details]
Patch for ews
The patch itself looks good but it should use std::unique_ptr instead of OwnPtr/PassOwnPtr!
(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 ? (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? Created attachment 228696 [details]
Patch
(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. (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. 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. Created attachment 228714 [details]
Patch
(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. Created attachment 228715 [details]
Patch
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. Created attachment 228718 [details]
Patch for landing
Comment on attachment 228718 [details] Patch for landing Clearing flags on attachment: 228718 Committed r166866: <http://trac.webkit.org/changeset/166866> All reviewed patches have been landed. Closing bug. |