Bug 154574 - Use std::make_unique<> when creating std::unique_ptr<>
Summary: Use std::make_unique<> when creating std::unique_ptr<>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-22 23:38 PST by Gyuyoung Kim
Modified: 2016-03-03 16:54 PST (History)
3 users (show)

See Also:


Attachments
Patch (10.18 KB, patch)
2016-02-22 23:39 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (3.28 KB, patch)
2016-02-24 22:40 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (4.74 KB, patch)
2016-02-25 23:16 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (3.32 KB, patch)
2016-03-02 06:54 PST, 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 2016-02-22 23:38:00 PST
Remove unnecessary create() factory.
Comment 1 Gyuyoung Kim 2016-02-22 23:39:59 PST
Created attachment 271992 [details]
Patch
Comment 2 Anders Carlsson 2016-02-23 07:03:48 PST
Comment on attachment 271992 [details]
Patch

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

> Source/WebCore/Modules/webdatabase/DatabaseTracker.h:52
> -    // FIXME: This is a hack so we can easily delete databases from the UI process in WebKit2.
> -    WEBCORE_EXPORT static std::unique_ptr<DatabaseTracker> trackerWithDatabasePath(const String& databasePath);
> +    explicit DatabaseTracker(const String& databasePath);

I'd rather we kept this since we don't want people to create DatabaseTracker objects willy nilly. Having an explicit factory function makes that more clear.
Comment 3 Gyuyoung Kim 2016-02-24 22:40:32 PST
Created attachment 272175 [details]
Patch
Comment 4 Gyuyoung Kim 2016-02-24 22:42:01 PST
(In reply to comment #2)
> Comment on attachment 271992 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271992&action=review
> 
> > Source/WebCore/Modules/webdatabase/DatabaseTracker.h:52
> > -    // FIXME: This is a hack so we can easily delete databases from the UI process in WebKit2.
> > -    WEBCORE_EXPORT static std::unique_ptr<DatabaseTracker> trackerWithDatabasePath(const String& databasePath);
> > +    explicit DatabaseTracker(const String& databasePath);
> 
> I'd rather we kept this since we don't want people to create DatabaseTracker
> objects willy nilly. Having an explicit factory function makes that more
> clear.

I see. Let's keep it.
Comment 5 Alex Christensen 2016-02-25 08:22:23 PST
Comment on attachment 272175 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStatistics.h:47
>      static std::unique_ptr<Statistics> open(const String& cachePath);
> +    explicit Statistics(const String& databasePath);

As long as we're making the constructor public, let's get rid of Statistics::open, make the constructor take the cachePath, call pathByAppendingComponent in the new constructor, and replace the one call to Statistics::open with std::make_unique
Comment 6 Gyuyoung Kim 2016-02-25 23:16:26 PST
Created attachment 272305 [details]
Patch
Comment 7 Gyuyoung Kim 2016-02-26 02:28:55 PST
(In reply to comment #5)
> Comment on attachment 272175 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272175&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatistics.h:47
> >      static std::unique_ptr<Statistics> open(const String& cachePath);
> > +    explicit Statistics(const String& databasePath);
> 
> As long as we're making the constructor public, let's get rid of
> Statistics::open, make the constructor take the cachePath, call
> pathByAppendingComponent in the new constructor, and replace the one call to
> Statistics::open with std::make_unique

Ok, looks better it. Modified.
Comment 8 Darin Adler 2016-02-26 08:06:08 PST
Comment on attachment 272305 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStatistics.cpp:-78
> -    ASSERT(RunLoop::isMain());

What was the rationale for deleting this assertion?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStatistics.cpp:-81
> -    return std::unique_ptr<Statistics>(new Statistics(databasePath));

This is the line of code that we should have changed to use std::make_unique. I don’t think it was an obvious choice to remove the function entirely. I see some small problems that introduced that are not solved well.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStatistics.h:46
> -    static std::unique_ptr<Statistics> open(const String& cachePath);
> +    explicit Statistics(const String& databasePath, const String& cachePath = emptyString());

This doesn’t seem like a good change. It introduces a new source of error where you could pass in strings in the wrong order or be confused between the different strings.

If we want to make the constructor public then it should have only one argument, the same one that the public "open" function had.

I can’t figure out why we think the databasePath argument is needed. Who is calling the function with that non-empty? And if that function is needed it should be exposed in a private way, not unnecessarily exposed to public.
Comment 9 Darin Adler 2016-02-26 08:07:50 PST
Comment on attachment 272305 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/cache/NetworkCacheStatistics.h:46
>> +    explicit Statistics(const String& databasePath, const String& cachePath = emptyString());
> 
> This doesn’t seem like a good change. It introduces a new source of error where you could pass in strings in the wrong order or be confused between the different strings.
> 
> If we want to make the constructor public then it should have only one argument, the same one that the public "open" function had.
> 
> I can’t figure out why we think the databasePath argument is needed. Who is calling the function with that non-empty? And if that function is needed it should be exposed in a private way, not unnecessarily exposed to public.

Sorry, I missed Alex’s earlier comment.

I don’t agree that these two argument form of the constructor is the way to go. I’d rather see named functions if type is not going to distinguish what’s going on. This approach where we pass two strings, but only one must be non-empty, does not seem to be a good design.
Comment 10 Gyuyoung Kim 2016-02-27 23:57:51 PST
Comment on attachment 272305 [details]
Patch

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

>>> Source/WebKit2/NetworkProcess/cache/NetworkCacheStatistics.h:46
>>> +    explicit Statistics(const String& databasePath, const String& cachePath = emptyString());
>> 
>> This doesn’t seem like a good change. It introduces a new source of error where you could pass in strings in the wrong order or be confused between the different strings.
>> 
>> If we want to make the constructor public then it should have only one argument, the same one that the public "open" function had.
>> 
>> I can’t figure out why we think the databasePath argument is needed. Who is calling the function with that non-empty? And if that function is needed it should be exposed in a private way, not unnecessarily exposed to public.
> 
> Sorry, I missed Alex’s earlier comment.
> 
> I don’t agree that these two argument form of the constructor is the way to go. I’d rather see named functions if type is not going to distinguish what’s going on. This approach where we pass two strings, but only one must be non-empty, does not seem to be a good design.

Do you mean it is better to keep open() function ?
Comment 11 Gyuyoung Kim 2016-03-02 06:54:26 PST
Created attachment 272655 [details]
Patch
Comment 12 WebKit Commit Bot 2016-03-03 16:54:12 PST
Comment on attachment 272655 [details]
Patch

Clearing flags on attachment: 272655

Committed r197532: <http://trac.webkit.org/changeset/197532>
Comment 13 WebKit Commit Bot 2016-03-03 16:54:17 PST
All reviewed patches have been landed.  Closing bug.