Bug 189533 - Web Inspector: Cookies view should use model objects instead of raw payload data
Summary: Web Inspector: Cookies view should use model objects instead of raw payload data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-11 17:25 PDT by Matt Baker
Modified: 2018-12-14 12:41 PST (History)
8 users (show)

See Also:


Attachments
Patch (5.76 KB, patch)
2018-09-11 17:29 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.43 MB, application/zip)
2018-09-11 21:14 PDT, EWS Watchlist
no flags Details
Patch (6.79 KB, patch)
2018-09-12 18:12 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-sierra (3.16 MB, application/zip)
2018-09-12 20:15 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.61 MB, application/zip)
2018-09-12 23:06 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews103 for mac-sierra (2.55 MB, application/zip)
2018-09-12 23:36 PDT, EWS Watchlist
no flags Details
Patch (7.22 KB, patch)
2018-11-08 17:59 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (10.29 KB, patch)
2018-12-05 12:03 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch for landing (11.21 KB, patch)
2018-12-05 16:52 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.50 MB, application/zip)
2018-12-05 18:06 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (2.34 MB, application/zip)
2018-12-05 18:27 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.60 MB, application/zip)
2018-12-05 19:23 PST, EWS Watchlist
no flags Details
Patch for landing (17.08 KB, patch)
2018-12-13 20:14 PST, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2018-09-11 17:25:58 PDT
Some cleanup before work on https://bugs.webkit.org/show_bug.cgi?id=66381.
Comment 1 Radar WebKit Bug Importer 2018-09-11 17:26:24 PDT
<rdar://problem/44364183>
Comment 2 Matt Baker 2018-09-11 17:29:14 PDT
Created attachment 349496 [details]
Patch
Comment 3 EWS Watchlist 2018-09-11 21:14:18 PDT
Comment on attachment 349496 [details]
Patch

Attachment 349496 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9185480

New failing tests:
accessibility/smart-invert-reference.html
Comment 4 EWS Watchlist 2018-09-11 21:14:20 PDT
Created attachment 349518 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 5 Devin Rousso 2018-09-12 08:53:23 PDT
Comment on attachment 349496 [details]
Patch

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

r-, only because `WI.Cookie` is missing the "session" value, which is defined in the protocol.  We should include that, especially if we plan on using model objects, as otherwise other uses of `cookie.session` won't work.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:42
> +        this._type = type;

I've been trying to transition some of our "value bag" style model objects to use optional parameters for non-required values.

    constructor(type, name, value, {raw, expires, maxAge, path, domain, secure, httpOnly, sameSite} = {})

This makes it MUCH easier to add/remove parameters and makes the code easier to read at the construction site (in my opinion).  While you're doing this refresh, it'd be great if you could do that too (there are only a few creation sites so it should be easy).

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:86
> +    static fromPayload(payload)

Style: static "things" should be above public "things"
Comment 6 Joseph Pecoraro 2018-09-12 11:53:41 PDT
Comment on attachment 349496 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:65
> +    get type() { return this._type; }
> +    get name() { return this._name; }
> +    get value() { return this._value; }
> +    get rawHeader() { return this._rawHeader; }
> +    get expires() { return this._expires; }

I was hoping we could move some of the simple model objects away from getters/setters when they are readonly, like this. I think I introduced this class in 2017. I don't see any disadvantages to using direct properties (should be slightly simpler in all cases).
Comment 7 Matt Baker 2018-09-12 12:10:41 PDT
Comment on attachment 349496 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:42
>> +        this._type = type;
> 
> I've been trying to transition some of our "value bag" style model objects to use optional parameters for non-required values.
> 
>     constructor(type, name, value, {raw, expires, maxAge, path, domain, secure, httpOnly, sameSite} = {})
> 
> This makes it MUCH easier to add/remove parameters and makes the code easier to read at the construction site (in my opinion).  While you're doing this refresh, it'd be great if you could do that too (there are only a few creation sites so it should be easy).

Optional parameters solve a problem that doesn't exist, in this case. Cookies are always constructed with all parameters specified, or only the first three.

>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:65
>> +    get expires() { return this._expires; }
> 
> I was hoping we could move some of the simple model objects away from getters/setters when they are readonly, like this. I think I introduced this class in 2017. I don't see any disadvantages to using direct properties (should be slightly simpler in all cases).

I'm okay with using direct properties for POD model objects. Personally I prefer the explicitness and safety of even trivial getters/setters, but since the private properties that back them aren't really private, I suppose it's overkill.

