[webkit-reviews] review granted: [Bug 49332] Do not allow access to existing HTML5 databases in private browsing mode : [Attachment 76135] [PATCH] separate read-only transaction state from private browsing no read-write state (fixed ChangeLog conflict)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Anton D'Auria <adauria at apple.com>'s
request for review:
Bug 49332: Do not allow access to existing HTML5 databases in private browsing
mode
https://bugs.webkit.org/show_bug.cgi?id=49332

Attachment 76135: [PATCH] separate read-only transaction state from private
browsing no read-write state (fixed ChangeLog conflict)
https://bugs.webkit.org/attachment.cgi?id=76135&action=review

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

r=me, but the enum/bit combo is a bit ugly, the bitifield use here is sloppy
and there is some incorrectly formatted code

> WebCore/storage/DatabaseAuthorizer.h:97
> +    void setPermissions(int permissions);

It doesn’t seem right to use int for permissions, but use an enum for the
values. I think we normally have a better idiom for this sort of thing.

> WebCore/storage/DatabaseAuthorizer.h:114
> +    int m_permissions;
>      bool m_securityEnabled : 1;

All these bools are set to be a single bit to save memory, I suppose, and then
the three bit permissions is taking up an entire 32-bit int! I think the use of
bitfields on the bools is excessive. You could use a bitfield for permissions,
but because we are using a signed type, int, we’d need to use four bits. It
would be better to use unsigned and three bits. Or not use bitfields at all in
this class.

> WebCore/storage/SQLTransactionSync.cpp:94
> +    if (!m_database->scriptExecutionContext()->allowDatabaseAccess())
> +	 permissions |= DatabaseAuthorizer::NoAccessMask;
> +    else if (m_readOnly)
> +	 permissions |= DatabaseAuthorizer::ReadOnlyMask;

This is not indented correctly.