Bug 188614

Created attachment 347201 [details] Patch

Comment on attachment 347201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347201&action=review > Source/WebKit/Shared/CookieManagerCallbackID.h:32 > +using CookieManagerCallbackID = WebKit::CallbackID; Ideally we'll have a whole bunch of these. Could we put them all in the same file? Could we make CallbackID a template? enum class CallbackIDType { CookieManager, DrawingArea, ... } template<CallbackIDType> class CallbackID ... > Source/WebKit/UIProcess/WebCookieManagerProxy.h:58 > - > + :(

Comment on attachment 347201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347201&action=review > Source/WebKit/ChangeLog:11 > + e.g. DrawingAreaProxy's CallbackMap vs. WebPageProxy's. How are you going to make different CallbackIDs? Are you going to inherit classes from CallbackID? Or are you going to make CallbackID a template? Because having these definitions using CookieManagerCallbackID = WebKit::CallbackID; using DrawingAreaCallbackID = WebKit::CallbackID; does not make CookieManagerCallbackID and DrawingAreaCallbackID different. They are just aliases for WebKit::CallbackID. > Source/WebKit/Shared/OptionalCallbackID.h:35 > +template <typename CallbackIDType> > class OptionalCallbackID { Maybe you want to make CallbackID the default type of CallbackIDType: template <typename CallbackIDType = CallbackID> class OptionalCallbackID { So you can say OptionalCallbackID(m_callbacks.put(...)); Instead of OptionalCallbackID<CallbackID>(m_callbacks.put(...)); I know this is unrelated. But I think it will make sense if we make OptionalCallbackID a super class of CallbackID. They share most of the logic but CallbackID is just stricter than OptionalCallbackID. > Source/WebKit/Shared/OptionalCallbackID.h:39 > - ALWAYS_INLINE explicit OptionalCallbackID(const CallbackID& otherID) > + ALWAYS_INLINE explicit OptionalCallbackID(const CallbackIDType& otherID) This constructor looks weird to me. If we make OptionalCallbackID a super class of all the CallbackID classes then this can be replaced by the copy constructor. In fact you can delete it and rely on the compiler to generate it for you. But if you want the constructor to be that way and you are assuming that the input is a sub class of CallbackID, then I would suggest adding this assert: static_assert(std::is_base_of<CallbackID, CallbackIDType>::value, "The input should be a sub class of CallbackID"); > Source/WebKit/UIProcess/GenericCallback.h:170 > +template<typename CallbackIDType> > class CallbackMap { default typename: template<typename CallbackIDType = CallbackID> class CallbackMap { > Source/WebKit/UIProcess/GenericCallback.h:172 > CallbackID put(Ref<CallbackBase>&& callback) Do we need to change this to return CallbackIDType? > Source/WebKit/UIProcess/GenericCallback.h:193 > - CallbackID put(Function<void(T...)>&& function, const ProcessThrottler::BackgroundActivityToken& activityToken) > + CallbackIDType put(Function<void(T...)>&& function, const ProcessThrottler::BackgroundActivityToken& activityToken) This is getting more confusing. This is a template function which takes a callback function with a pack parameter typename. The callback creates a super class of CallbackBase. We are fine with any type of callback but we want the return type to be CallbackIDType. > Source/WebKit/UIProcess/GenericCallback.h:200 > - RefPtr<T> take(CallbackID callbackID) > + RefPtr<T> take(CallbackIDType callbackID) Ditto. This is a template function which takes the type of the return value but the input is the template class type. So it seems we are now stricter with the input parameter but we cast the output as the caller wants. > Source/WebKit/UIProcess/WebCookieManagerProxy.h:112 > + void didGetHostnamesWithCookies(const Vector<String>&, WebKit::CookieManagerCallbackID); > + void didGetHTTPCookieAcceptPolicy(uint32_t policy, WebKit::CookieManagerCallbackID); Do we need to have the namespace WebKit before CookieManagerCallbackID? I think CookieManagerCallbackID is in WebKit namespace. > Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:508 > - m_pendingCallbackIDs.append(static_cast<RemoteLayerTreeTransaction::TransactionCallbackID>(callbackID)); > + m_pendingCallbackIDs.append(callbackID); This casting could be removed without your patch. I wonder why we added it in the first place.

(In reply to Said Abou-Hallawa from comment #3) > Comment on attachment 347201 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=347201&action=review > > > Source/WebKit/ChangeLog:11 > > + e.g. DrawingAreaProxy's CallbackMap vs. WebPageProxy's. > > How are you going to make different CallbackIDs? Are you going to inherit > classes from CallbackID? Or are you going to make CallbackID a template? > Because having these definitions > > using CookieManagerCallbackID = WebKit::CallbackID; > using DrawingAreaCallbackID = WebKit::CallbackID; > > does not make CookieManagerCallbackID and DrawingAreaCallbackID different. > They are just aliases for WebKit::CallbackID. Alex's suggestion fixes this. > Maybe you want to make CallbackID the default type of CallbackIDType: > > template <typename CallbackIDType = CallbackID> > class OptionalCallbackID { > > So you can say > > OptionalCallbackID(m_callbacks.put(...)); > > Instead of > > OptionalCallbackID<CallbackID>(m_callbacks.put(...)); I don't want that, I want you to suffer just as much if you choose not to create your own type as if you do. Don't want the bad thing to be easier! > I know this is unrelated. But I think it will make sense if we make > OptionalCallbackID a super class of CallbackID. They share most of the logic > but CallbackID is just stricter than OptionalCallbackID. > > > Source/WebKit/Shared/OptionalCallbackID.h:39 > > - ALWAYS_INLINE explicit OptionalCallbackID(const CallbackID& otherID) > > + ALWAYS_INLINE explicit OptionalCallbackID(const CallbackIDType& otherID) > > This constructor looks weird to me. If we make OptionalCallbackID a super > class of all the CallbackID classes then this can be replaced by the copy > constructor. In fact you can delete it and rely on the compiler to generate > it for you. > > But if you want the constructor to be that way and you are assuming that the > input is a sub class of CallbackID, then I would suggest adding this assert: > > static_assert(std::is_base_of<CallbackID, CallbackIDType>::value, "The input > should be a sub class of CallbackID"); Ehh? This just means "construct me with one of the templatized type instead of any type". It doesn't necessarily HAVE to be one of the specific subtypes, because CallbackID is valid for folks who don't yet have their own type. > > Source/WebKit/UIProcess/GenericCallback.h:170 > > +template<typename CallbackIDType> > > class CallbackMap { > > default typename: Ditto above. > template<typename CallbackIDType = CallbackID> > class CallbackMap { > > > Source/WebKit/UIProcess/GenericCallback.h:172 > > CallbackID put(Ref<CallbackBase>&& callback) > > Do we need to change this to return CallbackIDType? > > > Source/WebKit/UIProcess/GenericCallback.h:193 > > - CallbackID put(Function<void(T...)>&& function, const ProcessThrottler::BackgroundActivityToken& activityToken) > > + CallbackIDType put(Function<void(T...)>&& function, const ProcessThrottler::BackgroundActivityToken& activityToken) > > This is getting more confusing. This is a template function which takes a > callback function with a pack parameter typename. The callback creates a > super class of CallbackBase. We are fine with any type of callback but we > want the return type to be CallbackIDType. > > > Source/WebKit/UIProcess/GenericCallback.h:200 > > - RefPtr<T> take(CallbackID callbackID) > > + RefPtr<T> take(CallbackIDType callbackID) > > Ditto. This is a template function which takes the type of the return value > but the input is the template class type. So it seems we are now stricter > with the input parameter but we cast the output as the caller wants. Right, this is just about strictening up the CallbackID types, not the callback functions themselves. That would be a different project. > > Source/WebKit/UIProcess/WebCookieManagerProxy.h:112 > > + void didGetHostnamesWithCookies(const Vector<String>&, WebKit::CookieManagerCallbackID); > > + void didGetHTTPCookieAcceptPolicy(uint32_t policy, WebKit::CookieManagerCallbackID); > > Do we need to have the namespace WebKit before CookieManagerCallbackID? I > think CookieManagerCallbackID is in WebKit namespace. Likely not. > > Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:508 > > - m_pendingCallbackIDs.append(static_cast<RemoteLayerTreeTransaction::TransactionCallbackID>(callbackID)); > > + m_pendingCallbackIDs.append(callbackID); > > This casting could be removed without your patch. I wonder why we added it > in the first place. Unknown.

Comment on attachment 347201 [details] Patch This has been requesting review for more than one year. If this is still needed, please rebase and re-request review.