Bug 112605 – [Chromium] Code cleanup: removing support for non-deferred 2d canvas rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Justin Novosad]

[Chromium] Code cleanup: removing support for non-deferred 2d canvas rendering

[Justin Novosad]

Created attachment 193622 [details]
Patch

[James Robinson]

Patch doesn't seem to apply, could you rebase?

Will this flip behavior for layout tests?

[Justin Novosad]

Created attachment 193626 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Justin Novosad]

(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.

[Stephen White]

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.)

[Justin Novosad]

Created attachment 193629 [details]
Patch

[Justin Novosad]

(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.

[James Robinson]

(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?

[Justin Novosad]

(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...

[James Robinson]

(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.

[WebKit Review Bot]

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

[Justin Novosad]

(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?

[James Robinson]

I think it'd be useful to repro the failures and figure out where they are coming from.  What's the diff?

[Justin Novosad]

(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

[James Robinson]

OK - I'm find with marking them as failing in TestExpectations with a link to that bug.

[James Robinson]

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.

[Justin Novosad]

Created attachment 194044 [details]
Patch

[Justin Novosad]

Created attachment 194046 [details]
Patch for landing

[WebKit Review Bot]

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

[Justin Novosad]

Created attachment 194051 [details]
Patch for landing

[WebKit Review Bot]

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

[Justin Novosad]

Created attachment 194055 [details]
Patch for landing

[Justin Novosad]

Created attachment 194057 [details]
Patch for landing

[WebKit Review Bot]

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

[e-mail] 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

[Justin Novosad]

Created attachment 194062 [details]
Patch for landing

[WebKit Review Bot]

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

[e-mail] 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

[Justin Novosad]

Created attachment 194066 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 194066 [details]
Patch for landing

Clearing flags on attachment: 194066

Committed r146351: <http://trac.webkit.org/changeset/146351>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.