[webkit-reviews] review granted: [Bug 177988] Web Inspector: Network Tab - Cookies Detail View : [Attachment 323143] [PATCH] Proposed Fix

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 9 10:49:08 PDT 2017


Brian Burg <bburg at apple.com> has granted Joseph Pecoraro <joepeck at webkit.org>'s
request for review:
Bug 177988: Web Inspector: Network Tab - Cookies Detail View
https://bugs.webkit.org/show_bug.cgi?id=177988

Attachment 323143: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=323143&action=review




--- Comment #15 from Brian Burg <bburg at apple.com> ---
Comment on attachment 323143
  --> https://bugs.webkit.org/attachment.cgi?id=323143
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=323143&action=review

r=me, this table code is super easy to follow now! (Well, at least it fits in
just one file..)

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:560
> +localizedStrings["Max-Age"] = "Max-Age";

This is a header literal, yes? In that case it shouldn't be localized.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:29
> +    {

I tend to agree with Devin's comment about moving optional stuff into
options={}.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:58
> +    // RFC 6265 Defines the HTTP Cookie and Set-Cookie header fields:

nit: defines

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:79
> +		   console.error("Failed to parse Cookie pair", pair);

Please use WI.reportInternalError("Failed to parse Cookie", {header, pair}) so
that engineers will report cookies that this code doesn't know how to parse.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:102
> +	       console.error("Failed to parse Set-Cookie header", header);

Ditto.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:123
> +		   console.error("Failed to parse Set-Cookie attribute:",
attribute);

Ditto.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:142
> +		       console.warn("Invalid MaxAge value:", attributeValue);

Is this something we ever care about seeing in Inspector^2?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:439
> +	       // FIXME: The backend sends multiple "Set-Cookie" headers in one
"Set-Cookie" with multiple values

Is this something that we can fix without worrying about breakage, or is it
performed outside of Inspector code?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:442
> +	       // ", " in the the date format "Expires=Tue, 03-Oct-2017
04:39:21 GMT". To improve hueristics

Nit: heuristics

> Source/WebInspectorUI/UserInterface/Views/ResourceCookiesContentView.js:114
> +    _incompleteSectionWithMessage(section, message)

Nit: there's no verb here but it's doing stuff. Perhaps 
_markSectionIncompleteWithMessage, _markSectionIncompleteWithLoadingIndicator?


More information about the webkit-reviews mailing list