Bug 162559 – [Fetch API] Refactor FetchBody to use std::experimental::variant

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

A body can only have one type of data at a time.

[youenn fablet]

Created attachment 289829 [details]
Patch

[Alex Christensen]

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.

[youenn fablet]

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.

[youenn fablet]

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

[youenn fablet]

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

[Alex Christensen]

Sure.  Followup patches can be done.
DeferrableRefCountedBase::m_refCount should be made mutable.

[WebKit Commit Bot]

Comment on attachment 289829 [details]
Patch

Clearing flags on attachment: 289829

Committed r206421: <http://trac.webkit.org/changeset/206421>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Even after reading the comments above, I think we should also get rid of m_type.

[youenn fablet]

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