[webkit-reviews] review denied: [Bug 73654] Upstream BlackBerry Cookie Management Classes : [Attachment 128254] 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 128254: Patch
https://bugs.webkit.org/attachment.cgi?id=128254&action=review

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