[webkit-reviews] review granted: [Bug 137225] Add basic caching for Document.cookie API : [Attachment 238977] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 1 14:56:14 PDT 2014


Alexey Proskuryakov <ap at webkit.org> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 137225: Add basic caching for Document.cookie API
https://bugs.webkit.org/show_bug.cgi?id=137225

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=238977&action=review


> Source/WebCore/dom/Document.cpp:3858
> +	   setDOMCookieCache(cookies(this, cookieURL));

setDOMCookieCache sounds like it's taking some kind of CookieCache object,
which it is not. Maybe setCachedDOMCookies would be less confusing?

> Source/WebCore/dom/Document.cpp:3860
> +    return domCookieCache();

Ditto. cachedDOMCookies?

> Source/WebCore/dom/Document.cpp:3988
> +    m_cookieURL = url;
> +    invalidateDOMCookieCache();

I'm wondering if we ever call this function with the same value repeatedly. It
may not matter in practice, we probably don't do that between
document.cookie.calls too often, if ever.

> Source/WebCore/dom/Document.cpp:6197
> +    // The cookie cache is valid until we go back to the event loop.

Should probably say "at most", as sync loading can also invalidate it.

> LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html:18
> +<script src='resources/cookies-test-post.js'></script>

I'm somewhat concerned that this may not clean up the cookies before the next
test. Could you please double-check that?


More information about the webkit-reviews mailing list