Created attachment 289829 [details] Patch

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.

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.

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

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

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

Comment on attachment 289829 [details] Patch Clearing flags on attachment: 289829 Committed r206421: <http://trac.webkit.org/changeset/206421>

All reviewed patches have been landed. Closing bug.

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

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