Bug 115660 - Write storage changes to disk
Summary: Write storage changes to disk
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anders Carlsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-06 10:45 PDT by Anders Carlsson
Modified: 2013-05-06 11:09 PDT (History)
0 users

See Also:


Attachments
Patch (10.51 KB, patch)
2013-05-06 10:47 PDT, Anders Carlsson
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2013-05-06 10:45:50 PDT
Write storage changes to disk
Comment 1 Anders Carlsson 2013-05-06 10:47:59 PDT
Created attachment 200721 [details]
Patch
Comment 2 Andreas Kling 2013-05-06 11:02:24 PDT
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?
Comment 3 Anders Carlsson 2013-05-06 11:07:54 PDT
(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).
Comment 4 Anders Carlsson 2013-05-06 11:09:20 PDT
Committed r149615: <http://trac.webkit.org/changeset/149615>