Bug 23365 - Hook up accelerated compositing layers into WebHTMLView
: Hook up accelerated compositing layers into WebHTMLView
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P1 Normal
Assigned To:
:
:
:
: 23359
  Show dependency treegraph
 
Reported: 2009-01-15 15:22 PST by
Modified: 2009-03-02 11:51 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2009-01-15 18:36:12 PST -------
Created an attachment (id=26781) [details]
Patch, changelog
------- Comment #2 From 2009-01-31 16:35:06 PST -------
(From update of attachment 26781 [details])
> +#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 From 2009-02-01 12:59:09 PST -------
Created an attachment (id=27234) [details]
Revised patch, changelogs
------- Comment #4 From 2009-02-03 10:55:06 PST -------
(From update of attachment 27234 [details])
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 From 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