Summary: | Hook up accelerated compositing layers into WebHTMLView | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||
Component: | Layout and Rendering | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cmarrin, dino, hyatt | ||||||
Priority: | P1 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 23359 | ||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2009-01-15 15:22:27 PST
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 |