[webkit-reviews] review denied: [Bug 186104] Stop using StorageTracker.db in LocalStroageDatabaseTracker : [Attachment 341588] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 186104: Stop using StorageTracker.db in LocalStroageDatabaseTracker
https://bugs.webkit.org/show_bug.cgi?id=186104

Attachment 341588: Patch

https://bugs.webkit.org/attachment.cgi?id=341588&action=review




--- Comment #2 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 341588
  --> https://bugs.webkit.org/attachment.cgi?id=341588
Patch

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

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));

Shouldn't we do this in the dispatch call above so that we do I/O on the
background thread instead of the main thread?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:87
> +    if (path.isEmpty())

if (!path.isEmpty())
    SQLiteFileSystem::deleteDatabaseFile(path);

since we'd want to notify the clients even if the path was empty I would 
assume?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:98
> +    Vector<String> paths =
FileSystem::listDirectory(m_localStorageDirectory, "*.localstorage");

auto paths

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:111
> +    importOrigins();

Looks like this unconditionally re-imports all origins from disk, even though
they may have already been imported. This seems bad.

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:131
> +	   origins.append(origin);

This should have been uncheckedAppend() since you reserveInitialCapacity()
beforehand. However, in this case, we should rely on pre-existing
copyToVector() instead of duplicating the logic for copying a container into a
vector.

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:171
> +    m_origins.clear();

What if origins were already imported?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:174
>	   if (!path.endsWith(".localstorage"))

Why is this needed? Maybe this could be an assertion?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.h:60
> +	   double creationTime;

We try to avoid storing timestamps as double nowadays. This and the one below
should probably be WallTime instead of double.