Summary: | Add a ResourceBundle abstraction | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Don Olmstead <don.olmstead> | ||||||||||||||||||||
Component: | Web Template Framework | Assignee: | Don Olmstead <don.olmstead> | ||||||||||||||||||||
Status: | RESOLVED INVALID | ||||||||||||||||||||||
Severity: | Normal | CC: | annulen, aperez, benjamin, bfulgham, cdumez, cmarcelo, darin, ews-watchlist, gyuyoung.kim, ryuan.choi, sergio, yoshiaki.jitsukawa, ysuzuki | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 175336 | ||||||||||||||||||||||
Attachments: |
|
Description
Don Olmstead
2017-09-01 11:30:17 PDT
Currently blocking removal of CFLite from WinCairo as a number of places are using webkitBundle which returns a CFBundleRef. 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. This sounds like a great idea! Created attachment 449527 [details]
WIP Patch
Created attachment 449548 [details]
WIP Patch
Created attachment 449551 [details]
WIP Patch
Created attachment 449606 [details]
Patch
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 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 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 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. Created attachment 449612 [details]
Patch
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. Oh! That's a good catch! Created attachment 449660 [details]
Patch
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 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(); Created attachment 449666 [details]
Patch
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. Created attachment 449677 [details]
Patch
Created attachment 449678 [details]
Patch
Fixed in 253925@main |