[webkit-reviews] review granted: [Bug 134254] Don't allow for sudden termination while writing to local storage. : [Attachment 233742] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Roger Fong <roger_fong at apple.com>'s
request for review:
Bug 134254: Don't allow for sudden termination while writing to local storage.
https://bugs.webkit.org/show_bug.cgi?id=134254

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=233742&action=review


> Source/WebKit2/ChangeLog:3
> +	   Don't allow for sudden termination while writing to local storage.

“Don’t allow sudden termination”

“Don’t allow for” means something different.

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:36
> +#include <WebCore/SuddenTermination.h>

Don’t need to add this both to the header and the cpp file. If you are unable
to remove it from the header (see comment below), please remove it from here.

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:269
> +	   if (m_disableSuddenTerminationWhileWritingToLocalStorage)
> +	       m_disableSuddenTerminationWhileWritingToLocalStorage.reset();

Normally we just assign nullptr instead of using the reset function. Also, it’s
overkill to add the if statement. So these two lines should be replaced with
just this:

    m_disableSuddenTerminationWhileWritingToLocalStorage = nullptr;

Is there something that guarantees the database is flushed out to disk? How did
you test?

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.h:30
> +#include <WebCore/SuddenTermination.h>

Do we really need this include? Can we just do a forward declaration instead?