Bug 162559 - [Fetch API] Refactor FetchBody to use std::experimental::variant
Summary: [Fetch API] Refactor FetchBody to use std::experimental::variant
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: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 151937
  Show dependency treegraph
 
Reported: 2016-09-26 07:38 PDT by youenn fablet
Modified: 2016-10-01 06:16 PDT (History)
3 users (show)

See Also:


Attachments
Patch (13.73 KB, patch)
2016-09-26 07:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-09-26 07:38:57 PDT
A body can only have one type of data at a time.
Comment 1 youenn fablet 2016-09-26 07:43:56 PDT
Created attachment 289829 [details]
Patch
Comment 2 Alex Christensen 2016-09-26 08:03:29 PDT
Comment on attachment 289829 [details]
Patch

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

Cool! I like this.

> Source/WebCore/Modules/fetch/FetchBody.cpp:321
> +        RefPtr<FormData> body = const_cast<FormData*>(&formDataBody());

Can we do a RefPtr<const FormData>?  See https://trac.webkit.org/changeset/203257

> Source/WebCore/Modules/fetch/FetchBody.cpp:344
> +        clone.m_data = const_cast<Blob&>(blobBody());

Could we make m_data a variant of types like Ref<const Blob> to avoid const_casts here?

> Source/WebCore/Modules/fetch/FetchBody.h:98
>      FetchBody(Type type) : m_type(type) { }

Do we need this?

> Source/WebCore/Modules/fetch/FetchBody.h:119
>      Type m_type { Type::None };

Doesn't the variant keep track of the type now?  This seems like redundant information.
Comment 3 youenn fablet 2016-09-26 08:13:37 PDT
Thanks for the feedback.

> > Source/WebCore/Modules/fetch/FetchBody.cpp:321
> > +        RefPtr<FormData> body = const_cast<FormData*>(&formDataBody());
> 
> Can we do a RefPtr<const FormData>?  See
> https://trac.webkit.org/changeset/203257

Interesting, will check this.

> > Source/WebCore/Modules/fetch/FetchBody.cpp:344
> > +        clone.m_data = const_cast<Blob&>(blobBody());
> 
> Could we make m_data a variant of types like Ref<const Blob> to avoid
> const_casts here?

Will check this.

> > Source/WebCore/Modules/fetch/FetchBody.h:98
> >      FetchBody(Type type) : m_type(type) { }
> 
> Do we need this?

Probably no longer, will check this.

> > Source/WebCore/Modules/fetch/FetchBody.h:119
> >      Type m_type { Type::None };
> 
> Doesn't the variant keep track of the type now?  This seems like redundant
> information.

Type is tracking other things, like if data is a ReadableStream, if data is stored in FetchBodyConsumer, or if its being loaded.
While we need to keep that information, it may be nicer to split the Type enum into two or more fields, one of which being redundant with the variant.
I plan to check this as a follow-up patch.
Comment 4 youenn fablet 2016-09-26 08:42:02 PDT
> > > Source/WebCore/Modules/fetch/FetchBody.cpp:344
> > > +        clone.m_data = const_cast<Blob&>(blobBody());
> > 
> > Could we make m_data a variant of types like Ref<const Blob> to avoid
> > const_casts here?
> 
> Will check this.

That is working for Blob, it is not working for ArrayBuffer and ArrayBufferView.
DeferrableRefCounted::ref/deref are not marked as const.

For FormData, I am not sure we should be using a Ref<const FormData> since we sometimes call FormData::generateFiles which is not const.

I think it makes sense to change Blob, ArrayBuffer and ArrayBufferView at the same time. Can we tackle it in a follow-up patch?
Comment 5 youenn fablet 2016-09-26 08:44:45 PDT
> > > Source/WebCore/Modules/fetch/FetchBody.h:98
> > >      FetchBody(Type type) : m_type(type) { }
> > 
> > Do we need this?
> 
> Probably no longer, will check this.

It is used to build FetchBody using readable code like { Type::Loading }.
I would prefer keeping it.
Comment 6 Alex Christensen 2016-09-26 22:53:02 PDT
Sure.  Followup patches can be done.
DeferrableRefCountedBase::m_refCount should be made mutable.
Comment 7 WebKit Commit Bot 2016-09-27 01:01:08 PDT
Comment on attachment 289829 [details]
Patch

Clearing flags on attachment: 289829

Committed r206421: <http://trac.webkit.org/changeset/206421>
Comment 8 WebKit Commit Bot 2016-09-27 01:01:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Darin Adler 2016-09-27 09:45:56 PDT
Even after reading the comments above, I think we should also get rid of m_type.
Comment 10 youenn fablet 2016-10-01 06:16:25 PDT
(In reply to comment #9)
> Even after reading the comments above, I think we should also get rid of
> m_type.

Partially done in bug 162559.