[webkit-reviews] review denied: [Bug 45416] Allow Chromium port to link with system-provided SQLite : [Attachment 66952] patch

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has denied Paweł Hajdan, Jr.
<phajdan.jr at chromium.org>'s request for review:
Bug 45416: Allow Chromium port to link with system-provided SQLite
https://bugs.webkit.org/show_bug.cgi?id=45416

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

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66952&action=prettypatch

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:46
>  // Chromium's Posix implementation of SQLite VFS
we should probably mention here that this code was mostly copy-pasted from
os_unix.c.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:47
>  namespace {
webkit prefers static functions/structs over anonymous namespaces.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:120
> +    
no need for empty line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:127
> +    
no need for empty line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:131
> +	       || (sqliteIOErr == SQLITE_IOERR_UNLOCK)
keep || at the end of lines. i know the webkit style guide says to put them at
the beginning of the line, but there's a lot of code that violates this rule,
and i've never seen it enforced.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:133
> +	       || (sqliteIOErr == SQLITE_IOERR_CHECKRESERVEDLOCK)) {
no need for {}.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:139
> +    
no need for empty line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:142
> +    
no need for empty line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:205
> +    struct stat statbuf;
don't think you need 'struct' here (but i might be wrong).

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:316
> +	   } else if (lock.l_type != F_UNLCK) {
no need for {}.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:330
> +    struct flock lock;
don't think we need 'struct'.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:334
> +    lock.l_len = SQLiteSharedSize;
why is there no case for (pFile->fileFlags & SQLITE_WHOLE_FILE_LOCKING != 0)?

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:409
> +	   && (pLock->locktype >= SQLITE_LOCK_PENDING || locktype >
SQLITE_LOCK_SHARED)) {
move && to the end of the previous line. also, no need for {}.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:416
> +	   && (pLock->locktype == SQLITE_LOCK_SHARED || pLock->locktype ==
SQLITE_LOCK_RESERVED)) {
&& on previous line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:431
> +	   || (locktype == SQLITE_LOCK_EXCLUSIVE && pFile->locktype <
SQLITE_LOCK_PENDING)) {
|| on previous line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:527
> +	   } else {
no need for {}.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:543
> +
remove extra line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:559
> +	       if (rangeLock(pFile, F_RDLCK, &tErrno) == -1) {
combine these 2 if-blocks into a single one.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:623
> +static int closeChromiumFile(sqlite3_file* id)
change name to chromiumCloseNoLock(), or something similar.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:636
> +	   if (close(pFile->h)) {
combine these 2 if-blocks into a single one.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:768
> +    if (fsync(pFile->h)) {
why did full_fsync() get replaced by fsync()?

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:812
> +#if PLATFORM(MAC)
this code should not be #ifdef'd. it's not specific to Mac OS in os_unix.c.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:842
> +    return 512;
define a constant for this right on top of this function. something like
SQliteDefaultSectorSize should do. add a comment that it's the same as
SQLITE_DEFAULT_SECTOR_SIZE, which is defined in os.h.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:928
> +    } else {
no need for {}.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:940
> +    struct stat sStat;
probably don't need 'struct' here.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:973
> +    ChromiumFile* unixSQLite3File = reinterpret_cast<ChromiumFile*>(id);
s/unixSQLite3File/chromiumFile/g.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1009
> +    int result = fillInChromiumFile(vfs, fd, -1, id, fileName, noLock);
s/result/rc/, for consistency.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:-125
> -	   *res = 1;   // if the file doesn't exist, attr < 0
leave this comment here.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1090
>      sqlite3_vfs* unix_vfs = sqlite3_vfs_find("unix");
we don't need the "unix" VFS anymore; delete this line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1094
>	   unix_vfs->mxPathname,
define a constant PosixMaxPathName = 512, and use that here. add a comment that
it corresponds to MAX_PATHNAME defined in os_unix.c.