[webkit-reviews] review granted: [Bug 110600] webdatabase: Need a method for reliable multi-process quota management. : [Attachment 190770] the fix 2.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 110600: webdatabase: Need a method for reliable multi-process quota
management.
https://bugs.webkit.org/show_bug.cgi?id=110600

Attachment 190770: the fix 2.
https://bugs.webkit.org/attachment.cgi?id=190770&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=190770&action=review


Please resolve the design issues here before landing.

Let's do a follow-up patch to add an in-process Mutex to FileLock, so that it
provides exclusion between threads. (Otherwise, there's no real reason to have
a table of FileLocks.)

> Source/WTF/ChangeLog:24
> +	   * wtf/FileLockDarwin.cpp: Added.

Let's call this FileLockUnix.cpp, since the flock API is available on most
flavors of Unix and Linux. (See http://linux.die.net/man/2/flock.) This is our
convention for other classes like this.

> Source/WTF/wtf/FileLockDarwin.cpp:47
> +    m_lock.filename = filename;
> +    int length = filename.length();
> +
> +    m_lock.filenameCStr = new char[length + 1];
> +    strncpy(m_lock.filenameCStr, filename.latin1().data(), length);
> +    m_lock.filenameCStr[length] = '\0';

Let's not reinvent String=>CString conversion here. Instead, let's store just a
CString as a data member, and initialize it with filename.latin1().

> Source/WTF/wtf/FileLockDarwin.cpp:49
> +    m_lock.fd = open(m_lock.filenameCStr, O_RDWR | O_CREAT, S_IRUSR |
S_IWUSR);

It would be nice, in a follow-up patch, to hold the file open only as long as
it is supposed to be locked.

> Source/WTF/wtf/FileLockDarwin.cpp:58
> +    if (m_deleteFileFunc)
> +	   (*m_deleteFileFunc)(m_lock.filename.latin1().data());

Let's not use a deletion callback here. FileLock has its own fd for this file,
which it knows how to manage, regardless of what's happening in the
user-visible file system. Only our client knows when it's the best time to
remove the .lock file from the user-visible file system. That should happen
independently of releasing or deleting the file lock.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:679
> +    FileLock* lock = m_fileLockMap.take(origin);
> +    if (lock)
> +	   delete lock;

Please use OwnPtr instead of manual pointer management.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:681
> +    else
> +	   deleteFile(pathByAppendingComponent(originPath(origin),
String(".lock")));

This shouldn't be conditional.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:848
> +	   deleteFileLockFor(origin);

This function should remove the lock from the table and also unlink the .lock
file.