Bug 23365 - Hook up accelerated compositing layers into WebHTMLView
: Hook up accelerated compositing layers into WebHTMLView
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P1 Normal
Assigned To: Simon Fraser (smfr)
:
Depends on:
Blocks: 23359
  Show dependency treegraph
 
Reported: 2009-01-15 15:22 PST by Simon Fraser (smfr)
Modified: 2009-03-02 11:51 PST (History)
3 users (show)

See Also:


Attachments
Patch, changelog (17.24 KB, patch)
2009-01-15 18:36 PST, Simon Fraser (smfr)
darin: review-
Details | Formatted Diff | Diff
Revised patch, changelogs (25.07 KB, patch)
2009-02-01 12:59 PST, Simon Fraser (smfr)
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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

> +#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.
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())

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