Since FrameTree is defined as a separate object, its API can be hidden from Frame. This saves compilation time, especially on changing FrameTree.h
Created attachment 126752 [details] Patch
Comment on attachment 126752 [details] Patch Attachment 126752 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11508753
Comment on attachment 126752 [details] Patch Attachment 126752 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11509726
Comment on attachment 126752 [details] Patch Attachment 126752 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11506763
Comment on attachment 126752 [details] Patch I am strongly in favor of this change (and changes like it). But last time I tried this, Sam Wenig mentioned that some of these were intentional for perf. Perhaps I'm mis-remembering Sam. But you should be sure to check the perf bots a day after you land just to make sure we haven't regressed.
Comment on attachment 126752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126752&action=review > Source/WebCore/page/Frame.h:108 > + Frame* parent() const; Why are we adding this? The reason we created FrameTree was to get functions like this out of Frame. Adding this back seems like a step in the wrong direction!
Comment on attachment 126752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126752&action=review > Source/WebCore/page/Frame.h:210 > - mutable FrameTree m_treeNode; > + OwnPtr<FrameTree> m_treeNode; Last time we tried this it was a significant slowdown. How did we measure performance this time?
I don’t think review+ is a good state for a patch that does not compile on Qt, GTK, or EFL, and that has unknown performance impact and makes at least one debatable design change (adding Frame::parent).
Comment on attachment 126752 [details] Patch My appologies. I was aware of all that you list, with the exception of the Frame::parent design choice at time of review. I trust that Morita would be able to see the build failures himself and fix them accordingly. I share your concern of the possibility of performance issues, but I put a lot of faith in our perf bots to catch possible regressions. It's difficult for me to understand how this could have much perf impact, although I believe you that others have shown it to in the past. We don't allocate that many Frame objects, and I have a difficutl time imagining that the pointer indirection to the FrameTree could be that hot. Do you have a specific test which you'd like to see this shown as clean on? We have PLT bots (http://build.chromium.org/f/chromium/perf/dashboard/overview.html) as well as http://webkit-perf.appspot.com/ which process every change, and should catch any regression. Based on Darin's comments, it sounds like he would like to see the EWS bots pass before any further review. I can't comment on the debatability of the parent accessor on Frame, but we could do some digging to find the old FrameTree bugs and read.
Darin, Eric, thanks for your comments. I was not aware the prior effort to do something like this and the consequence of the performance impact. I'm sorry for that. What I'm trying to do right now is for unveiling dependency to frame tree traversal because I'm going to attack <iframe> in Shadow DOM bug (Bug 65595) where I might need to touch the way to traversal the tree. Anyway, I'll clear all bot redness before continuing discussion.
Created attachment 126873 [details] Patch
After reading around the code, I start wondering that it may better to merge FrameTree to Frame. This is because certain number of classes already use FrameTree: https://gist.github.com/1822443 thus separating two classes doesn't help encapsulation very much. Another point is that any other tree-like classes don't represent their trees like this, that makes me feel this as an irregular design. I hope this being more like Node. What we need to do instead of decomposing FrameTree might be - Providing some more intelligent accessors for neighbor frames. - Providing some kind of traverser/iterator for neighboring Frame, FrameView and Document. Even people who don't care about Frame need to touch Frame and FrameTree to traverse its tree structure. I expect large part of FrameTree client follows a few of typical patterns. We could capture such patterns.
Unassign myself and close for now since currently I have no intention to push this.