Bug 23365 – Hook up accelerated compositing layers into WebHTMLView

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

This bug tracks changes relate to hooking up GraphicsLayers into the native view system on Mac.

[Simon Fraser (smfr)]

Created attachment 26781 [details]
Patch, changelog

[Darin Adler]

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.

[Simon Fraser (smfr)]

Created attachment 27234 [details]
Revised patch, changelogs

[Dave Hyatt]

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

[Simon Fraser (smfr)]

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