[webkit-reviews] review denied: [Bug 73654] Upstream BlackBerry Cookie Management Classes : [Attachment 119643] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied ocheung at rim.com's request for review:
Bug 73654: Upstream BlackBerry Cookie Management Classes
https://bugs.webkit.org/show_bug.cgi?id=73654

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

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=119643&action=review


Still some work to do

>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:39
> +

Why extra line?

>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:178
> +    String dropBackupQuery("DROP TABLE IF EXISTS Backup_"+m_tableName+";");

String dropBackupQuery("DROP TABLE IF EXISTS Backup_"+m_tableName+";");
should be
String dropBackupQuery("DROP TABLE IF EXISTS Backup_" + m_tableName + ";");
to be consistent with other code. You are doing this in other places as well.

>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:231
> +    // Create table if not exsist in case that the alterTableIfNeeded()
failed accidentially.

Typo -> accidentially

>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:353
> +}

Looks like you can consolidate these three methods into one, by adding one
param for Insert/Update/Delete.

>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:510
> +	   } else {

Why extra line?

>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:520
> +

Why extra line?

> Source/WebCore/platform/blackberry/CookieJarBlackBerry.cpp:47
> +WTF::String cookies(Document const* document, KURL const& url)

Is WTF prefix needed?

> Source/WebCore/platform/blackberry/CookieManager.cpp:184
> +

Why extra line?

> Source/WebCore/platform/blackberry/CookieManager.cpp:195
> +

Why extra line?