Add support for -webkit-mask properties in the accelerated compositing in chromium's accelerated compositing path.
Created attachment 74346 [details] proposed patch
Comment on attachment 74346 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=74346&action=review > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:853 > + " vec4 maskColor = texture2D(s_maskTexture, v_texCoord); \n" > + " gl_FragColor = vec4(texColor.x, texColor.y, texColor.z, texColor.w) * alpha * maskColor.w; \n" Are you sure it's a good idea to impose the extra texture fetch and multiply in all cases? Masking doesn't seem to be a very common operation (the compositor seems to have mostly been working fine up to this point) and I'm concerned about making the default shader more expensive since it touches every pixel in the browser window. It seems to me it would be better to have two shaders, one for when masking is enabled and one for when it isn't. Would this impose too much code complexity? It seems it would be pretty self-contained in LayerRendererChromium::drawLayer. > LayoutTests/platform/chromium-gpu/test_expectations.txt:56 > -BUGNONE WIN LINUX : fast/canvas = PASS > -BUGNONE WIN LINUX : canvas/philip = PASS > +//BUGNONE WIN LINUX : fast/canvas = PASS > +//BUGNONE WIN LINUX : canvas/philip = PASS Why are these being commented out?
Created attachment 74429 [details] Patch addressing review comments
(In reply to comment #2) > (From update of attachment 74346 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74346&action=review > > > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:853 > > + " vec4 maskColor = texture2D(s_maskTexture, v_texCoord); \n" > > + " gl_FragColor = vec4(texColor.x, texColor.y, texColor.z, texColor.w) * alpha * maskColor.w; \n" > > Are you sure it's a good idea to impose the extra texture fetch and multiply in all cases? Masking doesn't seem to be a very common operation (the compositor seems to have mostly been working fine up to this point) and I'm concerned about making the default shader more expensive since it touches every pixel in the browser window. It seems to me it would be better to have two shaders, one for when masking is enabled and one for when it isn't. Would this impose too much code complexity? It seems it would be pretty self-contained in LayerRendererChromium::drawLayer. Good point. I created a different shader to be used by RenderSurfaces that do masking. > > > LayoutTests/platform/chromium-gpu/test_expectations.txt:56 > > -BUGNONE WIN LINUX : fast/canvas = PASS > > -BUGNONE WIN LINUX : canvas/philip = PASS > > +//BUGNONE WIN LINUX : fast/canvas = PASS > > +//BUGNONE WIN LINUX : canvas/philip = PASS > > Why are these being commented out? Ooops... They shouldn't be. I reverted the file.
Created attachment 74435 [details] Removed ANGLE workaround which is no longer necessary
Comment on attachment 74435 [details] Removed ANGLE workaround which is no longer necessary View in context: https://bugs.webkit.org/attachment.cgi?id=74435&action=review Looks good. r- because of the needed revert to ChangeLog. Also please consider something different than the ASSERT_NOT_REACHED + working code in LayerChromium::bindContentsTexture(). > WebCore/platform/graphics/chromium/LayerChromium.cpp:523 > + // We don't really expect to call this method for the base layer type. > + ASSERT_NOT_REACHED(); That's pretty gross. > LayoutTests/ChangeLog:8 > +2010-11-18 Vangelis Kokkevis <vangelis@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Need a short description and bug URL (OOPS!) > + > + * platform/chromium-gpu/test_expectations.txt: > + This file needs to be reverted.
Created attachment 74609 [details] Proposed patch - Removed unecessary changelog and ASSERT
Comment on attachment 74609 [details] Proposed patch - Removed unecessary changelog and ASSERT Looks good.
Comment on attachment 74609 [details] Proposed patch - Removed unecessary changelog and ASSERT Rejecting attachment 74609 [details] from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--non-interactive', 74609]" exit_code: 2 Last 500 characters of output: s FAILED -- saving rejects to file WebCore/platform/graphics/chromium/LayerRendererChromium.cpp.rej patching file WebCore/platform/graphics/chromium/LayerRendererChromium.h Hunk #1 succeeded at 143 with fuzz 2 (offset -4 lines). patching file WebCore/platform/graphics/chromium/RenderSurfaceChromium.h Hunk #1 succeeded at 82 with fuzz 2 (offset 27 lines). Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Russell', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/7000112
Created attachment 79808 [details] Patch
(In reply to comment #10) > Created an attachment (id=79808) [details] > Patch Had to rework the previous patch to account for code changes that happened since the original upload, including the introduction of the TextureManager and moving much of the RenderSurface drawing logic into the RenderSurface class. Can you please have another quick look? Thanks!
Attachment 79808 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7503288
Created attachment 79815 [details] Patch
(In reply to comment #13) > Created an attachment (id=79815) [details] > Patch Fixed Linux compilation error
Comment on attachment 79815 [details] Patch Looks fine. (I wish we could see the differences between patches with the review tool...)
Committed r76475: <http://trac.webkit.org/changeset/76475>