Need a WK2 API to filter which subframes go into WebArchives as they are created In radar as <rdar://problem/10742441>
Created attachment 125394 [details] Patch v1 - Add filtering capability to WebCore::LegacyWebArchive and expose as a new WK2 API
Comment on attachment 125394 [details] Patch v1 - Add filtering capability to WebCore::LegacyWebArchive and expose as a new WK2 API Instead of having a callback + context pointer, the more common way to do this in WebCore is to have an abstract class with a pure virtual member function and then make a subclass inside WebKit2. I think that would be more clear.
Created attachment 125415 [details] Patch v2 - Use a callback object for the WebCore -> WebKit interface
Comment on attachment 125415 [details] Patch v2 - Use a callback object for the WebCore -> WebKit interface View in context: https://bugs.webkit.org/attachment.cgi?id=125415&action=review > Source/WebCore/loader/archive/cf/LegacyWebArchive.h:40 > +class LegacyWebArchiveSubframeFilterCallback { Could just call this FrameFilter. There’s nothing here that is specific to LegacyWebArchive. > Source/WebCore/loader/archive/cf/LegacyWebArchive.h:43 > + virtual bool shouldIncludeSubframe(Frame*) = 0; I think this should be const. > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleFrame.h:77 > +typedef bool (*WKBundleFrameCopyWebArchiveSubframeFilterCallback)(WKBundleFrameRef frame, WKBundleFrameRef subframe, void* context); I don’t think the type needs to include the name of the function it’s used in. I would call this a WKBundleFrameFilterFunction. > Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:744 > + virtual bool shouldIncludeSubframe(Frame*); This should be private rather than public. This should use the override keyword. > Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:761 > + WebFrame* webFrame = static_cast<WebFrameLoaderClient*>(frame->loader()->client())->webFrame(); Might want to restructure this to use an early return so we don’t do all this work if m_callback is 0. > Source/WebKit2/WebProcess/WebPage/WebFrame.cpp:763 > + return m_callback ? m_callback(toAPI(m_topLevelWebFrame), toAPI(webFrame), m_context) : true; I sometimes write this as: return !m_callback || m_callback(xxx); Eliminates that thing where ": true" is so far from the "?". > Source/WebKit2/WebProcess/WebPage/WebFrame.h:141 > + typedef bool (*WebFrameCopyWebArchiveSubframeFilterCallback)(WKBundleFrameRef, WKBundleFrameRef subframe, void* context); Since this type is a member, I suggest keeping its name much smaller. Maybe FrameFilterFunction.