[webkit-reviews] review denied: [Bug 176236] Add a ResourceBundle abstraction : [Attachment 449660] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Don Olmstead <don.olmstead at sony.com>'s
request for review:
Bug 176236: Add a ResourceBundle abstraction
https://bugs.webkit.org/show_bug.cgi?id=176236

Attachment 449660: Patch

https://bugs.webkit.org/attachment.cgi?id=449660&action=review




--- Comment #17 from Darin Adler <darin at apple.com> ---
Comment on attachment 449660
  --> https://bugs.webkit.org/attachment.cgi?id=449660
Patch

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

> Source/WTF/wtf/ResourceBundle.cpp:45
> +    return std::unique_ptr<ResourceBundle>(new ResourceBundle(pathString));

This should be WTFMove(pathString) to avoid some reference count churn.

> Source/WTF/wtf/ResourceBundle.cpp:49
> +    : m_root(root)

This should be WTFMove(root) to avoid some reference count churn.

> Source/WTF/wtf/ResourceBundle.h:48
> +    inline operator CFBundleRef() const { return m_bundle.get(); }

The "inline" here is not needed, has no effect.

> Source/WTF/wtf/cf/ResourceBundleCF.cpp:69
> +    auto bundleURL = adoptCF(CFBundleCopyBundleURL(m_bundle.get()));
> +    return URL(bundleURL.get());

I think this is better as a one liner without a local.

    return adoptCF(CFBundleCopyBundleURL(m_bundle.get())).get();

Also, there is no need to explicitly specify the URL constructor. It’s not an
explicit constructor.

> Source/WTF/wtf/cf/ResourceBundleCF.cpp:74
> +    auto requestedURLRef = adoptCF(CFBundleCopyResourceURL(m_bundle.get(),
name.createCFString().get(), type.createCFString().get(),
directory.createCFString().get()));

I think the "Ref" in the name of this local variable is misleading. We normally
wouldn’t use that name for something of in a RetainPtr. Why not just call this
requestedURL?

> Source/WTF/wtf/cf/ResourceBundleCF.cpp:84
> +    static constexpr size_t maxPathSize = 4096;
> +    UInt8 requestedFilePath[maxPathSize];
> +
> +    if (!CFURLGetFileSystemRepresentation(requestedURLRef.get(), false,
requestedFilePath, maxPathSize))
> +	   return String();
> +
> +    return String(requestedFilePath);

This is incorrect, and will interpret the path as Latin-1 instead of UTF-8,
incorrectly handling any path with any non-ASCII characters.

Since we are returning a WTF::String and not a C string containing a file
system representation, we must use CFURLCopyFileSystemPath, not
CFURLGetFileSystemRepresentation.

> Source/WTF/wtf/cf/ResourceBundleCF.cpp:90
> +    auto bundleURL = adoptCF(CFBundleCopyResourceURL(m_bundle.get(),
name.createCFString().get(), type.createCFString().get(),
directory.createCFString().get()));
> +    return URL(bundleURL.get());

return adoptCF(CFBundleCopyResourceURL(m_bundle.get(),
name.createCFString().get(), type.createCFString().get(),
directory.createCFString().get())).get();