Bug 23365

Created attachment 26781 [details] Patch, changelog

Comment on attachment 26781 [details] Patch, changelog > +#if USE(ACCELERATED_COMPOSITING) > + class GraphicsLayer; > +#endif > class HitTestResult; Conditional sections should be in separate paragraphs after the unconditional, not sorted in. If existing conditionals are not done in this style, please change them. > #include "Page.h" > +#if USE(ACCELERATED_COMPOSITING) > +#include "RenderLayerCompositor.h" > +#endif > #include "RenderPart.h" Ditto. > + RenderView* view = static_cast<RenderView*>(m_frame->contentRenderer()); No typecast needed here. > + page->chrome()->client()->setNeedsSynchronizedGraphicsFlush(frame()); This client function should have "frame" in its name, and I also think the name needs to be clearer that it's setting a one-time state. Needs a single synchronized flush, not synchronized flushing forever. Also, I think it's not good for multiple independent frames to each think this is needed or not. Is this something that needs to be coordinated at a higher level, such as the WebView or the entire NSWindow? > + void updateCompositingLayers(bool force = false); Can we avoid adding this new function with a boolean? How about using two named functions? Or an enum instead of a boolean? > + void wasAddedToWindow(); > + void willBeRemovedFromWindow(); Window is a confusing term in a web browser, and we normally avoid the term. Maybe HostWindow? Hyatt would have an opinion. > + WebFrameView* frameView = [kit(frame) frameView]; > + WebHTMLView* docView = (WebHTMLView *)[frameView documentView]; Why did you choose to put frameView into a local variable, but not the frame? I suggest collapsing these into one line. I'm not a fan of "doc" as an abbreviation for document. How about documentView? > +#if USE(ACCELERATED_COMPOSITING) > + NSView* layerHostingView; > + BOOL needsSynchronizedFlush; > +#endif Trailing spaces on the #endif. For NSView we put a space before the *; it's a strange rule, but we've mostly been consistent about it (it's in the style guide). > + NSArray* newSubviews = [[NSArray alloc] initWithObjects:_private->layerHostingView, nil]; > + _subviews = newSubviews; Whoah, really? > - if (_private->enumeratingSubviews) > + if (_private && _private->enumeratingSubviews) You need a comment explaining why this caller needs to null-check _private. > - if (_private->enumeratingSubviews) > + if (_private && _private->enumeratingSubviews) Ditto. > +#if USE(ACCELERATED_COMPOSITING) > + return (_private->layerHostingView != 0); We don't use parentheses in a return statement like this. Also, you should be using "nil", not "0", since this is an Objective-C object pointer. > + [_private->layerHostingView setLayer:0]; nil rather than 0. > + _private->layerHostingView = 0; Ditto. > + _private->page->wasAddedToWindow(); Either the behavior or the function name here is wrong. We were not added to the window yet at this point (in viewWillMoveToWindow). I'm going to say review- for now since there were a lot of comments, but they were all minor.

Created attachment 27234 [details] Revised patch, changelogs

Comment on attachment 27234 [details] Revised patch, changelogs Drop the "E" from ECompositingUpdate. I prefer the enum style of including the type in the values to help make it more clear. How about NormalCompositingUpdate, ForcedCompositingUpdate. Change if (!parent->renderer()->view()) to if (parent->renderer()->documentBeingDestroyed()) Obj-C style is NSView *layerHostingView. Remember that all code you add to WebHTMLView is just a future headache, since we want to move to a viewless model in the future. r=me

Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/WebCore.base.exp M WebCore/page/ChromeClient.h M WebCore/page/FrameView.cpp M WebCore/page/FrameView.h M WebCore/page/Page.cpp M WebCore/page/Page.h M WebCore/rendering/RenderLayerCompositor.cpp M WebCore/rendering/RenderLayerCompositor.h M WebCore/rendering/RenderView.cpp M WebCore/rendering/RenderView.h M WebKit/mac/ChangeLog M WebKit/mac/WebCoreSupport/WebChromeClient.h M WebKit/mac/WebCoreSupport/WebChromeClient.mm M WebKit/mac/WebView/WebHTMLView.mm M WebKit/mac/WebView/WebHTMLViewInternal.h M WebKit/mac/WebView/WebHTMLViewPrivate.h M WebKit/mac/WebView/WebView.mm M WebKit/mac/WebView/WebViewInternal.h Committed r40543