>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:86
>> +    static fromPayload(payload)
> 
> Style: static "things" should be above public "things"

I left the order as-is to simplify the diff. Will change.
Comment 8 Brian Burg 2018-09-12 12:27:55 PDT
Comment on attachment 349496 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:42
>>> +        this._type = type;
>> 
>> I've been trying to transition some of our "value bag" style model objects to use optional parameters for non-required values.
>> 
>>     constructor(type, name, value, {raw, expires, maxAge, path, domain, secure, httpOnly, sameSite} = {})
>> 
>> This makes it MUCH easier to add/remove parameters and makes the code easier to read at the construction site (in my opinion).  While you're doing this refresh, it'd be great if you could do that too (there are only a few creation sites so it should be easy).
> 
> Optional parameters solve a problem that doesn't exist, in this case. Cookies are always constructed with all parameters specified, or only the first three.

So.. it's optional, and that should be obvious without hunting down call sites. I don't understand your objection.
Comment 9 Devin Rousso 2018-09-12 13:10:19 PDT
Comment on attachment 349496 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:42
>>>> +        this._type = type;
>>> 
>>> I've been trying to transition some of our "value bag" style model objects to use optional parameters for non-required values.
>>> 
>>>     constructor(type, name, value, {raw, expires, maxAge, path, domain, secure, httpOnly, sameSite} = {})
>>> 
>>> This makes it MUCH easier to add/remove parameters and makes the code easier to read at the construction site (in my opinion).  While you're doing this refresh, it'd be great if you could do that too (there are only a few creation sites so it should be easy).
>> 
>> Optional parameters solve a problem that doesn't exist, in this case. Cookies are always constructed with all parameters specified, or only the first three.
> 
> So.. it's optional, and that should be obvious without hunting down call sites. I don't understand your objection.

Not only is this about optional parameters, but also about readability.  If we ever needed to add another argument to `WI.Cookie`, it becomes much easier to read (especially in the diff) and the order doesn't matter.  Look at `WI.Resource` for an example of why this makes sense.

>>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:65
>>> +    get expires() { return this._expires; }
>> 
>> I was hoping we could move some of the simple model objects away from getters/setters when they are readonly, like this. I think I introduced this class in 2017. I don't see any disadvantages to using direct properties (should be slightly simpler in all cases).
> 
> I'm okay with using direct properties for POD model objects. Personally I prefer the explicitness and safety of even trivial getters/setters, but since the private properties that back them aren't really private, I suppose it's overkill.

I personally like getters, just to keep us on our "best behavior" :P

