Bug 73654

Created attachment 119643 [details] Patch

Comment on attachment 119643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119643&action=review Still some work to do > Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBackingStore.cpp:39 > + Why extra line? > Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.cpp:231 > + // Create table if not exsist in case that the alterTableIfNeeded() failed accidentially. Typo -> accidentially > Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.cpp:510 > + } else { Why extra line? > Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBackingStore.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?

Created attachment 119670 [details] Patch

Comment on attachment 119670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119670&action=review > Source/WebCore/platform/blackberry/ParsedCookie.cpp:164 > + cookie += protocol(); I didn't catch this earlier, but everywhere we append heavily like here we should use StringBuilder, will give far less mallocs.

Comment on attachment 119670 [details] Patch 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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.cpp:128 > + if (name == "protocol") Ditto. > Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.cpp:177 > + String dropBackupQuery("DROP TABLE IF EXISTS Backup_" + m_tableName + ";"); Ditto. > Source/WebCore/platform/blackberry/CookieDatabaseBackingStore/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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/CookieDatabaseBackingStore.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.

I'll update Otto's patches and take over the upstreaming of this set of files.

Created attachment 128254 [details] Patch

Dan in response to your comments on Otto's patch I've posted a follow up. It addresses most of your concerns, with a few exceptions. 1) I didn't refactor the code to have upgradeTableIfNeeded to return a bool. After examining the code there is a need to have this method behave the way it is implemented. (I'm trying to recall my exact reasoning but forget since I did this while ago, I'll post a new comment about why when I remember). 2) StringBuilder is not needed in CookieDatabaseBackingStore.cpp because that code is ran once when a database is loaded into memory. There are too few lines of code that use += that would benefit from this in any significant way. 3) To maintain functionality and as initial upstream of this code I'm not implementing OwnPtr at this time. I'll make a separate bug for this and address it in a follow up patch. It is logically separate and I'd prefer to have a separate patch that can be easily reverted if necessary.

Alexis, didn't you also write an sqlite based cookie storage and land that somewhere in WebCore? Should these be combined somehow?

