[webkit-reviews] review denied: [Bug 73654] Upstream BlackBerry Cookie Management Classes : [Attachment 119670] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Dec 20 13:01:38 PST 2011
Daniel Bates <dbates at webkit.org> 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 119670: Patch
https://bugs.webkit.org/attachment.cgi?id=119670&action=review
------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119670&action=review
> Source/WebCore/ChangeLog:4
> + Upstream BlackBerry Cookie Management Classes
> + https://bugs.webkit.org/show_bug.cgi?id=73654
This change log message is very long due to the number of functions/methods we
have in these added files. I suggest either truncating the list of
functions/methods or removing them from the changelog entry such that we only
list the added files.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:36
> +#include <wtf/text/CString.h>
This include is unnecessary as its included by SQLiteDatabase.h, which is
included by SQLiteStatement.h (above).
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:45
> +#endif // ENABLE_COOKIE_DEBUG
Nit: The inline comment is unnecessary here since this #if/#else block is short
enough to fit inside a reasonably-sized editor window.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:53
> +static double const s_databaseTimerInterval = 2;
Nit: We tend to put the const specifier before the static type of the variable.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:115
> + String query("PRAGMA table_info(" + m_tableName + ");");
It's unnecessary to invoke the copy constructor for WTF::String here. I would
write this as:
String query = "PRAGMA table_info(" + m_tableName + ");"
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:126
> + if (name == "creationTime")
We should use DEFINE_STATIC_LOCAL to define a WTF::String object for
"creationTime" with static storage duration so that we don't have to construct
a new WTF::String on each iteration.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:128
> + if (name == "protocol")
Ditto.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:136
> + // Drop and Recreate the cookie table to update to the newest database
fields
Nit:
Recreate => recreate
newest => latest
Missing a period at the end of this sentence.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:137
> + // We do not use alter table - add column because that method cannot add
primary keys
Missing a period at the end of this sentence.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:141
> + String renameQuery("ALTER TABLE " + m_tableName + " RENAME TO Backup_" +
m_tableName + ";");
It's unnecessary to invoke the copy constructor for WTF::String here. I would
write this as:
String renameQuery = "ALTER TABLE " + m_tableName + " RENAME TO Backup_" +
m_tableName + ";";
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:160
> + String migrationQuery("INSERT OR REPLACE INTO ");
> + migrationQuery += m_tableName;
> + migrationQuery += " SELECT *";
> + if (!creationTimeExists)
> + migrationQuery += ",''";
> + if (!protocolExists)
> + migrationQuery += ",''";
> + migrationQuery += " FROM Backup_" + m_tableName;
We should use StringBuilder to build up this query since it calls malloc() less
than WTF::String's operator +=.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:165
> + String setCreationTimeQuery("UPDATE " + m_tableName + " SET
creationTime = lastAccessed;");
It's unnecessary to invoke the copy constructor for WTF::String here. I would
write this as:
String setCreationTimeQuery = "UPDATE " + m_tableName + " SET creationTime =
lastAccessed;";
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:171
> + String setProtocolQuery("UPDATE " + m_tableName + " SET protocol =
'http' WHERE isSecure = '0';");
> + String setProtocolQuery2("UPDATE " + m_tableName + " SET protocol =
'https' WHERE isSecure = '1';");
It's unnecessary to invoke the copy constructor for WTF::String here with the
string concatenation result.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:177
> + String dropBackupQuery("DROP TABLE IF EXISTS Backup_" + m_tableName +
";");
Ditto.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:182
> + for (size_t i = 0; i < commands.size(); ++i) {
Although the compiler will inline the result of commands.size(), for your
consideration I suggest conforming to the WebKit Code Style Guidelines and
explicitly defining a local variable for commands.size(). See (2) of section
Other Punctuation on <http://www.webkit.org/coding/coding-style.html>.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:191
> + // finally it will try to keep a backup of the current cookie
table for the future restoration.
> + // NOTICE: this will essentially DELETE the current table, but
that's better
> + // than continually hitting this case and never being able to
use the cookie table.
> + // if this is ever hit, it's definitely a bug.
This comment is disingenuous. The word "backup" suggests that we are making a
copy of the cookie table, but this disagrees with both the remark in the NOTICE
as well as the actual code. Instead, we are renaming the cookie table so as to
preserve it for some future restoration. This has the side effect of clearing
the current cookie table. How does this error handling work should we get to
here twice? I mean, it looks like we overwrite the backup table.
For your consideration, I suggest making this comment concise. Maybe something
like:
// We should never get here, but if we do, rename the current cookie table for
future restoration. This has the side effect of
// clearing the current cookie table, but that's better than continually
hitting this case and hence never being able to use the
// cookie table.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:193
> + String renameQuery("ALTER TABLE " + m_tableName + " RENAME TO
Backup2_" + m_tableName + ";");
It's unnecessary to invoke the copy constructor for WTF::String here with the
string concatenation result.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:226
> + const String primaryKeyFields("PRIMARY KEY (protocol, host, path,
name)");
> + const String databaseFields("name TEXT, value TEXT, host TEXT, path
TEXT, expiry DOUBLE, lastAccessed DOUBLE, isSecure INTEGER, isHttpOnly INTEGER,
creationTime DOUBLE, protocol TEXT");
We should use DEFINE_STATIC_LOCAL to make these static so that we don't
repeatedly construct these String objects.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:227
> + // Upgrade table to add the new column creationTime and protocol for
backward compliance.
This sentence doesn't read well. Maybe "backward compliance" should be
"backwards compatibility"?
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:230
> + // Create table if not exsist in case that the alterTableIfNeeded()
failed accidentally.
I can't find the function alterTableIfNeeded(). I take it you mean method
CookieDatabaseBackingStore::upgradeTableIfNeeded()?
Suppose CookieDatabaseBackingStore::upgradeTableIfNeeded() fails to create the
table m_tableName. I'm a bit unclear why we make a second attempt here to
create the table m_tableName without changing any additional state after the
first failure. I take it that there may be some kind of environment change
between attempts such that the second attempt may succeed? Assuming it makes
sense to make a second attempt to create the table, I suggest extracting the
SQL conditional of whether the table exists (i.e. "IF NOT EXIST") out of the
SQL expression and into a C++ if-condition so that we only take time and memory
to build up an SQL query and execute it when the table actually doesn't exist.
Currently, we take time and memory to build an SQL query and execute it
regardless of whether the table already exists. Towards implement this, I
suggest having CookieDatabaseBackingStore::upgradeTableIfNeeded() some how
signal its success or failure, say by returning a boolean result. Assuming a
boolean result of false means it failed to create the table then we can make a
second attempt to create the table by building the SQL query and executing it.
A side benefit of having CookieDatabaseBackingStore::upgradeTableIfNeeded()
return whether it succeeded or failed is that this comment can be replaced with
an if-statement that checks for the failure condition.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:234
> + String createTableQuery("CREATE TABLE IF NOT EXISTS ");
> + createTableQuery += m_tableName;
> + // This table schema is compliant with Mozilla's.
> + createTableQuery += " (" + databaseFields + ", " +
primaryKeyFields+");";
This code is almost identical to code in
CookieDatabaseBackingStore::upgradeTableIfNeeded(). We should extract out this
common code into a shared function.
>
Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBac
kingStore.cpp:245
> + String insertQuery("INSERT OR REPLACE INTO ");
> + insertQuery += m_tableName;
> + insertQuery += " (name, value, host, path, expiry, lastAccessed,
isSecure, isHttpOnly, creationTime, protocol) VALUES (?1, ?2, ?3, ?4, ?5, ?6,
?7, ?8, ?9, ?10);";
The string ""INSERT OR REPLACE INTO " appears in both this function and
CookieDatabaseBackingStore::upgradeTableIfNeeded() we should make this a file
static variable so that we can share this string between these functions.
Depending on how hot this code path is, we may want to consider making the
string " (name, value, host, path, expiry, lastAccessed, isSecure, isHttpOnly,
creationTime, protocol) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10);"
static as well.
Moreover, we should use StringBuilder here.
> Source/WebCore/platform/blackberry/CookieManager.cpp:74
> + static CookieManager *cookieManager = 0;
> + if (!cookieManager) {
> + // Open the cookieJar now and get the backing store cookies to fill
the manager.
> + cookieManager = new CookieManager;
> +
cookieManager->m_cookieBackingStore->open(cookieManager->cookieJar());
> + cookieManager->getBackingStoreCookies();
> + CookieLog("CookieManager - Backingstore load complete.\n");
> + }
> + return *cookieManager;
We should use DEFINE_STATIC_LOCAL to declare the static instance of
CookieManager and to simplify the code in this function.
> Source/WebCore/platform/blackberry/CookieManager.h:46
> +enum BackingStoreRemoval {
Maybe a more descriptive name for this enum is BackingStoreRemovalPolicy.
> Source/WebCore/platform/blackberry/CookieManager.h:52
> +enum HttpOnlyCookieFiltering {
Http => HTTP
Can we come up with a more descriptive name for this enum?
> Source/WebCore/platform/blackberry/CookieManager.h:75
> +/*
> + * The CookieManager class is a singleton class that handles and
selectively persists
> + * incoming cookies. This class contains a tree of domains for quicker
> + * cookie domain lookup. The top of the tree represents a null value for a
null domain.
> + * The null domain contains references to top level domains and each node
below
> + * represents a sub-section of a domain, delimited by "."
> + *
> + * If a cookie has a domain "a.b.com", it will be stored in the node named
"a" in this tree.
> + * in the branch ""->"com"->"b"->"a"
> + *
> + * Cookie specs follow the RFC6265 spec sheet.
> + * http://tools.ietf.org/html/rfc6265
> + */
RFC6265 => RFC 6265
Please use C++-style comments.
> Source/WebCore/platform/blackberry/CookieManager.h:102
> + static unsigned int maxCookieLength() { return s_maxCookieLength; }
Nit: unsigned int => unsigned
> Source/WebCore/platform/blackberry/CookieManager.h:119
> + void checkAndTreatCookie(ParsedCookie* candidateCookie,
BackingStoreRemoval postToBackingStore);
Neither the parameter names candidateCookie nor BackingStoreRemoval add much
value. Please remove them.
> Source/WebCore/platform/blackberry/CookieManager.h:123
> + void addCookieToMap(CookieMap* targetMap, ParsedCookie* candidateCookie,
BackingStoreRemoval postToBackingStore);
None of the parameter names in this declaration are very descriptive. Please
remove all of them.
> Source/WebCore/platform/blackberry/CookieManager.h:124
> + void update(CookieMap* targetMap, ParsedCookie* newCookie,
BackingStoreRemoval postToBackingStore);
Ditto.
> Source/WebCore/platform/blackberry/CookieManager.h:133
> + // Count all cookies, cookies are limited by max_count
Where is max_count defined? Do you mean s_maxCookieCountPerHost?
Moreover, this comment seems unnecessary. I suggest making the name of this
variable more descriptive instead of having this comment. Maybe rename m_count
to m_numberOfCookies?
> Source/WebCore/platform/blackberry/CookieManager.h:148
> + static const unsigned int s_globalMaxCookieCount = 6000;
> + static const unsigned int s_maxCookieCountPerHost = 60;
> + static const unsigned int s_cookiesToDeleteWhenLimitReached = 60;
> + static const unsigned int s_delayToStartCookieCleanup = 10;
Nit: unsigned int => unsigned
> Source/WebCore/platform/blackberry/CookieManager.h:149
> + // Cookie size limit of 4kB as advised per RFC2109
Nit: I suggest adding an empty line above this comment to make it stand out
more from the list of static const declarations above it.
This comment is missing a period. As per the WebKit Code Style guidelines, we
prefer comments that are full sentences.
I also suggest referencing the section of this RFC that mentions this limit.
>From briefly looking, it seems that this limit is covered in section 6.3
Implementation Limits. You may also want to consider referencing a hyperlink to
the RFC in your comment: <http://www.ietf.org/rfc/rfc2109.txt>.
RFC2109 => RFC 2109
> Source/WebCore/platform/blackberry/CookieManager.h:150
> + static const unsigned int s_maxCookieLength = 4096;
Nit: unsigned int => unsigned
> Source/WebCore/platform/blackberry/CookieParser.cpp:56
> +Vector<ParsedCookie*> CookieParser::parse(const String& cookies)
This function should be returning a Vector of OwnPtr<ParsedCookie> objects so
that we don't have to explicitly handle the deletion the Cookie objects when
removing entries from the Vector.
> Source/WebCore/platform/blackberry/CookieParser.cpp:93
> + ParsedCookie* res = new ParsedCookie(curTime);
We should make this an OwnPtr and have this function return a PassOwnPtr. Then
we can remove the "delete res" calls in all the early return code paths. At the
end of function, we should call OwnPtr::release() to transfer ownership of the
ParsedCookie object to the caller.
More information about the webkit-reviews
mailing list