I find that direct properties are often MUCH harder to follow, especially for debugging.
Comment 10 Matt Baker 2018-09-12 13:26:29 PDT
(In reply to Devin Rousso from comment #9)
> Comment on attachment 349496 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349496&action=review
> 
> >>>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:42
> >>>> +        this._type = type;
> >>> 
> >>> I've been trying to transition some of our "value bag" style model objects to use optional parameters for non-required values.
> >>> 
> >>>     constructor(type, name, value, {raw, expires, maxAge, path, domain, secure, httpOnly, sameSite} = {})
> >>> 
> >>> This makes it MUCH easier to add/remove parameters and makes the code easier to read at the construction site (in my opinion).  While you're doing this refresh, it'd be great if you could do that too (there are only a few creation sites so it should be easy).
> >> 
> >> Optional parameters solve a problem that doesn't exist, in this case. Cookies are always constructed with all parameters specified, or only the first three.
> > 
> > So.. it's optional, and that should be obvious without hunting down call sites. I don't understand your objection.
> 
> Not only is this about optional parameters, but also about readability.  If
> we ever needed to add another argument to `WI.Cookie`, it becomes much
> easier to read (especially in the diff) and the order doesn't matter.  Look
> at `WI.Resource` for an example of why this makes sense.

Okay I'm convinced.

> >>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:65
> >>> +    get expires() { return this._expires; }
> >> 
> >> I was hoping we could move some of the simple model objects away from getters/setters when they are readonly, like this. I think I introduced this class in 2017. I don't see any disadvantages to using direct properties (should be slightly simpler in all cases).
> > 
> > I'm okay with using direct properties for POD model objects. Personally I prefer the explicitness and safety of even trivial getters/setters, but since the private properties that back them aren't really private, I suppose it's overkill.
> 
> I personally like getters, just to keep us on our "best behavior" :P
> 
> I find that direct properties are often MUCH harder to follow, especially
> for debugging.

Agree. I think direct properties obscure the class's interface.
Comment 11 Matt Baker 2018-09-12 18:12:59 PDT
Created attachment 349608 [details]
Patch
Comment 12 EWS Watchlist 2018-09-12 20:15:01 PDT
Comment on attachment 349608 [details]
Patch

Attachment 349608 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9198277

New failing tests:
inspector/unit-tests/cookie.html
Comment 13 EWS Watchlist 2018-09-12 20:15:03 PDT
Created attachment 349617 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 14 EWS Watchlist 2018-09-12 23:06:08 PDT
Comment on attachment 349608 [details]
Patch

Attachment 349608 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9199654

New failing tests:
http/tests/inspector/network/har/har-page.html
inspector/unit-tests/cookie.html
Comment 15 EWS Watchlist 2018-09-12 23:06:10 PDT
Created attachment 349632 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 16 EWS Watchlist 2018-09-12 23:36:32 PDT
Comment on attachment 349608 [details]
Patch

Attachment 349608 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9199948

New failing tests:
inspector/unit-tests/cookie.html
Comment 17 EWS Watchlist 2018-09-12 23:36:34 PDT
Created attachment 349633 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 18 Joseph Pecoraro 2018-09-13 10:36:08 PDT
Comment on attachment 349608 [details]
Patch

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

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

Honestly I'm not a fan of this construction for model objects. I know Devin has been pushing it. I think it creates unnecessary runtime work, but I don't have any supporting data.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:233
>          return new WI.Cookie(WI.Cookie.Type.Response, name, value, header, expires, maxAge, path, domain, secure, httpOnly, sameSite);

This constructor call site needs to be updated.
Comment 19 Matt Baker 2018-09-13 15:07:46 PDT
Comment on attachment 349608 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:28
>> +    constructor(type, name, value, {raw, session, expires, size, maxAge, path, domain, secure, httpOnly, sameSite} = {})
> 
> Honestly I'm not a fan of this construction for model objects. I know Devin has been pushing it. I think it creates unnecessary runtime work, but I don't have any supporting data.

I can see pros and cons, although in this case it actually made Cookie.fromPayload more awkward, not less. Since payload.expires needs to be converted to a Date, and modifying the original payload feels wrong, these more elegant approaches are unworkable:

static fromPayload(payload)
{
    // Cookie constructor expects payload.expires to be an instanceof Date.
    return new WI.Cookie(WI.Cookie.Type.Response, payload.name, payload.value, payload);
}

static fromPayload(payload)
{
    // Modifying the original payload seems like bad a practice.
    if (!payload.session && payload.expires)
        payload.expires = new Date(payload.expires);

    return new WI.Cookie(WI.Cookie.Type.Response, payload.name, payload.value, payload);
}

As far as I can see, there are two benefits to grouping optional arguments into an object:

1) Code becomes self-documenting, as Brian points out. It is clear looking at a model's constructor which arguments are optional.
2) Excluding optional arguments can be cleaner. This only applies when the number of optional arguments is large. For example, `new Foo(required, null, null, option3, option4, null, option6)` becomes `new Foo(required, {option3, option4, option6})`.

Cookie, specifically, does not benefit from 2 since we always specify all of the optional arguments or none of them, which I mentioned in an earlier comments but may not have been clear.
Comment 20 Matt Baker 2018-09-13 15:11:59 PDT
As far as performance differences, I didn't notice anything significant running the following (crude) tests:

<script>
class Object {
    constructor(a, b, c, d, e, f, g, h) {
        this.a = a;
        this.b = b;
        this.c = c;
        this.d = d;
        this.e = e;
        this.f = f;
        this.g = g;
        this.h = h;
    }
}

class ObjectUsingOptions {
    constructor(a, b, c, {d, e, f, g, h} = {}) {
        this.a = a;
        this.b = b;
        this.c = c;
        this.d = d;
        this.e = e;
        this.f = f;
        this.g = g;
        this.h = h;
    }
}

const n = 10000;

let name1 = `Create ${n} Objects.`;
console.profile(name1);
for (let i = 0; i < n; ++i) {
    let a = "a", b = "b", c = "c", d = "d", e = "e", f = "f", g = "g", h = "h";
    new Object("a", "b", "c", "d", "e", "f", "g", "h");
}
console.profileEnd(name1);

