Bug 51546 - WebKit2 needs to mirror the frame tree in the UIProcess
Summary: WebKit2 needs to mirror the frame tree in the UIProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-23 11:22 PST by Sam Weinig
Modified: 2010-12-24 22:11 PST (History)
3 users (show)

See Also:


Attachments
Patch (38.10 KB, patch)
2010-12-23 11:49 PST, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2010-12-23 11:22:42 PST
Unfortunately, it looks like we need to mirror the frame tree in the UIProcess.
Comment 1 Sam Weinig 2010-12-23 11:49:58 PST
Created attachment 77354 [details]
Patch
Comment 2 WebKit Review Bot 2010-12-23 11:54:47 PST
Attachment 77354 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7322135
Comment 3 Darin Adler 2010-12-23 12:00:16 PST
Comment on attachment 77354 [details]
Patch

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.
Comment 4 Sam Weinig 2010-12-23 12:10:23 PST
Landed in r74571.
Comment 5 Adam Barth 2010-12-23 14:23:20 PST
(In reply to comment #0)
> Unfortunately, it looks like we need to mirror the frame tree in the UIProcess.

I'm curious why you need to mirror the Frame tree.  Chrome gets away without doing that.  The equivalent of the UIProcess only understands the concept of a WebView.
Comment 6 Sam Weinig 2010-12-24 14:33:19 PST
(In reply to comment #5)
> (In reply to comment #0)
> > Unfortunately, it looks like we need to mirror the frame tree in the UIProcess.
> 
> I'm curious why you need to mirror the Frame tree.  Chrome gets away without doing that.  The equivalent of the UIProcess only understands the concept of a WebView.

Safari uses the information in its Activity Window.  There are other ways to implement it however, so I will be trying to figure out if we can do something else efficiently enough that we can remove this.  I really want to remove this.  It sucks.
Comment 7 Adam Barth 2010-12-24 22:04:28 PST
> Safari uses the information in its Activity Window.  There are other ways to implement it however, so I will be trying to figure out if we can do something else efficiently enough that we can remove this.  I really want to remove this.  It sucks.

Ah, that makes sense.  Yeah, Chrome addresses this use case with the Web Inspector.  One of the design pressures we try to push back against is having the browser process (UIProcess) learn too much about WebKit internals.  Considerations might well be different in WebKit2, though, depending on how general a client you'd like to support.  Thanks for satisfying my curiosity.
Comment 8 Sam Weinig 2010-12-24 22:11:31 PST
(In reply to comment #7)
> > Safari uses the information in its Activity Window.  There are other ways to implement it however, so I will be trying to figure out if we can do something else efficiently enough that we can remove this.  I really want to remove this.  It sucks.
> 
> Ah, that makes sense.  Yeah, Chrome addresses this use case with the Web Inspector.  One of the design pressures we try to push back against is having the browser process (UIProcess) learn too much about WebKit internals.  Considerations might well be different in WebKit2, though, depending on how general a client you'd like to support.  Thanks for satisfying my curiosity.

That is a goal of WebKit2 as well.