Some cleanup before work on https://bugs.webkit.org/show_bug.cgi?id=66381.
<rdar://problem/44364183>
Created attachment 349496 [details] Patch
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
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 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 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 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 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 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.
(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.
Created attachment 349608 [details] Patch
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
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 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
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 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
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 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 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.
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>
(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 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);
(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.
(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!
Clearing r? due to EWS failures.
(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.
Created attachment 354298 [details] Patch
Comment on attachment 354298 [details] Patch r=me You could write a test for cookie.url. There is: LayoutTests/inspector/unit-tests/cookie.html
Created attachment 356635 [details] Patch
(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 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` 🤔
Created attachment 356679 [details] Patch for landing
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 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
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 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
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 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
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
Created attachment 357293 [details] Patch for landing
Comment on attachment 357293 [details] Patch for landing Clearing flags on attachment: 357293 Committed r239226: <https://trac.webkit.org/changeset/239226>
All reviewed patches have been landed. Closing bug.