Bug 115660 – Write storage changes to disk

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

Write storage changes to disk

[Anders Carlsson]

Created attachment 200721 [details]
Patch

[Andreas Kling]

Comment on attachment 200721 [details]
Patch

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?

[Anders Carlsson]

(In reply to comment #2)
> 
> > Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:248
> > +    for (HashMap<String, String>::const_iterator it = changedItems.begin(), end = changedItems.end(); it != end; ++it) {
> 
> auto?

Yes.

> 
> > 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?

The various bind functions will only fail if the column index is out of range or if malloc fails - I don’t think we need to care about those conditions.

> 
> > Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:264
> > +        statement.reset();
> 
> Missing error handling. Is this intentional?

Reset will only return an error if the previous step returned an error.

> 
> > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:160
> > +    if (m_localStorageDatabase)
> > +        m_localStorageDatabase->setItem(key, value);
> 
> Should we still be dispatching events if !m_localStorageDatabase?

Yup. (For session storage).

[Anders Carlsson]

Committed r149615: <http://trac.webkit.org/changeset/149615>