Bug 176236 – Add a ResourceBundle abstraction

• Component: Web Template Framework
• Status: RESOLVED INVALID

[Don Olmstead]

Currently there is CFBundle for CF and GResource for GTK. Provide an interface that can be used across ports.

[Don Olmstead]

Currently blocking removal of CFLite from WinCairo as a number of places are using webkitBundle which returns a CFBundleRef.

[Yusuke Suzuki]

Currently I think JSC does not use resource bundles (not sure about the inspector part, but JSCOnly does not). So putting it in PAL would be good.

[Brent Fulgham]

This sounds like a great idea!

[Don Olmstead]

Created attachment 449527 [details]
WIP Patch

[Don Olmstead]

Created attachment 449548 [details]
WIP Patch

[Don Olmstead]

Created attachment 449551 [details]
WIP Patch

[Don Olmstead]

Created attachment 449606 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 449606 [details]
Patch

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

r=me with comment.

> Source/WTF/wtf/ResourceBundle.cpp:63
> +    String fileName = name + "."_s + type;

Let's use makeString here, which is more efficient.

makeString(name, "."_s, type)

> Source/WTF/wtf/ResourceBundle.h:45
> +    WTF_EXPORT_PRIVATE static std::unique_ptr<ResourceBundle> create(String);

Should we use `const String&`?
(We cannot use StringView here since we need to convert it to CFString via String's method).

> Source/WTF/wtf/ResourceBundle.h:55
> +    WTF_EXPORT_PRIVATE String resourcePath(String name, String type, String directory) const;
> +    WTF_EXPORT_PRIVATE URL resourceURL(String name, String type, String directory) const;

Should we use `const String&`?
(We cannot use StringView here since we need to convert it to CFString via String's method).

[Darin Adler]

Comment on attachment 449606 [details]
Patch

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

>> Source/WTF/wtf/ResourceBundle.h:45
>> +    WTF_EXPORT_PRIVATE static std::unique_ptr<ResourceBundle> create(String);
> 
> Should we use `const String&`?
> (We cannot use StringView here since we need to convert it to CFString via String's method).

We have StringView::createCFString. I don’t see any reason we couldn’t use that.

>> Source/WTF/wtf/ResourceBundle.h:55
>> +    WTF_EXPORT_PRIVATE URL resourceURL(String name, String type, String directory) const;
> 
> Should we use `const String&`?
> (We cannot use StringView here since we need to convert it to CFString via String's method).

Ditto.

[Darin Adler]

Comment on attachment 449606 [details]
Patch

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

>> Source/WTF/wtf/ResourceBundle.cpp:63
>> +    String fileName = name + "."_s + type;
> 
> Let's use makeString here, which is more efficient.
> 
> makeString(name, "."_s, type)

I also prefer makeString, but for the record, makeString is likely not more efficient here because the String operator+ algorithm has optimizations so there are no intermediate strings created.

[Darin Adler]

Comment on attachment 449606 [details]
Patch

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

>>> Source/WTF/wtf/ResourceBundle.cpp:63
>>> +    String fileName = name + "."_s + type;
>> 
>> Let's use makeString here, which is more efficient.
>> 
>> makeString(name, "."_s, type)
> 
> I also prefer makeString, but for the record, makeString is likely not more efficient here because the String operator+ algorithm has optimizations so there are no intermediate strings created.

But with makeString we can do a tiny bit better performance with this:

    makeString(name, '.', type)

Maybe some day we will optimize makeString for string literals, but until then, single characters will be slightly faster in makeString than single-character strings.

[Don Olmstead]

Created attachment 449612 [details]
Patch

[Darin Adler]

Comment on attachment 449612 [details]
Patch

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

> Source/WTF/wtf/cf/ResourceBundleCF.cpp:73
> +    RetainPtr<CFURLRef> requestedURLRef = CFBundleCopyResourceURL(m_bundle.get(), name.createCFString().get(), type.createCFString().get(), directory.createCFString().get());

This will leak the URL. Needs to use adoptCF:

    auto requestedURL = adoptCF(CFBundleCopyResourceURL(m_bundle.get(), name.createCFString().get(), type.createCFString().get(), directory.createCFString().get()));

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

Same problem here, calling CFBundleCopyResourceURL without adoptCF will lead to a leak.

[Yusuke Suzuki]

Oh! That's a good catch!

[Don Olmstead]

Created attachment 449660 [details]
Patch

[Don Olmstead]

Believe I have all the adoptCF code added. I do want to see what the API tests on Mac return to make sure the new tests pass.

[Darin Adler]

Comment on attachment 449660 [details]
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();

[Don Olmstead]

Created attachment 449666 [details]
Patch

[Darin Adler]

Comment on attachment 449666 [details]
Patch

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

> Source/WTF/wtf/ResourceBundle.cpp:64
> +    auto fileName = makeString(name, "."_s, type);

I suggested '.' rather than "."_s, which will have better performance.

> Source/WTF/wtf/cf/ResourceBundleCF.cpp:47
> +#if PLATFORM(WIN)
> +        kCFURLWindowsPathStyle,
> +#else
> +        kCFURLPOSIXPathStyle,
> +#endif

Seems like it would be elegant to put a constant at the top of the file instead of putting it inside each function

#if PLATFORM(WIN)
constexpr auto pathStyle = kCFURLWindowsPathStyle;
#else
constexpr auto pathStyle = kCFURLPOSIXPathStyle;
#endif

Then the calls to CFURLCreateWithFileSystemPath and CFURLCopyFileSystemPath could just be normal calls that would fit fairly well in a single line.

Feel free to use a longer name if you think it would be less ambiguous, but honestly I don’t know what more to say rather than "this is the path style we use here", so I think the short name would work.

> Source/WTF/wtf/cf/ResourceBundleCF.cpp:75
> +        return String();

Sometimes we use { } instead of String() in a line like this and I slightly prefer it.

[Don Olmstead]

Created attachment 449677 [details]
Patch

[Don Olmstead]

Created attachment 449678 [details]
Patch

[Don Olmstead]

Fixed in 253925@main