[webkit-reviews] review granted: [Bug 116096] DOMFileSystemBase: merge BlackBerry implementation : [Attachment 202793] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alberto Garcia <agarcia at igalia.com>'s
request for review:
Bug 116096: DOMFileSystemBase: merge BlackBerry implementation
https://bugs.webkit.org/show_bug.cgi?id=116096

Attachment 202793: Patch
https://bugs.webkit.org/attachment.cgi?id=202793&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=202793&action=review


This is OK, but not ideal. A few comments below.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:90
> +	   if (path.isEmpty() || path[0] != '/')
> +	       return false;

The path.isEmpty() check here is redundant. Please remove it. I think I made
this same comment on an earlier version of this patch.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:92
> +	   if (path.find(temporaryPathPrefix, 1) == 1) {

find(x, 1) == 1 is an inefficient way to check if a prefix is present. That’s
because it will keep searching for the prefix in the whole string to return an
exact value of where we found it then afterward just check if it’s 1. It’s just
not a good way to write the code. The good way to write it would be with
hasPrefix, which we could easily do if temporaryPathPrefix had a "/" in it.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:95
> +	   } else if (path.find(persistentPathPrefix, 1) == 1) {

Same comment as above.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:102
> +	   if (path.isEmpty() || path[0] != '/')
> +	       return false;

The path.isEmpty() check here is redundant. Please remove it. I think I made
this same comment on an earlier version of this patch.

> Source/WebCore/Modules/filesystem/DOMFileSystemBase.cpp:104
> +	   filePath.swap(path);

A little strange to do a swap here instead of an assignment. I suppose it’s
more efficient.