[webkit-reviews] review denied: [Bug 93127] Adding APIs to Chromium WebKit API to allow for creating and monitoring frame hierarchy. : [Attachment 156390] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Nasko Oskov <nasko at chromium.org>'s
request for review:
Bug 93127: Adding APIs to Chromium WebKit API to allow for creating and
monitoring frame hierarchy.
https://bugs.webkit.org/show_bug.cgi?id=93127

Attachment 156390: Patch
https://bugs.webkit.org/attachment.cgi?id=156390&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=156390&action=review


This is really close, but I'd like to see one more iteration.

> Source/WebKit/chromium/public/WebFrameClient.h:112
> +    // A child frame was created in this frame.
> +    virtual void didCreateFrame(WebFrame* parent, WebFrame* child) { }
> +
>      // This frame has been detached from the view.
> -    //
> -    // FIXME: Do not use this in new code. Currently this is used by code in

> -    // Chromium that errantly caches WebKit objects.
>      virtual void frameDetached(WebFrame*) { }
>  
>      // This frame is about to be closed.

Can you add a comment explaining the lifecycle of a frame w.r.t. these
functions?  Presumably didCreateFrame and frameDetached are always called at
the beginning and end of the lifecycle, respectively.  From our discussion, it
sounds like willClose is called before frameDetached, but only in some
situations...

> Source/WebKit/chromium/public/WebFrameClient.h:406
> -	   WebFrame* source,
> +	   WebFrame* sourceFrame,
> +	   WebFrame* targetFrame,

Doesn't this break the API?  Do we need to land this change in stages with a
stub?