Cleanup platform Cookie.
Created attachment 340425 [details] Patch
Attachment 340425 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:76: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 340426 [details] Patch
Attachment 340426 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:76: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 340428 [details] Patch
Attachment 340428 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:76: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 340428 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340428&action=review > Source/WebCore/platform/Cookie.h:118 > - std::optional<String> value; > - decoder >> value; > - if (!value) > + if (!decoder.decode(cookie.value)) > return std::nullopt; This is moving away from the newer way of doing decoding.
(In reply to Alex Christensen from comment #7) > Comment on attachment 340428 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340428&action=review > > > Source/WebCore/platform/Cookie.h:118 > > - std::optional<String> value; > > - decoder >> value; > > - if (!value) > > + if (!decoder.decode(cookie.value)) > > return std::nullopt; > > This is moving away from the newer way of doing decoding. Can you please elaborate on the benefit with respect to this class?
The old way of decoding, decoder.decode(T&), requires that T have a default constructor. The new way of decoding, operator>>(std::optional<T>&) does not, so I introduced it to allow sending and receiving types that do not have a default constructor. It would be nice if our IPC code were consistent and new, so I don't think we should take a step towards the old way of doing things.
(In reply to Alex Christensen from comment #9) > The old way of decoding, decoder.decode(T&), requires that T have a default > constructor. The new way of decoding, operator>>(std::optional<T>&) does > not, so I introduced it to allow sending and receiving types that do not > have a default constructor. It would be nice if our IPC code were > consistent and new, so I don't think we should take a step towards the old > way of doing things. The new optional<> way allows us to decode types without default constructors - Which is great! I've used it for that reason. But it seems bizarre to say that means we can't *ever* use the old pattern, even when this constraint doesn't apply. I think the old way is much more readable code and still use it *when it can* be used. I disagree with the statement "it would be nice if our IPC code were consistent and new" "New" does not mean better, and "consistent" sounds to me like an attempt to impose a special case on the general case even when the general case would be preferable. Is there a technical, non-subjective reason why the new way should always be applied? Otherwise, could this patch be re-reviewed?
Comment on attachment 340428 [details] Patch I don't think it's good to undo code modernization work. Making the IPC code consistent and new is a step towards making it more autogenerated or generated by meta programming. If you really want to undo this work anyways, go for it.
Comment on attachment 340428 [details] Patch Attachment 340428 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7696036 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
Created attachment 340476 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 340428 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340428&action=review R=me. > Source/WebCore/platform/Cookie.h:62 > + return name.isNull() && value.isNull() && domain.isNull() && path.isNull() && !created I think the previous version was more readable, but I leave this up to you.
(In reply to Alex Christensen from comment #11) > Comment on attachment 340428 [details] > Patch > > I don't think it's good to undo code modernization work. I agree with Brady's characterization that what you call "modernization" is "an attempt to impose a special case on the general case even when the general case would be preferable." > Making the IPC code consistent and new is a step towards making it more autogenerated or generated by meta programming. I consider the goals "making the IPC code consistent and new" and "making it more autogenerated or generated by meta programming" as independent of each other. The former may make it easier to do the latter, but is not necessary to achieve the latter. > If you really want to undo this work anyways, go for it. We need to have a discussion (maybe on webkit-dev) about how we want to evolve the IPC decoding/encoding. Neither Brady nor I am convinced that decoding to std::optional is the "one true way" to do all decoding.
(In reply to Per Arne Vollan from comment #14) > > Source/WebCore/platform/Cookie.h:62 > > + return name.isNull() && value.isNull() && domain.isNull() && path.isNull() && !created > > I think the previous version was more readable, but I leave this up to you. Will revert this stylistic change before landing.
Committed r231859: <https://trac.webkit.org/changeset/231859>
<rdar://problem/40302575>