[webkit-reviews] review granted: [Bug 51546] WebKit2 needs to mirror the frame tree in the UIProcess : [Attachment 77354] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 51546: WebKit2 needs to mirror the frame tree in the UIProcess
https://bugs.webkit.org/show_bug.cgi?id=51546

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=77354&action=review

> WebKit2/UIProcess/WebFrameProxy.cpp:184
> +    ASSERT(child->page() == page());

Assert that child->m_parentFrame is 0?
Assert that child->m_nextSibling is 0?
Assert that child->m_previousSibling is 0?

> WebKit2/UIProcess/WebFrameProxy.cpp:193
> +	   oldLast->m_nextSibling = child;

Assert that oldLast->m_nextSibling was 0?

> WebKit2/UIProcess/WebFrameProxy.cpp:205
> +    std::swap(newLocationForNext, child->m_nextSibling);
> +    std::swap(newLocationForPrevious, child->m_previousSibling);

How about "using namespace std" at the top instead?

> WebKit2/UIProcess/WebFrameProxy.cpp:226
> +void WebFrameProxy::dumpFrameTree(unsigned indent)

It would be nicer if the name made it clear it’s a stdout-print type of dump. I
like leaving this kind of function in the code as long as it’s obvious how to
invoke it. If only someone had left an easy string-printing function in
WTF::String!!!

> WebKit2/UIProcess/WebFrameProxy.h:136
> +    void removeAllChildFrames();

You mentioned we should remove this declaration for a function that is never
used or defined.