[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