The Chromium port of the accelerated compositing feature needs to be ported to Mac OS X.
Created attachment 63289 [details] Patch From the ChangeLog: Fixed compilation problems with gcc. Ported to Core Graphics and adjusted scrolling and incremental updating code, taking into account lower-left coordinate system origin. Added WebGLES2Context::resizeOnscreenContent, needed on Mac OS X to report window size changes. Sent resize notifications to WebGLES2Context. Note that this patch must land after http://codereview.chromium.org/3067026 .
Attachment 63289 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:855: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 63292 [details] Revised patch Fixed style error in previous patch.
Comment on attachment 63292 [details] Revised patch WebCore/platform/graphics/chromium/LayerChromium.cpp:190 + OwnPtr<GraphicsContext> graphicsContext(new GraphicsContext(contextCG.get())); nit: why not just allocate this GraphicsContext on the stack? WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:254 + m_rootLayerCGContext = RetainPtr<CGContextRef>(AdoptCF, 0); nit: you can also write m_rootLayerCGContext.adoptCF(0), which I think is more conventional. WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:117 + colorSpace = RetainPtr<CGColorSpaceRef>(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGBLinear)); colorSpace.adoptCF(CGColorSpaceCreateWithName(...)); WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:260 + m_rootLayerCGContext = RetainPtr<CGContextRef>(AdoptCF, CGBitmapContextCreate(m_rootLayerBackingStore.data(), ditto WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:390 + scrolledLayerMatrix.translate3d((int)floorf(0.5 * visibleRect.width() + 0.5) - scrollDelta.x(), nit: slightly unfortunate to copy/paste this fairly intricate statement. you could just use a local variable that has a value of either +1 or -1 and include that local variable in the final expression. WebKit/chromium/public/WebGLES2Context.h:64 + #if defined(__APPLE__) is this the best define, or would it make sense to use WEBKIT_USING_CG instead? would this code make sense if CG were used on other platforms (hypothetically speaking). WebKit/chromium/src/WebViewImpl.cpp:915 + #if OS(DARWIN) DARWIN or CG? WebKit/chromium/src/WebViewImpl.cpp:2230 + #if OS(DARWIN) ditto
Created attachment 63396 [details] Revised patch Addressed review feedback.
(In reply to comment #4) > (From update of attachment 63292 [details]) > WebCore/platform/graphics/chromium/LayerChromium.cpp:190 > + OwnPtr<GraphicsContext> graphicsContext(new GraphicsContext(contextCG.get())); > nit: why not just allocate this GraphicsContext on the stack? Done. I had just copied code from earlier in the file. > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:254 > + m_rootLayerCGContext = RetainPtr<CGContextRef>(AdoptCF, 0); > nit: you can also write m_rootLayerCGContext.adoptCF(0), which I think is more conventional. Done. > WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:117 > + colorSpace = RetainPtr<CGColorSpaceRef>(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGBLinear)); > colorSpace.adoptCF(CGColorSpaceCreateWithName(...)); Done. > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:260 > + m_rootLayerCGContext = RetainPtr<CGContextRef>(AdoptCF, CGBitmapContextCreate(m_rootLayerBackingStore.data(), > ditto Done. > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:390 > + scrolledLayerMatrix.translate3d((int)floorf(0.5 * visibleRect.width() + 0.5) - scrollDelta.x(), > nit: slightly unfortunate to copy/paste this fairly intricate statement. > you could just use a local variable that has a value of either +1 or -1 > and include that local variable in the final expression. Good idea. Done. > WebKit/chromium/public/WebGLES2Context.h:64 > + #if defined(__APPLE__) > is this the best define, or would it make sense to use WEBKIT_USING_CG instead? > would this code make sense if CG were used on other platforms (hypothetically > speaking). The associated #ifdef on the Chrome side is OS_MACOSX, so I think we should use this #ifdef here rather than WEBKIT_USING_CG. > WebKit/chromium/src/WebViewImpl.cpp:915 > + #if OS(DARWIN) > DARWIN or CG? > > WebKit/chromium/src/WebViewImpl.cpp:2230 > + #if OS(DARWIN) > ditto In both of these cases again the associated #ifdef in Chrome is OS_MACOSX, so I think this is the right #if.
Comment on attachment 63396 [details] Revised patch ok. I have no strong feelings with DARWIN vs. USING_WEBKIT_CG. If fishd objects, then we can change.
Committed r64656: <http://trac.webkit.org/changeset/64656>
> 107 RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGImageGetColorSpace(cgImage)); I think this is wrong: "CGImageGetColorSpace" doesn't contain "Copy" or "Create", so you're not supposed to release it. I'll send out a patch to fix this.
Comment on attachment 63396 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=63396&action=prettypatch > WebKit/chromium/public/WebGLES2Context.h:65 > + virtual void resizeOnscreenContent(const WebSize&) = 0; nit: the first part of the comment is redundant with the use of the __APPLE__ macro. one thing that would be helpful to know is whether this method does its work synchronously or not. i notice that the call to this function happens before we have actually generated new pixels. has consideration been given to how we will synchronize "re-compositing" and "re-sizing" so that if things are fast that resizing will be smooth without tearing along the edges?
Comment on attachment 63396 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=63396&action=prettypatch > WebKit/chromium/public/WebGLES2Context.h:65 > + virtual void resizeOnscreenContent(const WebSize&) = 0; This method does its work asynchronously with respect to the renderer, but serially with respect to other graphics calls issued from the renderer. It is required that it runs before we perform the next render cycle because that render occurs into the backing store which is reallocated by this method. More work will be needed to make resizing work well on Mac OS X. See http://crbug.com/53165 .
Comment on attachment 63396 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=63396&action=prettypatch > WebKit/chromium/public/WebGLES2Context.h:65 > + virtual void resizeOnscreenContent(const WebSize&) = 0; OK, that makes sense. My next question: why does our OSX port need to be different in this respect? On Windows at least, I think the resize of the native widget happens before we call WebViewImpl::resize(). Why can't we do the same on OSX? Why does this need to round-trip through WebViewImpl::resize(), which is an API call?
(In reply to comment #12) > (From update of attachment 63396 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=63396&action=prettypatch > > > WebKit/chromium/public/WebGLES2Context.h:65 > > + virtual void resizeOnscreenContent(const WebSize&) = 0; > OK, that makes sense. My next question: why does our OSX port need to be different in this respect? On Windows at least, I think the resize of the native widget happens before we call WebViewImpl::resize(). Why can't we do the same on OSX? Why does this need to round-trip through WebViewImpl::resize(), which is an API call? In the Mac port of the compositor we manage the backing store into which the compositor renders, because there is no such concept as sharing window handles or XIDs across processes. This is the fundamental difference between the Mac port and the Windows/Linux ports and why this must be done with an API call which trampolines over to the GPU process, getting it to reallocate its IOSurface and sending the browser a notification of this fact.