(In reply to comment #9) > Alexis, didn't you also write an sqlite based cookie storage and land that somewhere in WebCore? Should these be combined somehow? Where does this code live, I'd be interested in taking a look at it.

Comment on attachment 128254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128254&action=review Looks good, but still some stuff to fix. > Source/WebCore/ChangeLog:9 > + Passes all non Cookie 2 tests since Cookie 2 is not implemented supported at this time. not implemented supported? > Source/WebCore/platform/blackberry/CookieJarBlackBerry.cpp:49 > + if (!static_cast<WebCore::FrameLoaderClientBlackBerry*>(frame->loader()->client())->cookiesEnabled()) No need for WebCore:: > Source/WebCore/platform/blackberry/CookieJarBlackBerry.cpp:70 > + if (!static_cast<WebCore::FrameLoaderClientBlackBerry*>(frame->loader()->client())->cookiesEnabled()) No need for WebCore:: > Source/WebCore/platform/blackberry/CookieJarBlackBerry.cpp:106 > + if (!static_cast<WebCore::FrameLoaderClientBlackBerry*>(document->frame()->loader()->client())->cookiesEnabled()) No need for WebCore:: > Source/WebCore/platform/blackberry/CookieJarBlackBerry.cpp:107 > + return WTF::String(); You can remove all WTF:: prefixes in this file. > Source/WebCore/platform/blackberry/CookieManager.h:59 > + CookieStorageAcceptPolicyAlways = 0, No need to assign to zero. > Source/WebCore/platform/blackberry/CookieManager.h:152 > + static const unsigned s_maxCookieLength = 4096; No need to be in the header, what about a static in the .cpp? > Source/WebCore/platform/blackberry/CookieMap.h:59 > + // Returning the original cookie object so manager can keep a reference to the updates in the database queue Lacks some periods. > Source/WebCore/platform/blackberry/CookieMap.h:62 > + // Need to return the reference to the removed cookie so manager can deal with it (garbage collect) Ditto. > Source/WebCore/platform/blackberry/CookieMap.h:66 > + // Returns a map with that given subdomain Ditto. > Source/WebCore/platform/blackberry/CookieMap.h:84 > + // The "google" cookiemap will contain "accounts" in its subdomain map Ditto. > Source/WebCore/platform/blackberry/CookieParser.cpp:58 > + unsigned cookieStart, cookieEnd = 0; cookieEnd may be the wrong name, what about cookieCount? > Source/WebCore/platform/blackberry/CookieParser.cpp:66 > + while (cookieEnd <= cookies.length()) { It is probably worth it to assign cookies.length() to a temp var, since it is used a lot below. > Source/WebCore/platform/blackberry/ParsedCookie.cpp:122 > + int value = maxAge.toIntStrict(&ok, 10); The default is 10, don't need to specify it. > Source/WebCore/platform/blackberry/ParsedCookie.cpp:147 > + // Session cookies do not expires, they will just not be saved to the backing store. expires -> expire > Source/WebCore/platform/blackberry/ParsedCookie.h:42 > +class ParsedCookie { Would this be more efficient if we use WTF_MAKE_FAST_ALLOCATED? Would be interesting to test. > Source/WebCore/platform/blackberry/ParsedCookie.h:48 > + ParsedCookie(const String& /*name*/, const String& /*value*/, const String& /*domain*/,const String& /*protocol*/, const String& /*path*/, double /*expiry*/, double /*lastAccessed*/, double /*creationTime*/, bool /*isSecure*/, bool /*isHttpOnly*/); Why the use of the comments? I see no need. > Source/WebCore/platform/blackberry/ParsedCookie.h:107 > + bool m_isMaxAgeSet; Moving this to the other bools will make ParsedCookie objects smaller.

Comment on attachment 128254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128254&action=review >> Source/WebCore/ChangeLog:9 >> + Passes all non Cookie 2 tests since Cookie 2 is not implemented supported at this time. > > not implemented supported? will change to not implemented/supported >> Source/WebCore/platform/blackberry/CookieJarBlackBerry.cpp:49 >> + if (!static_cast<WebCore::FrameLoaderClientBlackBerry*>(frame->loader()->client())->cookiesEnabled()) > > No need for WebCore:: will remove all references to WebCore:: and WTF:: >> Source/WebCore/platform/blackberry/CookieManager.h:59 >> + CookieStorageAcceptPolicyAlways = 0, > > No need to assign to zero. ok >> Source/WebCore/platform/blackberry/CookieManager.h:152 >> + static const unsigned s_maxCookieLength = 4096; > > No need to be in the header, what about a static in the .cpp? If this can be moved maybe all the static constants can be moved? >> Source/WebCore/platform/blackberry/CookieMap.h:59 >> + // Returning the original cookie object so manager can keep a reference to the updates in the database queue > > Lacks some periods. ok >> Source/WebCore/platform/blackberry/CookieParser.cpp:58 >> + unsigned cookieStart, cookieEnd = 0; > > cookieEnd may be the wrong name, what about cookieCount? this isn't a count but a pointer to the end of the cookie substring. >> Source/WebCore/platform/blackberry/CookieParser.cpp:66 >> + while (cookieEnd <= cookies.length()) { > > It is probably worth it to assign cookies.length() to a temp var, since it is used a lot below. ok >> Source/WebCore/platform/blackberry/ParsedCookie.h:48 >> + ParsedCookie(const String& /*name*/, const String& /*value*/, const String& /*domain*/,const String& /*protocol*/, const String& /*path*/, double /*expiry*/, double /*lastAccessed*/, double /*creationTime*/, bool /*isSecure*/, bool /*isHttpOnly*/); > > Why the use of the comments? I see no need. I'll add the full signature with variable names instead of comments. >> Source/WebCore/platform/blackberry/ParsedCookie.h:107 >> + bool m_isMaxAgeSet; > > Moving this to the other bools will make ParsedCookie objects smaller. will do

(In reply to comment #10) > (In reply to comment #9) > > Alexis, didn't you also write an sqlite based cookie storage and land that somewhere in WebCore? Should these be combined somehow? > > Where does this code live, I'd be interested in taking a look at it. http://trac.webkit.org/browser/trunk/Source/WebCore/platform/qt/CookieJarQt.cpp It seems much simplier than what you guys did.

Created attachment 128303 [details] Patch

(In reply to comment #13) > (In reply to comment #10) > > (In reply to comment #9) > > > Alexis, didn't you also write an sqlite based cookie storage and land that somewhere in WebCore? Should these be combined somehow? > > > > Where does this code live, I'd be interested in taking a look at it. > > http://trac.webkit.org/browser/trunk/Source/WebCore/platform/qt/CookieJarQt.cpp > > It seems much simplier than what you guys did. This makes use of a lot of Qt classes for its backend impl which we can't make use of. Our implementation is a little more complicated because of platform specific issues. It would be nice to have a common SQLite implementation though and if you're willing to abstract away some of your Qt dependencies then we can give it a shot. If you're up for the challenge then please file a bug and cc me.

Comment on attachment 128303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128303&action=review Looks good, can be cleaned up a bit more though. > Source/WebCore/platform/blackberry/CookieJarBlackBerry.cpp:54 > + String result = cookieManager().getCookie(url, NoHttpOnlyCookie); Seems no need for temp var. > Source/WebCore/platform/blackberry/CookieManager.cpp:39 > +#include "ParsedCookie.h" Already included in header, please do checks for similar cases. > Source/WebCore/platform/blackberry/CookieManager.cpp:108 > + // TODO: m_managerMap and the top layer protocolMaps are not properly deleted. TODO should be FIXME. > Source/WebCore/platform/blackberry/CookieManager.cpp:134 > + return false; Could do just return (protocol == "file" || protocol == "local" ); > Source/WebCore/platform/blackberry/CookieManager.cpp:420 > + initiateCookieLimitCleanUp(); Indenting. > Source/WebCore/platform/blackberry/CookieManager.cpp:453 > + bool oldIsSession = oldCookie->isSession(); These can go into the if. > Source/WebCore/platform/blackberry/CookieManager.h:44 > +class ParsedCookie; Unneeded, see include above. > Source/WebCore/platform/blackberry/CookieManager.h:101 > + // Constants getter. Seems useless comment. > Source/WebCore/platform/blackberry/CookieParser.cpp:93 > +{ All comments in here need review, most are not proper sentences lacking period etc. > Source/WebCore/platform/blackberry/CookieParser.cpp:332 > + return 0; This pattern of five lines gets repeated a lot, you could make a macro for it. > Source/WebCore/platform/blackberry/CookieParser.cpp:345 > + // Check if the cookie is valid with respect to the size limit/ Needs period. > ChangeLog:9 > + This test is ran twice and the averate read and write for each of the 2 runs is shown. average

Comment on attachment 128303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128303&action=review >> Source/WebCore/platform/blackberry/CookieJarBlackBerry.cpp:54 >> + String result = cookieManager().getCookie(url, NoHttpOnlyCookie); > > Seems no need for temp var. ok >> Source/WebCore/platform/blackberry/CookieManager.cpp:39 >> +#include "ParsedCookie.h" > > Already included in header, please do checks for similar cases. sure >> Source/WebCore/platform/blackberry/CookieManager.cpp:108 >> + // TODO: m_managerMap and the top layer protocolMaps are not properly deleted. > > TODO should be FIXME. changed >> Source/WebCore/platform/blackberry/CookieManager.cpp:134 >> + return false; > > Could do just return (protocol == "file" || protocol == "local" ); will do >> Source/WebCore/platform/blackberry/CookieManager.cpp:420 >> + initiateCookieLimitCleanUp(); > > Indenting. I'm surprised style checker didn't catch this... >> Source/WebCore/platform/blackberry/CookieManager.cpp:453 >> + bool oldIsSession = oldCookie->isSession(); > > These can go into the if. updating >> Source/WebCore/platform/blackberry/CookieManager.h:44 >> +class ParsedCookie; > > Unneeded, see include above. fixing >> Source/WebCore/platform/blackberry/CookieManager.h:101 >> + // Constants getter. > > Seems useless comment. ok >> Source/WebCore/platform/blackberry/CookieParser.cpp:93 >> +{ > > All comments in here need review, most are not proper sentences lacking period etc. updated the comment to reflect what is going on >> Source/WebCore/platform/blackberry/CookieParser.cpp:332 >> + return 0; > > This pattern of five lines gets repeated a lot, you could make a macro for it. good idea >> Source/WebCore/platform/blackberry/CookieParser.cpp:345 >> + // Check if the cookie is valid with respect to the size limit/ > > Needs period. probably a typo >> ChangeLog:9 >> + This test is ran twice and the averate read and write for each of the 2 runs is shown. > > average oops

Created attachment 128496 [details] Patch

Comment on attachment 128496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128496&action=review Looks good, with some cleanups before landing. Note that these files will receive further cleanups after this has landed. > Source/WebCore/ChangeLog:1 > +2012-02-22 Konrad Piascik <kpiascik@rim.com> It is 23rd. > Source/WebCore/platform/blackberry/CookieManager.cpp:75 > + cookieManager().flushCookiesToBackingStore(); Could you do more cleanup here in future? Like deleting leaking "cookie" singletons? > Source/WebCore/platform/blackberry/CookieManager.cpp:219 > + No empty line please. > Source/WebCore/platform/blackberry/CookieManager.cpp:267 > + int i = delimitedHost.size()-1; int i = delimitedHost.size() - 1; > Source/WebCore/platform/blackberry/CookieManager.h:100 > + static unsigned int maxCookieLength() { return s_maxCookieLength; } Just unsigned should be the same, note that s_maxCookieLength is an unsigned itself. > Source/WebCore/platform/blackberry/CookieManager.h:144 > + static const unsigned s_maxCookieLength = 4096; This is uncommon, but according to some Google searches it should still work. > Source/WebCore/platform/blackberry/CookieMap.cpp:145 > + Better remove the empty line. > ManualTests/cookieSpeedTest.html:6 > + for(var i=0; i < 100; i++){ Better make the 100 into a constant, maybe we will run so quick in the future that we need bigger values :)

Created attachment 128534 [details] Patch fixes

(In reply to comment #19) > > Source/WebCore/platform/blackberry/CookieManager.cpp:75 > > + cookieManager().flushCookiesToBackingStore(); > > Could you do more cleanup here in future? Like deleting leaking "cookie" singletons? > Yes we can add cleanup more cleanup code here if needed in the future.

Comment on attachment 128534 [details] Patch Looks good.

Comment on attachment 128534 [details] Patch Clearing flags on attachment: 128534 Committed r108722: <http://trac.webkit.org/changeset/108722>

All reviewed patches have been landed. Closing bug.