[webkit-reviews] review granted: [Bug 115660] Write storage changes to disk : [Attachment 200721] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 115660: Write storage changes to disk
https://bugs.webkit.org/show_bug.cgi?id=115660

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

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=200721&action=review


r=me

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:40
> +static const double databaseUpdateInterval = 1.0;

I'd call this 'databaseUpdateIntervalInSeconds'

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:217
> +	   for (size_t i = 0; i < maximumItemsToUpdate; ++i) {

'i' should be 'int' here to match maximumItemsToUpdate and HashMap::size().

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:248
> +    for (HashMap<String, String>::const_iterator it = changedItems.begin(),
end = changedItems.end(); it != end; ++it) {

auto?

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:252
> +	   statement.bindText(1, it->key);

Missing error handling. Is this intentional?

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:256
> +	       statement.bindBlob(2, it->value);

Missing error handling. Is this intentional?

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:264
> +	   statement.reset();

Missing error handling. Is this intentional?

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.h:78
> +    HashMap<String, String> m_changedItems;
> +    bool m_didScheduleDatabaseUpdate;

The order of these members makes me twitch.

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:160
> +    if (m_localStorageDatabase)
> +	   m_localStorageDatabase->setItem(key, value);

Should we still be dispatching events if !m_localStorageDatabase?