[webkit-reviews] review granted: [Bug 189533] Web Inspector: Cookies view should use model objects instead of raw payload data : [Attachment 356635] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 189533: Web Inspector: Cookies view should use model objects instead of raw
payload data
https://bugs.webkit.org/show_bug.cgi?id=189533

Attachment 356635: Patch

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




--- Comment #31 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 356635
  --> https://bugs.webkit.org/attachment.cgi?id=356635
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:28
> +    constructor(type, name, value, {header, expires, maxAge, path, domain,
secure, httpOnly, sameSite, size} = {})

SO much better 😁

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:45
> +	   this._name = name || "";
> +	   this._value = value || "";

NIT: I know you copied this, but I typically use `||` fallbacks as a quick way
of distinguishing optional from non-optional, so since `name` and `value` are
effectively required, I'd remove the `|| ""`.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:46
> +	   this._size = size || this._name.length + this._value.length;

Interesting how we didn't have this value before.  It looks like it was sent by
the backend, but not used in the frontend?

Should we include a column for the `size` information in
`WI.ResourceCookieContentView`?  It looks like everything else is used there
except for this (and `_header`).

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:221
> +    get rawHeader() { return this._header; }

NIT: I'd rename this to just `header` while you're doing this refactoring.

> LayoutTests/ChangeLog:10
> +	   * inspector/unit-tests/cookie.html:

Unrelated: this doesn't appear to have any tests for
`WI.Cookie.parseCookieRequestHeader` 🤔