Bug 177111 - [Curl] improve the implementation of FormDataStream
Summary: [Curl] improve the implementation of FormDataStream
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-18 16:43 PDT by Basuke Suzuki
Modified: 2017-09-27 12:20 PDT (History)
8 users (show)

See Also:


Attachments
Fix (8.15 KB, patch)
2017-09-18 17:12 PDT, Basuke Suzuki
achristensen: review-
Details | Formatted Diff | Diff
fix (7.97 KB, patch)
2017-09-19 22:56 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2017-09-18 16:43:27 PDT
The implementation of FormDataStream in Curl port was old. Improve it using latest classes.
Comment 1 Basuke Suzuki 2017-09-18 17:12:45 PDT
Created attachment 321156 [details]
Fix
Comment 2 Alex Christensen 2017-09-18 20:55:12 PDT
Comment on attachment 321156 [details]
Fix

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

> Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.h:109
> +    RefPtr<FormDataStream> m_formDataStream { nullptr };

nullptr initializer isn't needed for RefPtr.
Couldn't you just use a std::unique_ptr instead of making FormDataStream refcounted?
Comment 3 Basuke Suzuki 2017-09-19 13:29:11 PDT
(In reply to Alex Christensen from comment #2)
> Comment on attachment 321156 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321156&action=review
> 
> > Source/WebCore/platform/network/curl/ResourceHandleCurlDelegate.h:109
> > +    RefPtr<FormDataStream> m_formDataStream { nullptr };
> 
> nullptr initializer isn't needed for RefPtr.

right. Will do.

How do you think of integer value initialization? They are default to zero if empty braces are placed like pointers will be nullptr. For clarity { 0 } is much acceptable for me compared to { nullptr }, but either works for me. Sooner or later people get understand when they see:
   int foobar { };


> Couldn't you just use a std::unique_ptr instead of making FormDataStream
> refcounted?

Do you mean use std::unique_ptr even FormData is RefCounted object? If so I don't understand the point. If you mean changing the whole implementation, because it is shared among the ports, that won't be one day patch and had better to be separated.
Comment 4 Alex Christensen 2017-09-19 13:31:17 PDT
FormDataStream is not refcounted, and might not need to be made refcounted like you did in this patch if you use std::unique_ptr instead of RefPtr.

int m_member { 0 }; is more explicit.  I'm not sure what people prefer.
Comment 5 Alex Christensen 2017-09-19 14:21:38 PDT
Comment on attachment 321156 [details]
Fix

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

> Source/WebCore/platform/network/curl/FormDataStreamCurl.cpp:8
>   * All rights reserved.
> + * Copyright (C) 2017 Sony Interactive Entertainment Inc.

Copyright should probably go above the All rights reserved.
Comment 6 Basuke Suzuki 2017-09-19 14:39:01 PDT
(In reply to Alex Christensen from comment #4)
> FormDataStream is not refcounted, and might not need to be made refcounted
> like you did in this patch if you use std::unique_ptr instead of RefPtr.

My apology I misunderstood you mentioned FromData, not FormDataStream. 

> int m_member { 0 }; is more explicit.  I'm not sure what people prefer.

Okay for me.
Comment 7 Basuke Suzuki 2017-09-19 22:56:40 PDT
Created attachment 321295 [details]
fix
Comment 8 WebKit Commit Bot 2017-09-20 10:31:10 PDT
Comment on attachment 321295 [details]
fix

Clearing flags on attachment: 321295

Committed r222269: <http://trac.webkit.org/changeset/222269>
Comment 9 WebKit Commit Bot 2017-09-20 10:31:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2017-09-27 12:20:33 PDT
<rdar://problem/34693088>