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

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied  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 Jeremy Orlow <jorlow at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84520&action=review

Please don't commit this without letting me take a look.  Overall, it
definitely seems on track.  I have a couple major concerns though.

I still think migrating away from the manual tasks to the new template based
task creation is better.  The more I think about it, the more I think it'd be
best to do that in a patch that's a precursor to this, so when you're adding
the new logic you're doing it in the right way.  But if you still feel strongly
that you want to do it in a follow up, I guess that's OK.

Have you tried splitting this patch up at all?	It's really massive and there's
a lot of subtle stuff going on.

The rest of my concerns are inline:

> Source/WebCore/storage/StorageAreaSync.cpp:371
> +    

please remove the added white space

> Source/WebCore/storage/StorageAreaSync.cpp:461
> +}	    

ditto

> Source/WebCore/storage/StorageAreaSync.cpp:465
> +    syncTimerFired(&m_syncTimer);

This simply schedules a sync, but doesn't block on it finishing....does that
work for your tests?  If so, then you should probably rename it as when I think
"sync" I think it's synchronously syncing it to disk, not that it's scheduling
work to be done asynchronously on another thread.

> Source/WebCore/storage/StorageNamespace.h:55
> +    virtual void testForceSync() = 0;

As I mentioned on IRC, I don't think this is a very good name (here and
elsewhere).  Test is often used in contexts other than a function that should
only be called in a test.

> Source/WebCore/storage/StorageNamespaceImpl.cpp:163
> +	   RefPtr<StorageAreaImpl> storageArea = it->second;

Not sure it's really necessary to save this to a tmp variable...especially
since you don't right below

> Source/WebCore/storage/StorageTracker.cpp:48
> +void StorageTracker::initializeTracker(const String& storagePath)

Am I reading this wrong, or is this essentially making it so there's one
storage tracker (and associated path) for all page groups?  If so, that
definitely needs to be fixed.  Is there a reason this is a singleton rather
than hanging off a page group?

> Source/WebCore/storage/StorageTracker.cpp:369
> +    MutexLocker lockDatabase(m_databaseGuard);

Am I reading this wrong, or would this (and a few others) block the main thread
while this IO is happening (if the main thread tries to acquire the lock)?

> Source/WebCore/storage/StorageTracker.cpp:464
> +void StorageTracker::testStorageSync()

Why so many names for basically the same concept?

>> Source/WebKit/chromium/src/StorageNamespaceProxy.cpp:97
>> +	// FIXME: Implement.
> 
> Why a FIXME instead of notImplemented?

Actually, please just put an ASSERT_NOT_REACHED() in these. I believe with your
current code we can't hit it, but if we ever hit it it'd definitely be a logic
error.

> LayoutTests/ChangeLog:8
> +	   * storage/domstorage/localstorage/storagetracker: Added.

I'm glad you put these in a directory.	For all the platforms that don't
implement the layout test controller interface, you'll need to add them to the
skipped lists though, right?  If so, just add the whole directory to the list,
rather than individual tests.  And be sure to leave a comment about why they're
skipped.  (Note that Chromium uses platform/chromium/test_expectations.txt
rather than a skip list)