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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
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 at 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 :)