Update local storage path

• Component: WebKit Misc.
• Status: RESOLVED INVALID

[Martin Hock]

Update local storage path
<rdar://problem/16178089>

[Martin Hock]

Created attachment 231090 [details]
patch

[Martin Hock]

Created attachment 231091 [details]
bug #

[Anton D'Auria]

This looks good. It's a little weird that you're assuming the UUID has a pattern. Do the dashes need to be in a fixed position? On the other hand, we only care about UUIDs of the past, which appear to have that pattern.

Should we only store relative paths from this point on so we can avoid this in the future?

[David Kilzer (:ddkilzer)]

Comment on attachment 231091 [details]
bug #

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];

[Alexey Proskuryakov]

Comment on attachment 231091 [details]
bug #

View in context: https://bugs.webkit.org/attachment.cgi?id=231091&action=review

> Source/WebKit/mac/Storage/WebStorageManager.mm:56
> +    // Return index of start of substring resembling the pattern /XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX/Library/

If this is the only case we need to support, then using a regular expression is a huge overkill, please just use regular string manipulation.

There could be some useful functions in FileSystem.h, too.

[Martin Hock]

Created attachment 231405 [details]
address comments

[Martin Hock]

(In reply to comment #4)
> (From update of attachment 231091 [details])
> 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!

I added the check and early return (with retain).

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

Fixed.

> > 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];

Fixed.

[Martin Hock]

(In reply to comment #5)
> (From update of attachment 231091 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231091&action=review
> 
> > Source/WebKit/mac/Storage/WebStorageManager.mm:56
> > +    // Return index of start of substring resembling the pattern /XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX/Library/
> 
> If this is the only case we need to support, then using a regular expression is a huge overkill, please just use regular string manipulation.
> 
> There could be some useful functions in FileSystem.h, too.

I have rewritten the code to avoid using a regular expression. It has the disadvantage of being substantially longer, though it is probably faster and should have very low memory overhead. I don't think that the old code had any significant performance or memory penalty, though.

[Alexey Proskuryakov]

Comment on attachment 231405 [details]
address comments

View in context: https://bugs.webkit.org/attachment.cgi?id=231405&action=review

I made some comments below, but then we talked and realized that this is not the right place for this code.

> Source/WebKit/mac/Storage/WebStorageManager.mm:38
> +#if PLATFORM(IOS)
> +#import <yarr/RegularExpression.h>
> +#endif

This is not needed any more.

> Source/WebKit/mac/Storage/WebStorageManager.mm:44
> +#if PLATFORM(IOS)
> +using JSC::Yarr::RegularExpression;
> +#endif

Neither is this.

> Source/WebKit/mac/Storage/WebStorageManager.mm:55
> +static bool isHex(const String& str, size_t indexStart, size_t indexEnd)

We have a function for this, isASCIIHexDigit() in ASCIICType.h.