[webkit-reviews] review denied: [Bug 45416] Allow Chromium port to link with system-provided SQLite : [Attachment 68080] 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 68080: patch
https://bugs.webkit.org/attachment.cgi?id=68080&action=review

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

looks pretty good. a few more comments. and please check that all layout tests
in LayoutTests/storage pass in chromium on linux and mac.

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:749
> +static int syncWrapper(int fd, bool fullSync)

change the return type of this function to 'bool', and rename 'rc' to 'synced',
or something like that.

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:757
> +#if PLATFORM(MAC)
> +    if (fullSync)
> +	   rc = fcntl(fd, F_FULLSYNC, 0);
> +#endif
> +    if (rc)
> +	   rc = fsync(fd);

i don't think this code mirrors what's in full_fsync(). i think it should be
more like:

   int rc = 1;
#if PLATFORM(MAC)
    if (fullSync)
	rc = fcntl(fd, F_FULLSYNC, 0);
    if (rc)
	rc = fsync(fd);
#else
    rc = fdatasync(fd);
#endif
    return rc;

(with the appropriate changes to make it return a boolean)

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:769
> +    if (syncWrapper(pFile->h, (flags & 0x0F) == SQLITE_SYNC_FULL)) {

let's add a isFullSync = flags & 0x0F == SQLITE_SYNC_FULL variable, since we
need it a few lines below too.

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1080
> +// We disallow loading DSOs inside the renderer process, so the following
> +// procedures are no-op.

1 line.

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1085
> +static void chromiumDlError(sqlite3_vfs*, int, char*)

empty lines between these functions.

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1153
> +static const int ChromiumMaxPathname = 512;

chromiumMaxPathname