[Webkit-unassigned] [Bug 116096] DOMFileSystemBase: merge BlackBerry implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=116096


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #202793|review?, commit-queue?      |review+, commit-queue-
               Flag|                            |




--- Comment #8 from Darin Adler <darin at apple.com>  2013-05-24 17:35:21 PST ---
(From update of attachment 202793)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.