[webkit-reviews] review denied: [Bug 132700] Update local storage path : [Attachment 231091] bug #

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied Martin Hock
<mhock at apple.com>'s request for review:
Bug 132700: Update local storage path
https://bugs.webkit.org/show_bug.cgi?id=132700

Attachment 231091: bug #
https://bugs.webkit.org/attachment.cgi?id=231091&action=review

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=231091&action=review


r- to address review feedback.

> Source/WebKit/mac/Storage/WebStorageManager.mm:154
> +    NSArray *paths = NSSearchPathForDirectoriesInDomains(NSLibraryDirectory,
NSUserDomainMask, YES);
> +    NSString *libraryDirectory = [paths objectAtIndex:0];

How do we know that NSSearchPathForDirectoriesInDomains() will only ever return
exactly one result?

We should at least assert here that the length of the NSArray is what we
expect:

    NSArray *paths = NSSearchPathForDirectoriesInDomains(NSLibraryDirectory,
NSUserDomainMask, YES);
    ASSERT([paths count] == 1);
    NSString *libraryDirectory = [paths objectAtIndex:0];

Also, objectAtIndex: will throw an exception if the index is out of range. 
Maybe that's so rare/unexpected that we don't care to check, but if uncaught,
this might prevent any app from launching.  I think we should check and simply
return early.

Note that if you return early, sLocalStoragePath needs to be retained above!

> Source/WebKit/mac/Storage/WebStorageManager.mm:159
> +	   // sLocalStoragePath doesn't begin with libraryDirectory, so fix it
> +	   // if a legacy path exists, replace the obsolete part with
libraryDirectory
> +	   // else, use default storage path

Nit: Sentences need periods at the end of them (lines 157, 159), and start with
capital letters if not variables.

> Source/WebKit/mac/Storage/WebStorageManager.mm:162
> +	   sLocalStoragePath = [libraryDirectory
stringByAppendingPathComponent:(match == -1) ? @"WebKit/LocalStorage" :
[sLocalStoragePath substringFromIndex:match + matchLength]];

Nit: I would find this code easier to read if the ternary statement was pulled
out into its own variable:

	NSString *storageSubdirectory = (match == -1) ? @"WebKit/LocalStorage"
: [sLocalStoragePath substringFromIndex:match + matchLength];
	sLocalStoragePath = [libraryDirectory
stringByAppendingPathComponent:storageSubdirectory];