let name2 = `Create ${n} Objects with options.`;
console.profile(name2);
for (let i = 0; i < n; ++i) {
    let options = {d: "d", e: "e", f: "f", g: "g", h: "h"};
    new ObjectUsingOptions("a", "b", "c", options);
}
console.profileEnd(name2);
</script>
Comment 21 Matt Baker 2018-09-13 15:38:33 PDT
(In reply to Devin Rousso from comment #5)
> Comment on attachment 349496 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349496&action=review
> 
> r-, only because `WI.Cookie` is missing the "session" value, which is
> defined in the protocol.  We should include that, especially if we plan on
> using model objects, as otherwise other uses of `cookie.session` won't work.

I wonder if we could get away with not passing `session` to the Cookie constructor. According to the documentation for NSHTTPCookie (https://developer.apple.com/documentation/foundation/nshttpcookie/1392991-sessiononly), having an expiration date does not imply !session. However, we don't surface these two values separately in the Storage tab. A cookie with session=true and expires=Date will show "Session" in the Expires column. This is consistent with the documentation, which states that if session is true, "the cookie should be discarded at the end of the session (regardless of expiration date)."

So we could check the values of session and expires when decoding the payload, and null out expires if session is true. UI using Cookie can test for !expires, or we could keep the `session` getter but have it derive its value.
Comment 22 Devin Rousso 2018-09-13 16:42:09 PDT
Comment on attachment 349608 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:28
>>> +    constructor(type, name, value, {raw, session, expires, size, maxAge, path, domain, secure, httpOnly, sameSite} = {})
>> 
>> Honestly I'm not a fan of this construction for model objects. I know Devin has been pushing it. I think it creates unnecessary runtime work, but I don't have any supporting data.
> 
> I can see pros and cons, although in this case it actually made Cookie.fromPayload more awkward, not less. Since payload.expires needs to be converted to a Date, and modifying the original payload feels wrong, these more elegant approaches are unworkable:
> 
> static fromPayload(payload)
> {
>     // Cookie constructor expects payload.expires to be an instanceof Date.
>     return new WI.Cookie(WI.Cookie.Type.Response, payload.name, payload.value, payload);
> }
> 
> static fromPayload(payload)
> {
>     // Modifying the original payload seems like bad a practice.
>     if (!payload.session && payload.expires)
>         payload.expires = new Date(payload.expires);
> 
>     return new WI.Cookie(WI.Cookie.Type.Response, payload.name, payload.value, payload);
> }
> 
> As far as I can see, there are two benefits to grouping optional arguments into an object:
> 
> 1) Code becomes self-documenting, as Brian points out. It is clear looking at a model's constructor which arguments are optional.
> 2) Excluding optional arguments can be cleaner. This only applies when the number of optional arguments is large. For example, `new Foo(required, null, null, option3, option4, null, option6)` becomes `new Foo(required, {option3, option4, option6})`.
> 
> Cookie, specifically, does not benefit from 2 since we always specify all of the optional arguments or none of them, which I mentioned in an earlier comments but may not have been clear.

One of the reasons I like using the optional parameters is because it allows us to effectively drop any `fromPayload` methods, as we can create them ourselves.  I recognize that in this case, we need to create a Date, so that isn't truly the case, but the point is there nonetheless.

You can use object rest/spread to get what you want from the payload object without having to modify it.

    let {name, value, ...optionalArguments} = payload;
    if (optionalArguments.expires)
        optionalArguments.expires = new Date(optionalArguments.expires);

    return new WI.Cookie(WI.Cookie.Type.Response, name, value, optionalArguments);
Comment 23 Matt Baker 2018-09-13 16:44:28 PDT
(In reply to Matt Baker from comment #19)
> As far as I can see, there are two benefits to grouping optional arguments
> into an object:
> 
> 1) Code becomes self-documenting, as Brian points out. It is clear looking
> at a model's constructor which arguments are optional.
> 2) Excluding optional arguments can be cleaner. This only applies when the
> number of optional arguments is large. For example, `new Foo(required, null,
> null, option3, option4, null, option6)` becomes `new Foo(required, {option3,
> option4, option6})`.

3) The order of optional parameters doesn't matter.

> Cookie, specifically, does not benefit from 2 since we always specify all of
> the optional arguments or none of them, which I mentioned in an earlier
> comments but may not have been clear.

Actually:

