[webkit-reviews] review granted: [Bug 176236] Add a ResourceBundle abstraction : [Attachment 449666] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 21 10:56:49 PST 2022

Darin Adler <darin at apple.com> has granted Don Olmstead
<don.olmstead at sony.com>'s request for review:
Bug 176236: Add a ResourceBundle abstraction

Attachment 449666: Patch


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

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
> +	   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

constexpr auto pathStyle = kCFURLWindowsPathStyle;
constexpr auto pathStyle = kCFURLPOSIXPathStyle;

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.

More information about the webkit-reviews mailing list