[Chromium] Code cleanup: removing support for non-deferred 2d canvas rendering
Created attachment 193622 [details] Patch
Patch doesn't seem to apply, could you rebase? Will this flip behavior for layout tests?
Created attachment 193626 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
(In reply to comment #2) > Patch doesn't seem to apply, could you rebase? > > Will this flip behavior for layout tests? The GPU bots are currently running with deferred 2d canvas rendering enabled, so they will not be affected by this change. The non-GPU layout tests are not affected because deferred rendering is only implemented for GPU-accelerated canvases.
Comment on attachment 193626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193626&action=review > Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:48 > + : m_backBufferTexture(textureId) Nit: We should keep an eye on the name of this member var, and make sure it makes sense (it doesn't really make sense right now, since we only have a single buffer, but Justin tells me more will be reimplemented soon.)
Created attachment 193629 [details] Patch
(In reply to comment #7) > Created an attachment (id=193629) [details] > Patch New Patch adds NULL ptr checks on m_canvas in Canvas2DLayerBridge.cpp to fix crashing unit tests. FWIW: This patch is just clearing the path for a deeper re-arch of Canvas2DLayerBridge to support the Mailboxes for communicating with the ubercompositor. Support for legacy non-deferred 2d canvas rendering was adding a lot of complexity that does not need to be a part of the upcoming re-design.
(In reply to comment #5) > (In reply to comment #2) > > Patch doesn't seem to apply, could you rebase? > > > > Will this flip behavior for layout tests? > > The GPU bots are currently running with deferred 2d canvas rendering enabled, so they will not be affected by this change. The non-GPU layout tests are not affected because deferred rendering is only implemented for GPU-accelerated canvases. All of the layout test bots run the platform/chromium/virtual/gpu/(fast/canvas|canvas/philip) tests through the GPU path (backed by osmesa). Will they change?
(In reply to comment #9) > (In reply to comment #5) > > (In reply to comment #2) > > > Patch doesn't seem to apply, could you rebase? > > > > > > Will this flip behavior for layout tests? > > > > The GPU bots are currently running with deferred 2d canvas rendering enabled, so they will not be affected by this change. The non-GPU layout tests are not affected because deferred rendering is only implemented for GPU-accelerated canvases. > > All of the layout test bots run the platform/chromium/virtual/gpu/(fast/canvas|canvas/philip) tests through the GPU path (backed by osmesa). Will they change? I did not think they would change, but apparently this patch changes them because the chromium ews bots are detecting regression with this patch that went undetected until now. Not sure how come we are in this state. Investigating...
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #5) > > > (In reply to comment #2) > > > > Patch doesn't seem to apply, could you rebase? > > > > > > > > Will this flip behavior for layout tests? > > > > > > The GPU bots are currently running with deferred 2d canvas rendering enabled, so they will not be affected by this change. The non-GPU layout tests are not affected because deferred rendering is only implemented for GPU-accelerated canvases. > > > > All of the layout test bots run the platform/chromium/virtual/gpu/(fast/canvas|canvas/philip) tests through the GPU path (backed by osmesa). Will they change? > > > I did not think they would change, but apparently this patch changes them because the chromium ews bots are detecting regression with this patch that went undetected until now. Not sure how come we are in this state. Investigating... It sounds from what you're saying that they previous ran without deferred canvas but now are running with it. I think running with the deferred mode enabled is better, since that's what we are shipping, but it's a difference.
Comment on attachment 193629 [details] Patch Attachment 193629 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17160577 New failing tests: platform/chromium/virtual/gpu/canvas/philip/tests/2d.gradient.radial.cone.beside.html platform/chromium/virtual/gpu/canvas/philip/tests/2d.gradient.radial.cone.shape1.html platform/chromium/virtual/gpu/canvas/philip/tests/2d.gradient.radial.cone.behind.html platform/chromium/virtual/gpu/canvas/philip/tests/2d.gradient.radial.cone.shape2.html platform/chromium/virtual/gpu/canvas/philip/tests/2d.gradient.radial.touch3.html platform/chromium/virtual/gpu/canvas/philip/tests/2d.gradient.radial.touch1.html
(In reply to comment #11) > > It sounds from what you're saying that they previous ran without deferred canvas but now are running with it. I think running with the deferred mode enabled is better, since that's what we are shipping, but it's a difference. Exactly. After taking a look at content/shell/webkit_test_controller.cc (in chromium), it appears that we were only enabling deferred mode for tests in the "compositing/" directory. And this has only been the case since 2 months ago: http://src.chromium.org/viewvc/chrome?view=rev&revision=177509 I had a patch to enable it by default in DRT. I thought I submitted it, but it looks like it fell through the cracks: https://bugs.webkit.org/show_bug.cgi?id=92525 The failing LayoutTests are not failing in the browser. Only in DRT. Not sure what is happening. Perhaps skia is generating shader code that is not compatible with mesa for the radial gradient effect? I suggest submitting this CL with the new failures flagged in TestExpectations, and I'll file a separate bug for the newly detected failures. Does that sound right?
I think it'd be useful to repro the failures and figure out where they are coming from. What's the diff?
(In reply to comment #14) > I think it'd be useful to repro the failures and figure out where they are coming from. What's the diff? Bug isolated. Looks like the regression came from skia in the 5594-5633 range Filed a bug: https://code.google.com/p/chromium/issues/detail?id=222140
OK - I'm find with marking them as failing in TestExpectations with a link to that bug.
Comment on attachment 193629 [details] Patch R=me, but be sure to update TestExpectations before landing. You may want to ask Stephen to take a look as well.
Created attachment 194044 [details] Patch
Created attachment 194046 [details] Patch for landing
Comment on attachment 194046 [details] Patch for landing Rejecting attachment 194046 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 194046, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: nderTree/chromium/TestShell.cpp.rej patching file Tools/DumpRenderTree/chromium/TestShell.h Hunk #1 FAILED at 121. Hunk #2 FAILED at 218. 2 out of 2 hunks FAILED -- saving rejects to file Tools/DumpRenderTree/chromium/TestShell.h.rej patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/platform/chromium/TestExpectations Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://webkit-commit-queue.appspot.com/results/17223377
Created attachment 194051 [details] Patch for landing
Comment on attachment 194051 [details] Patch for landing Rejecting attachment 194051 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'validate-changelog', '--non-interactive', 194051, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-commit-queue.appspot.com/results/17122785
Created attachment 194055 [details] Patch for landing
Created attachment 194057 [details] Patch for landing
Comment on attachment 194057 [details] Patch for landing Rejecting attachment 194057 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'validate-changelog', '--non-interactive', 194057, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue jamesr@chromium.org found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py. /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-commit-queue.appspot.com/results/17286012
Created attachment 194062 [details] Patch for landing
Comment on attachment 194062 [details] Patch for landing Rejecting attachment 194062 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-01', 'validate-changelog', '--non-interactive', 194062, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue jamesr@chromium.org found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py. /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-commit-queue.appspot.com/results/17200467
Created attachment 194066 [details] Patch for landing
Comment on attachment 194066 [details] Patch for landing Clearing flags on attachment: 194066 Committed r146351: <http://trac.webkit.org/changeset/146351>
All reviewed patches have been landed. Closing bug.