- maxAge isn't included in the Cookie payload
- session isn't part of the Set-Cookie HTTP response header. However, !expires implies session in this case, so it can still be passed to the Cookie constructor if we choose to keep session part of the model.
Comment 24 Matt Baker 2018-09-13 16:59:31 PDT
(In reply to Devin Rousso from comment #22)
> One of the reasons I like using the optional parameters is because it allows
> us to effectively drop any `fromPayload` methods, as we can create them
> ourselves.  I recognize that in this case, we need to create a Date, so that
> isn't truly the case, but the point is there nonetheless.

Actually I'm not sure this is the case. Of the ~20 models that implement fromPayload, only a handful are trivial (Layer, LoggingChannel, TypeSet).

Interestingly, a lot of the non-trivial fromPayload routines *do* mutate the payload. In practice it's okay, since we discard it after creating the model, but I'm still not a huge fan.

For the record, here are the models that mutate their payloads: CollectionEntry, CollectionEntryPreview, ObjectPreview, PropertyDescriptor, PropertyPreview, Recording, RemoteObject, StructureDescription

> You can use object rest/spread to get what you want from the payload object
> without having to modify it.
> 
>     let {name, value, ...optionalArguments} = payload;
>     if (optionalArguments.expires)
>         optionalArguments.expires = new Date(optionalArguments.expires);
> 
>     return new WI.Cookie(WI.Cookie.Type.Response, name, value,
> optionalArguments);

Cool!
Comment 25 Brian Burg 2018-10-08 09:28:37 PDT
Clearing r? due to EWS failures.
Comment 26 Matt Baker 2018-10-08 10:40:10 PDT
(In reply to Brian Burg from comment #25)
> Clearing r? due to EWS failures.

I now feel that this refactor isn’t mission critical and will revisit at a later date.
Comment 27 Matt Baker 2018-11-08 17:59:36 PST
Created attachment 354298 [details]
Patch
Comment 28 Joseph Pecoraro 2018-11-13 17:46:13 PST
Comment on attachment 354298 [details]
Patch

r=me

You could write a test for cookie.url. There is:
LayoutTests/inspector/unit-tests/cookie.html
Comment 29 Matt Baker 2018-12-05 12:03:35 PST
Created attachment 356635 [details]
Patch
Comment 30 Matt Baker 2018-12-05 12:06:34 PST
(In reply to Matt Baker from comment #29)
> Created attachment 356635 [details]
> Patch

Cleared r+; I changed Cookie to use the optional arguments pattern suggested by Devin, since it makes creating Cookie objects less cumbersome. :)
Comment 31 Devin Rousso 2018-12-05 15:42:36 PST
Comment on attachment 356635 [details]
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` πŸ€”
Comment 32 Matt Baker 2018-12-05 16:52:34 PST
Created attachment 356679 [details]
Patch for landing
Comment 33 WebKit Commit Bot 2018-12-05 17:46:36 PST
The commit-queue encountered the following flaky tests while processing attachment 356679 [details]:

http/wpt/css/css-animations/start-animation-001.html bug 190903 (authors: dino@apple.com, fred.wang@free.fr, and graouts@apple.com)
inspector/debugger/tail-deleted-frames-this-value.html bug 192442 (authors: drousso@apple.com, joepeck@webkit.org, and sbarati@apple.com)
The commit-queue is continuing to process your patch.
Comment 34 EWS Watchlist 2018-12-05 18:06:13 PST
Comment on attachment 356679 [details]
Patch for landing

Attachment 356679 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10285890

New failing tests:
inspector/unit-tests/cookie.html
Comment 35 EWS Watchlist 2018-12-05 18:06:15 PST
Created attachment 356690 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 36 EWS Watchlist 2018-12-05 18:27:00 PST
Comment on attachment 356679 [details]
Patch for landing

Attachment 356679 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10285689

New failing tests:
inspector/unit-tests/cookie.html
Comment 37 EWS Watchlist 2018-12-05 18:27:02 PST
Created attachment 356691 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 38 EWS Watchlist 2018-12-05 19:23:15 PST
Comment on attachment 356679 [details]
Patch for landing

Attachment 356679 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10286656

New failing tests:
inspector/unit-tests/cookie.html
Comment 39 EWS Watchlist 2018-12-05 19:23:17 PST
Created attachment 356697 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 40 Matt Baker 2018-12-13 20:14:51 PST
Created attachment 357293 [details]
Patch for landing
Comment 41 WebKit Commit Bot 2018-12-14 12:41:38 PST
Comment on attachment 357293 [details]
Patch for landing

Clearing flags on attachment: 357293

Committed r239226: <https://trac.webkit.org/changeset/239226>
Comment 42 WebKit Commit Bot 2018-12-14 12:41:40 PST
All reviewed patches have been landed.  Closing bug.