[webkit-reviews] review granted: [Bug 65791] [chromium] Make WebViewImpl point at CCLayerTreeHost and related separation : [Attachment 103548] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 10 17:01:46 PDT 2011


James Robinson <jamesr at chromium.org> has granted Nat Duca
<nduca at chromium.org>'s request for review:
Bug 65791: [chromium] Make WebViewImpl point at CCLayerTreeHost and related
separation
https://bugs.webkit.org/show_bug.cgi?id=65791

Attachment 103548: Patch
https://bugs.webkit.org/attachment.cgi?id=103548&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103548&action=review


This is looking really great.  Left a bunch of nits, but besides a bunch of
style-type things I think we're ready to rock and roll.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:141
> +    // TODO: This setting is currently ignored.

FIXME is preferred in WebKit style

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:152
> +    // TOOD, replace with CCLayerTreeHost

TOOD->TODO, or even better FIXME

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:595
> +    // FIXME: The multithreaded compositor case will not work as long as
> +    // copyTexImage2D resolves to the parent texture, because the main
> +    // thread can execute WebGL calls on the child context at any time,
> +    // potentially clobbering the parent texture that is being renderered
> +    // by the compositor thread.

I'm pretty sure you can nuke this FIXME now that Al fixed the command buffer's
glFlush() semantics.  This is all good now.

I also don't think we really need to flush() all child contexts any more since
we do a flush() on the appropriate context in updateCompositorResources(). 
That's probably for another patch, tho.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:620
> +	   // TODOD: might have to call

TODOD->FIXME.  I'm pretty sure you don't need the call, though, the root layer
render surface will get blown away on the next draw anyway.  So maybe just
delete the comment completely.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:195
> +

nit: extra newline

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:270
> +#endif

nit: would you mind adding a comment to this #endif saying what it was for?
something like:
#endif // !USE(THREADED_COMPOSITING)
would be sufficient

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:58
> +struct CCSettings {

could you add a no-arg constructor that initializes the bools to something
reasonable (maybe all false), just to be sure that we don't allocate one of
these, forget to set a bool, and have it be some random value? it can still be
a struct

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:116
> +    explicit CCLayerTreeHost(CCLayerTreeHostClient*, const CCSettings&);

nit: no 'explicit' for 2-arg c'tors, only for 1-arg c'tors

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:121
> +    PassRefPtr<LayerRendererChromium> createRenderer();

although it's quite verbose, would you mind naming this createLayerRenderer()?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:68
> +    explicit CCLayerTreeHostImpl(CCLayerTreeHostImplClient*,
PassRefPtr<LayerRendererChromium>);

nit: no 'explicit' for 2-arg constructors, just 1-arg

> Source/WebKit/chromium/src/WebViewImpl.cpp:76
> +#include "LayerChromium.h"

do we need this #include? it's a surprising dependency

> Source/WebKit/chromium/src/WebViewImpl.cpp:343
> +    , m_layerTreeHost(0)

WTF smart pointers (RefPtr<>, OwnPtr<>, etc) will null-initialize themselves,
no need to do so yourself.


More information about the webkit-reviews mailing list