Bug 23365

Summary: Hook up accelerated compositing layers into WebHTMLView
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Simon Fraser (smfr) <simon.fraser>
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    
Description Flags
Patch, changelog
darin: review-
Revised patch, changelogs hyatt: review+

Description Simon Fraser (smfr) 2009-01-15 15:22:27 PST
This bug tracks changes relate to hooking up GraphicsLayers into the native view system on Mac.
Comment 1 Simon Fraser (smfr) 2009-01-15 18:36:12 PST
Created attachment 26781 [details]
Patch, changelog
Comment 2 Darin Adler 2009-01-31 16:35:06 PST
Comment on attachment 26781 [details]
Patch, changelog

> +    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"
> +#include "RenderLayerCompositor.h"
> +#endif
>  #include "RenderPart.h"


> +    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?

> +    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)


> +    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;


> +        _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.
Comment 3 Simon Fraser (smfr) 2009-02-01 12:59:09 PST
Created attachment 27234 [details]
Revised patch, changelogs
Comment 4 Dave Hyatt 2009-02-03 10:55:06 PST
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())


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.

Comment 5 Simon Fraser (smfr) 2009-02-03 12:14:51 PST
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