Bug 189533 – Web Inspector: Cookies view should use model objects instead of raw payload data

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Some cleanup before work on https://bugs.webkit.org/show_bug.cgi?id=66381.

[Radar WebKit Bug Importer]

<rdar://problem/44364183>

[Matt Baker]

Created attachment 349496 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Devin Rousso]

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"

[Joseph Pecoraro]

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).

[Matt Baker]

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.

[Blaze Burg]

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.

[Devin Rousso]

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.

[Matt Baker]

(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.

[Matt Baker]

Created attachment 349608 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Joseph Pecoraro]

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.

[Matt Baker]

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.

[Matt Baker]

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>

[Matt Baker]

(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.

[Devin Rousso]

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);

[Matt Baker]

(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.

[Matt Baker]

(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!

[Blaze Burg]

Clearing r? due to EWS failures.

[Matt Baker]

(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.

[Matt Baker]

Created attachment 354298 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 354298 [details]
Patch

r=me

You could write a test for cookie.url. There is:
LayoutTests/inspector/unit-tests/cookie.html

[Matt Baker]

Created attachment 356635 [details]
Patch

[Matt Baker]

(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. :)

[Devin Rousso]

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` 🤔

[Matt Baker]

Created attachment 356679 [details]
Patch for landing

[WebKit Commit Bot]

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: [e-mail], [e-mail], and [e-mail])
inspector/debugger/tail-deleted-frames-this-value.html bug 192442 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Matt Baker]

Created attachment 357293 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 357293 [details]
Patch for landing

Clearing flags on attachment: 357293

Committed r239226: <https://trac.webkit.org/changeset/239226>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.