[webkit-reviews] review denied: [Bug 23365] Hook up accelerated compositing layers into WebHTMLView : [Attachment 26781] Patch, changelog
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Jan 31 16:35:06 PST 2009
Darin Adler <darin at apple.com> has denied Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 23365: Hook up accelerated compositing layers into WebHTMLView
https://bugs.webkit.org/show_bug.cgi?id=23365
Attachment 26781: Patch, changelog
https://bugs.webkit.org/attachment.cgi?id=26781&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> +#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.
More information about the webkit-reviews
mailing list