[webkit-reviews] review granted: [Bug 77766] Need a WK2 API to filter which subframes go into WebArchives as they are created : [Attachment 125415] Patch v2 - Use a callback object for the WebCore -> WebKit interface

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 77766: Need a WK2 API to filter which subframes go into WebArchives as they
are created
https://bugs.webkit.org/show_bug.cgi?id=77766

Attachment 125415: Patch v2 - Use a callback object for the WebCore -> WebKit
interface
https://bugs.webkit.org/attachment.cgi?id=125415&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.