Allow RecordingImageBufferSurface to prevent fallback. (issue 2840093002 by danakj@chromium.org)

dan...@chromium.org

unread,

Apr 25, 2017, 5:59:32 PM4/25/17

to ikilp...@chromium.org, p...@chromium.org, chromium...@chromium.org, dsch...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, blink-reviews-p...@chromium.org, ajuma+wat...@chromium.org, blink-rev...@chromium.org, pdr+graphi...@chromium.org, ju...@chromium.org, har...@chromium.org, dglazko...@chromium.org, caba...@adobe.com, fmalit...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, ajuma...@chromium.org, sche...@chromium.org, rob....@samsung.com

Reviewers: ikilpatrick, pdr.
CL: https://codereview.chromium.org/2840093002/

Message:
pdr: overall
ikilpatrick: csspaint

Description:
Allow RecordingImageBufferSurface to prevent fallback.

When it will only be used for a single frame, there is no need to
fallback for complex frames. This restores behaviour for CSSPaint
module to before https://codereview.chromium.org/2833593002/.

R=ikilp...@chromium.org, p...@chromium.org
BUG=671433

Affected files (+56, -46 lines):
M third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
M third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
M third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp
M third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
M third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.h
M third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp
M third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurfaceTest.cpp

dan...@chromium.org

unread,

Apr 25, 2017, 6:02:20 PM4/25/17

to ikilp...@chromium.org, p...@chromium.org, senor...@chromium.org, chromium...@chromium.org, dsch...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, blink-reviews-p...@chromium.org, ajuma+wat...@chromium.org, blink-rev...@chromium.org, pdr+graphi...@chromium.org, ju...@chromium.org, har...@chromium.org, dglazko...@chromium.org, caba...@adobe.com, fmalit...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, ajuma...@chromium.org, sche...@chromium.org, rob....@samsung.com

+senorblanco for canvas2d (will tbr)

https://codereview.chromium.org/2840093002/

p...@chromium.org

unread,

Apr 25, 2017, 7:00:27 PM4/25/17

to danakj...@chromium.org, ikilp...@chromium.org, senor...@chromium.org, ajuma...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dglazko...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fmalit...@chromium.org, har...@chromium.org, ju...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

https://codereview.chromium.org/2840093002/diff/1/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2840093002/diff/1/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp#newcode956
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:956: Size(),
RecordingImageBufferSurface::kAllowFallback, opacity_mode,
Should UnacceleratedImageBufferSurface also take an allow fallback
parameter? Do we need to allow fallback for the
UnacceleratedImageBufferSurface surface created a few lines below?

https://codereview.chromium.org/2840093002/

ju...@chromium.org

unread,

Apr 26, 2017, 10:25:25 AM4/26/17

to danakj...@chromium.org, p...@chromium.org, ikilp...@chromium.org, senor...@chromium.org, ajuma...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dglazko...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fmalit...@chromium.org, har...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

lgtm with build fixed.

https://codereview.chromium.org/2840093002/diff/1/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2840093002/diff/1/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp#newcode956
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:956: Size(),
RecordingImageBufferSurface::kAllowFallback, opacity_mode,

On 2017/04/25 23:00:27, pdr. wrote:
> Should UnacceleratedImageBufferSurface also take an allow fallback
parameter? Do
> we need to allow fallback for the UnacceleratedImageBufferSurface
surface
> created a few lines below?

No, there is no fallback functionality in
UnacceleratedImageBufferSurface

https://codereview.chromium.org/2840093002/diff/1/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
(right):

https://codereview.chromium.org/2840093002/diff/1/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp#newcode551
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:551:
WTF::MakeUnique<RecordingImageBufferSurface>(IntSize(10, 10),
kNonOpaque);
Missed this guy.

https://codereview.chromium.org/2840093002/

ikilp...@chromium.org

unread,

Apr 26, 2017, 4:36:05 PM4/26/17

to danakj...@chromium.org, p...@chromium.org, senor...@chromium.org, ju...@chromium.org, ajuma...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dglazko...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fmalit...@chromium.org, har...@chromium.org, ju...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

lgtm, thanks.

https://codereview.chromium.org/2840093002/

p...@chromium.org

unread,

Apr 26, 2017, 4:48:08 PM4/26/17

to danakj...@chromium.org, ikilp...@chromium.org, senor...@chromium.org, ju...@chromium.org, ajuma...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dglazko...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fmalit...@chromium.org, har...@chromium.org, ju...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

LGTM based on junov's response.

https://codereview.chromium.org/2840093002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Apr 26, 2017, 6:10:49 PM4/26/17

to danakj...@chromium.org, p...@chromium.org, ikilp...@chromium.org, senor...@chromium.org, ju...@chromium.org, commi...@chromium.org, ajuma...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dglazko...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fmalit...@chromium.org, har...@chromium.org, ju...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2840093002/20001

https://codereview.chromium.org/2840093002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Apr 26, 2017, 8:04:19 PM4/26/17

to danakj...@chromium.org, p...@chromium.org, ikilp...@chromium.org, senor...@chromium.org, ju...@chromium.org, commi...@chromium.org, ajuma...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dglazko...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fmalit...@chromium.org, har...@chromium.org, ju...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

Try jobs failed on following builders:
linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/440017)

https://codereview.chromium.org/2840093002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Apr 27, 2017, 4:43:59 PM4/27/17

to danakj...@chromium.org, p...@chromium.org, ikilp...@chromium.org, senor...@chromium.org, ju...@chromium.org, commi...@chromium.org, ajuma...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dglazko...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fmalit...@chromium.org, har...@chromium.org, ju...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,

Apr 27, 2017, 10:17:22 PM4/27/17

to danakj...@chromium.org, p...@chromium.org, ikilp...@chromium.org, senor...@chromium.org, ju...@chromium.org, commi...@chromium.org, ajuma...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dglazko...@chromium.org, dongseo...@intel.com, drott+bl...@chromium.org, dsch...@chromium.org, fmalit...@chromium.org, har...@chromium.org, ju...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, rob....@samsung.com, sche...@chromium.org

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/37e6e9d2d2685dc9a6382349bd06a21837663ed2

https://codereview.chromium.org/2840093002/

Reply all

Reply to author

Forward