Adding APIs to Chromium WebKit API to allow for creating and monitoring frame hierarchy.
Created attachment 156390 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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
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?
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.
Created attachment 156470 [details] Patch
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.
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.
> 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!
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?
Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp is part of WebKit.
Created attachment 156475 [details] Patch
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.
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.
Comment on attachment 156475 [details] Patch Attachment 156475 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13435195
Comment on attachment 156475 [details] Patch committing.
Comment on attachment 156475 [details] Patch Clearing flags on attachment: 156475 Committed r124790: <http://trac.webkit.org/changeset/124790>
All reviewed patches have been landed. Closing bug.