Bug 176236 - Add a ResourceBundle abstraction
Summary: Add a ResourceBundle abstraction
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords:
Depends on:
Blocks: 175336
  Show dependency treegraph
 
Reported: 2017-09-01 11:30 PDT by Don Olmstead
Modified: 2022-01-21 11:54 PST (History)
13 users (show)

See Also:


Attachments
WIP Patch (10.19 KB, patch)
2022-01-19 15:52 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (14.50 KB, patch)
2022-01-19 20:23 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (14.49 KB, patch)
2022-01-19 20:39 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (24.84 KB, patch)
2022-01-20 13:27 PST, Don Olmstead
ysuzuki: review+
Details | Formatted Diff | Diff
Patch (27.06 KB, patch)
2022-01-20 14:47 PST, Don Olmstead
darin: review-
Details | Formatted Diff | Diff
Patch (27.20 KB, patch)
2022-01-21 08:25 PST, Don Olmstead
darin: review-
Details | Formatted Diff | Diff
Patch (27.05 KB, patch)
2022-01-21 09:29 PST, Don Olmstead
darin: review+
Details | Formatted Diff | Diff
Patch (26.83 KB, patch)
2022-01-21 11:45 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (26.82 KB, patch)
2022-01-21 11:54 PST, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2017-09-01 11:30:17 PDT
Currently there is CFBundle for CF and GResource for GTK. Provide an interface that can be used across ports.
Comment 1 Don Olmstead 2017-09-01 11:31:10 PDT
Currently blocking removal of CFLite from WinCairo as a number of places are using webkitBundle which returns a CFBundleRef.
Comment 2 Yusuke Suzuki 2017-09-01 22:31:26 PDT
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.
Comment 3 Brent Fulgham 2019-02-21 16:25:37 PST
This sounds like a great idea!
Comment 4 Don Olmstead 2022-01-19 15:52:18 PST Comment hidden (obsolete)
Comment 5 Don Olmstead 2022-01-19 20:23:54 PST Comment hidden (obsolete)
Comment 6 Don Olmstead 2022-01-19 20:39:59 PST Comment hidden (obsolete)
Comment 7 Don Olmstead 2022-01-20 13:27:26 PST
Created attachment 449606 [details]
Patch
Comment 8 Yusuke Suzuki 2022-01-20 13:58:35 PST
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).
Comment 9 Darin Adler 2022-01-20 14:00:12 PST
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.
Comment 10 Darin Adler 2022-01-20 14:06:04 PST
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.
Comment 11 Darin Adler 2022-01-20 14:07:04 PST
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.
Comment 12 Don Olmstead 2022-01-20 14:47:01 PST
Created attachment 449612 [details]
Patch
Comment 13 Darin Adler 2022-01-20 15:25:42 PST
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.
Comment 14 Yusuke Suzuki 2022-01-20 17:04:12 PST
Oh! That's a good catch!
Comment 15 Don Olmstead 2022-01-21 08:25:31 PST
Created attachment 449660 [details]
Patch
Comment 16 Don Olmstead 2022-01-21 08:36:46 PST
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.
Comment 17 Darin Adler 2022-01-21 08:57:44 PST
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();
Comment 18 Don Olmstead 2022-01-21 09:29:33 PST
Created attachment 449666 [details]
Patch
Comment 19 Darin Adler 2022-01-21 10:56:49 PST
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.
Comment 20 Don Olmstead 2022-01-21 11:45:31 PST
Created attachment 449677 [details]
Patch
Comment 21 Don Olmstead 2022-01-21 11:54:09 PST
Created attachment 449678 [details]
Patch