[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