Bug 93127 – Adding APIs to Chromium WebKit API to allow for creating and monitoring frame hierarchy.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nasko Oskov]

Adding APIs to Chromium WebKit API to allow for creating and monitoring frame hierarchy.

[Nasko Oskov]

Created attachment 156390 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 156390 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156390&action=review

> Source/WebKit/chromium/public/WebDocument.h:108
> +    WEBKIT_EXPORT WebElement createElement(const WebString&);

I'd name this parameter.  Presumable it's the tag name?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:594
> +WebString WebFrameImpl::assignedName() const
> +{
> +    return m_frame->tree()->name();
> +}

Can we change these API names to match the WebCore names?  That might make your patch a bit harder to land...

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2136
> +      m_client->didCreateFrame(this, webframe.get());

four-space indent

[Adam Barth]

Comment on attachment 156390 [details]
Patch

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?

[Darin Fisher (:fishd, Google)]

Comment on attachment 156390 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156390&action=review

>> Source/WebKit/chromium/public/WebFrameClient.h:112
>>      // 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...

we need to remove willClose from the API.  it is never what you want.  frameDetached should probably also be renamed at some point to didDetachFrame.

[Nasko Oskov]

Created attachment 156470 [details]
Patch

[Nasko Oskov]

Comment on attachment 156390 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156390&action=review

>> Source/WebKit/chromium/public/WebDocument.h:108
>> +    WEBKIT_EXPORT WebElement createElement(const WebString&);
> 
> I'd name this parameter.  Presumable it's the tag name?

Add the name - tagName.

>>> Source/WebKit/chromium/public/WebFrameClient.h:112
>>>      // 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...
> 
> we need to remove willClose from the API.  it is never what you want.  frameDetached should probably also be renamed at some point to didDetachFrame.

I've added comments to reflect the lifetime. willClose is called very very late, when we have committed the provisional load and closing out old data sources. It wasn't apparent at all from the naming and I'd agree with Darin that this is not something you really want.

>> Source/WebKit/chromium/public/WebFrameClient.h:406
>> +        WebFrame* targetFrame,
> 
> Doesn't this break the API?  Do we need to land this change in stages with a stub?

Yes, you are totally right. My first mixed patch for Chrome and WebKit : ). I've added a stub.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:594
>> +}
> 
> Can we change these API names to match the WebCore names?  That might make your patch a bit harder to land...

I've added WebFrame::uniqueName. Once the change makes it over to the Chromium tree, I will change all callers to use that function and will subsequently rename the assignedName() to name(). Does that sound like a good plan?

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2136
>> +      m_client->didCreateFrame(this, webframe.get());
> 
> four-space indent

Fixed.

[Adam Barth]

Comment on attachment 156470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156470&action=review

> Source/WebKit/chromium/public/WebFrame.h:134
>      // The name of this frame.
>      virtual WebString name() const = 0;

Should we add a comment about how we're going to change this from uniqueName to assignedName ?

> Source/WebKit/chromium/public/WebFrameClient.h:418
>      virtual bool willCheckAndDispatchMessageEvent(
>          WebFrame* source,
>          WebSecurityOrigin target,
> +        WebDOMMessageEvent event)
> +    {
> +        return willCheckAndDispatchMessageEvent(source, 0, target, event);
> +    }
> +
> +    virtual bool willCheckAndDispatchMessageEvent(
> +        WebFrame* sourceFrame,
> +        WebFrame* targetFrame,
> +        WebSecurityOrigin target,
>          WebDOMMessageEvent) { return false; }

This seems backwards.  willCheckAndDispatchMessageEvent is called by WebKit, which means the new version needs to forward to the old version, not vice versa.

[Adam Barth]

> I've added WebFrame::uniqueName. Once the change makes it over to the Chromium tree, I will change all callers to use that function and will subsequently rename the assignedName() to name(). Does that sound like a good plan?

yes!

[Nasko Oskov]

Comment on attachment 156470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156470&action=review

>> Source/WebKit/chromium/public/WebFrameClient.h:418
>>          WebDOMMessageEvent) { return false; }
> 
> This seems backwards.  willCheckAndDispatchMessageEvent is called by WebKit, which means the new version needs to forward to the old version, not vice versa.

I thought it calls through Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp, which I've modified. Is it also called directly by WebKit?

[Adam Barth]

Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp is part of WebKit.

[Nasko Oskov]

Created attachment 156475 [details]
Patch

[Nasko Oskov]

Comment on attachment 156470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156470&action=review

>> Source/WebKit/chromium/public/WebFrame.h:134
>>      virtual WebString name() const = 0;
> 
> Should we add a comment about how we're going to change this from uniqueName to assignedName ?

Done.

>>> Source/WebKit/chromium/public/WebFrameClient.h:418
>>>          WebDOMMessageEvent) { return false; }
>> 
>> This seems backwards.  willCheckAndDispatchMessageEvent is called by WebKit, which means the new version needs to forward to the old version, not vice versa.
> 
> I thought it calls through Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp, which I've modified. Is it also called directly by WebKit?

Done.

[Adam Barth]

Comment on attachment 156475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156475&action=review

> Source/WebKit/chromium/public/WebFrameClient.h:417
> +        return willCheckAndDispatchMessageEvent(sourceFrame, target, event);

I would have marked the old willCheckAndDispatchMessageEvent function as deprecated, but hopefully you'll remove it soon enough.

[Build Bot]

Comment on attachment 156475 [details]
Patch

Attachment 156475 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13435195

[Albert J. Wong]

Comment on attachment 156475 [details]
Patch

committing.

[WebKit Review Bot]

Comment on attachment 156475 [details]
Patch

Clearing flags on attachment: 156475

Committed r124790: <http://trac.webkit.org/changeset/124790>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.