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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=176236

Attachment 449666: Patch

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




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