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

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied 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 128303: Patch
https://bugs.webkit.org/attachment.cgi?id=128303&action=review

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