Bug 185654 – Cleanup platform Cookie

• Component: Platform
• Status: RESOLVED FIXED

[Daniel Bates]

Cleanup platform Cookie.

[Daniel Bates]

Created attachment 340425 [details]
Patch

[EWS Watchlist]

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.

[Daniel Bates]

Created attachment 340426 [details]
Patch

[EWS Watchlist]

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.

[Daniel Bates]

Created attachment 340428 [details]
Patch

[EWS Watchlist]

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.

[Alex Christensen]

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.

[Daniel Bates]

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

[Alex Christensen]

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.

[Brady Eidson]

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

[Alex Christensen]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Per Arne Vollan]

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.

[Daniel Bates]

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

[Daniel Bates]

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

[Daniel Bates]

Committed r231859: <https://trac.webkit.org/changeset/231859>

[Radar WebKit Bug Importer]

<rdar://problem/40302575>