[webkit-reviews] review denied: [Bug 32160] Web Inspector: Allow retrieval of all transmitted cookies : [Attachment 44318] patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 32160: Web Inspector: Allow retrieval of all transmitted cookies
https://bugs.webkit.org/show_bug.cgi?id=32160

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> +	   for (var c = 0; c < allCookies.length; ++c) {

c is a strange index name.

> +	       for (var id in WebInspector.resources) {
> +		   var resource = WebInspector.resources[id];
> +		   var match =
resource.documentURL.match(WebInspector.URLRegExp);

You don't need to do matching numCookeis x numResources times. It is easier to
go though resources and store their domains into a set first (numResources
steps), then you can get all cookies for given frame within numCookies x
numDomains steps.

>  
> +WebInspector.Cookies.cookieMatchesResourceURL = function(cookie,
resourceURL)
> +{

I don't see this method used.

> +
> +WebInspector.Cookies.cookieDomainMatchesResourceDomain =
function(cookieDomain, resourceDomain)
> +{
> +    if (cookieDomain.charAt(0) !== '.')
> +	   return resourceDomain === cookieDomain;
> +    return !!resourceDomain.match(new RegExp("^([^\\.]+\\.)?" +
cookieDomain.substring(1).escapeForRegExp() + "$"), "i");

A test for this would be nice. You need to do a inspector test that would match
a bunch of URLs against given domain on the frontend side.