Use std::make_unique<> when creating std::unique_ptr<>

• Component: New Bugs
• Status: RESOLVED FIXED

[Gyuyoung Kim]

Remove unnecessary create() factory.

[Gyuyoung Kim]

Created attachment 271992 [details]
Patch

[Anders Carlsson]

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.

[Gyuyoung Kim]

Created attachment 272175 [details]
Patch

[Gyuyoung Kim]

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

[Alex Christensen]

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

[Gyuyoung Kim]

Created attachment 272305 [details]
Patch

[Gyuyoung Kim]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Gyuyoung Kim]

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 ?

[Gyuyoung Kim]

Created attachment 272655 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 272655 [details]
Patch

Clearing flags on attachment: 272655

Committed r197532: <http://trac.webkit.org/changeset/197532>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.