[webkit-reviews] review granted: [Bug 51878] Add WebKit1 API to view and delete local storage : [Attachment 84520] Patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has granted Anton D'Auria
<adauria at apple.com>'s request for review:
Bug 51878: Add WebKit1 API to view and delete local storage
https://bugs.webkit.org/show_bug.cgi?id=51878

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

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84520&action=review

I'm going to say r+ because all my comments were matters of style, not
substance, and it looked like previous comments have been addressed. However, I
think you should look at the style comments and consider addressing them. A new
patch is not needed for most, but please do post a new patch if you decide to
make a listDirectory() that uses POSIX calls, since code for that could use
fresh review.

> Source/WebCore/page/PageGroup.cpp:140
> +void PageGroup::testForceLocalStorageSync()

What does this function do? The name is a little unclear.

OK, after seeing the comment below, I get the purpose, but I think it would
help clarify to take "test" out of the name, even though the function is for
testing. As it is, it sounds like it tests forcing something, but it just syncs
local storage. In fact, just syncLocalStorage might be a good name, if the
"force" does not add an important distinction.

> Source/WebCore/platform/mac/FileSystemMac.mm:106
> +Vector<String> listDirectory(const String& path, const String& filter)
> +{
> +    Vector<String> entries;
> +
> +    // This can run on a background thread, so must create an autorelease
pool.
> +    NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
> +
> +    NSDirectoryEnumerator *dir = [[NSFileManager defaultManager]
enumeratorAtPath:path]; 
> +    NSString *file;
> +    while ((file = [dir nextObject])) {
> +	   if (filter.isEmpty() || ([file rangeOfString:filter].location !=
NSNotFound))
> +	       entries.append([path stringByAppendingPathComponent:file]);
> +    }
> +
> +    [pool release];
> +
> +    return entries;
> +}

I think it would be better to write this using POSIX calls and put it in
FileSystemPOSIX. That's how most mac FileSystem calls are implemented; it makes
the code more portable, and avoids the need for an autorelease pool.

> Source/WebCore/storage/LocalStorageTask.h:63
> +	   LocalStorageTask(Type, const String&);
> +	   LocalStorageTask(Type, const String&, const String&);

The String parameters here should probably have names in their declarations,
since the names are not obvious from context.

> Source/WebCore/storage/StorageNamespaceImpl.cpp:168
> +void StorageNamespaceImpl::testForceSync()

Similar naming issue here - "test" is not being used as a verb, so this doesn't
match WebKit naming style.

> Source/WebCore/storage/StorageTracker.h:100
> +    // FIXME: do we need an ownptr here?
> +    OwnPtr<OriginSet> m_originSet;

You're the one adding it - you should know if we need the OwnPtr! However, I
think it would be fine to just have this data member be an OriginSet rather
than an OwbPtr<OriginSet>. There's no obvious reason for the extra allocation
and indirection.

> Source/WebCore/storage/StorageTracker.h:102
> +    OwnPtr<OriginSet> m_originsBeingDeleted;

Likewise, I think the PwnPtr here is not needed, and it can just be an
OriginSet, not a smart pointer to one.

> Source/WebKit/chromium/src/StorageNamespaceProxy.cpp:97
> +    // FIXME: Implement.

Why a FIXME instead of notImplemented?

> Source/WebKit/chromium/src/StorageNamespaceProxy.cpp:102
> +    // FIXME: Implement.

Why a FIXME instead of notImplemented?

> Source/WebKit/chromium/src/StorageNamespaceProxy.cpp:107
> +    // FIXME: Implement for tests that require a sync to backing db.

Why a FIXME instead of notImplemented?

> Tools/DumpRenderTree/LayoutTestController.cpp:399
> +    // Has mac implementation

This comment seems unnecessary.

> Tools/DumpRenderTree/LayoutTestController.cpp:409
> +    // Has mac implementation

ditto

> Tools/DumpRenderTree/LayoutTestController.cpp:426
> +    // Has mac implementation

ditto

> Tools/DumpRenderTree/LayoutTestController.cpp:435
> +    // Has mac implementation

ditto