[webkit-reviews] review denied: [Bug 51878] Add WebKit1 API to view and delete local storage : [Attachment 81164] Added LocalStorageTracker to WebCore to track creation of LocalStorage dbs, and an API to list and delete them.

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied 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 81164: Added LocalStorageTracker to WebCore to track creation of
LocalStorage dbs, and an API to list and delete them.
https://bugs.webkit.org/attachment.cgi?id=81164&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81164&action=review

My biggest question is why we need a tracker database.	I'm fundamentally
against duplicating state since such duplicate state can get out of sync since
the number of possible error conditions quickly become very difficult to test
for and handle.  To be honest, I didn't read the tracker class in detail (yet)
so maybe it's obvious from the code, but if so then there probably should have
been something in the changeLog as well.  If there is a need, then I'd like to
see tests for the various error conditions go in with this patch.  For example,
what happens if there's an entry in the tracker, but the db doesn't exist.  Or
vice versa.  Etc.

> Source/WebCore/page/PageGroup.cpp:122
> +

not sure this newline is useful

> Source/WebCore/page/PageGroup.cpp:135
> +

not sure this newline is useful

> Source/WebCore/page/PageGroup.cpp:141
> +}

Put a newline after this.  (It's odd you added one before the #endif later in
the file, but didn't do one here. :-)

> Source/WebCore/storage/LocalStorageTask.h:51
> +	   static PassOwnPtr<LocalStorageTask> createOriginIdentifiersImport()
{ return adoptPtr(new LocalStorageTask(ImportOrigins)); }

This method of doing tasks was always clunky.  Let's not add any in the old
style and instead please use createCallbackTask
(http://codesearch.google.com/codesearch?q=createCallbackTask&exact_package=chr
omium&hl=en&vert=chromium).  It'd be really nice if you'd be willing to convert
the other ones while you're at it.

> Source/WebCore/storage/StorageAreaSync.cpp:142
> +void StorageAreaSync::scheduleCloseDatabase()

What's the difference between this and scheduleFinalSync?

> Source/WebCore/storage/StorageAreaSync.cpp:157
> +	   disableSuddenTermination();

I really hate these...	<thinking out loud>I wonder if we could make an object
that enables it on construction and disables on destruction and create/destroy
it instead of manually incrementing and decrementing</..>

> Source/WebCore/storage/StorageNamespaceImpl.cpp:155
> +    // a sync and for StorageAreaSync to recreate the backing db file.

These semantics are not at all clear from the name.  Ideally, the name would
better reflect this.  But at very least this comment should be in the header.

In what case are you clearing origins?	If it's because the user asked you to,
I'm not sure this is the right policy.	Allowing live pages to resurrect all
their data simply by touching a single piece of data seems a bit odd.

> Source/WebCore/storage/StorageTracker.cpp:3
> + *

use http://webkit.org/coding/bsd-license.html for new files (i.e. 2 clause)

> Source/WebCore/storage/StorageTracker.cpp:148
> +    SQLiteStatement statement(m_database, "SELECT origin FROM Origins");

Why do we need a table to track stuff?	Unless there's a _really_ good reason
or we're unifying all the quotas together, I don't think we should ever add
this.

> Source/WebCore/storage/StorageTrackerClient.h:40
> +	   virtual void dispatchDidModifyOrigin(const String& originIdentifier)
= 0;

Not sure the word dispatch adds information...is there precedence for it?