[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