Determine if OffscreenCanvas is paintable in different rendering contexts

Determine if OffscreenCanvas is paintable in different rendering contexts

This CL adds isPaintable() to OffscreenCanvas which mimics HTMLCanvasElement::
isPaintable() with some adjustments. Certain common constants and utility
functions shared by OffscreenCanvas and HTMLCanvasElement have been moved to
platform/graphics/. This CL is to facilitate toBlob implementation.

BUG=563830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/aaa6eca6791478e854193a8da307449cd6af0256
Cr-Commit-Position: refs/heads/master@{#413450}

[xlai (Olivia), 2016-08-18 22:40:54]

Description was changed from

==========
Determine if OffscreenCanvas is paintable in different rendering contexts

This CL adds isPaintable() to OffscreenCanvas which mimics HTMLCanvasElement::
isPaintable() with some adjustments. Certain common constants and utility
functions shared by OffscreenCanvas and HTMLCanvasElement have been moved to
platform/graphics/. This CL is to facilitate toBlob implementation.

BUG=563830
==========

to

==========
Determine if OffscreenCanvas is paintable in different rendering contexts

This CL adds isPaintable() to OffscreenCanvas which mimics HTMLCanvasElement::
isPaintable() with some adjustments. Certain common constants and utility
functions shared by OffscreenCanvas and HTMLCanvasElement have been moved to
platform/graphics/. This CL is to facilitate toBlob implementation.

BUG=563830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[xlai (Olivia), 2016-08-18 22:42:34]

xlai@chromium.org changed reviewers:
+ junov@chromium.org

[xlai (Olivia), 2016-08-18 22:42:35]

PTAL. I'm a bit hesitant in putting the imagebitmaprenderer context here.

[Justin Novosad, 2016-08-19 14:55:48]

https://codereview.chromium.org/2255683005/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvasModules.cpp
(right):

https://codereview.chromium.org/2255683005/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvasModules.cpp:28:
bool OffscreenCanvasModules::isPaintable(OffscreenCanvas& offscreenCanvas)
There is a cleaner way to implement this: put a pure virtual method
isPaintable() on CanvasRenderingContext.  It will allow you to keep
drawingBuffer() encapsulated in WebGLRenderingContextBase, and it will allow you
to implement isPaintable in core (in OffscreenCanvas proper), and it avoid
sticking explicit knowledge of all the context types in OffscreenCAnvas, which
is better for maintainability.

[xlai (Olivia), 2016-08-19 15:41:16]

Redo to ensure encapsulation. Note that isPaintable() is public in
CanvasRenderingContext so that OffscreenCanvas can call it. But its overriden by
private isPaintable() in the 3 context subclasses.

[Justin Novosad, 2016-08-19 16:49:39]

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:85: virtual
bool isPaintable() const { NOTREACHED(); return false; }
could you make this pure virtual instead of using NOTREACHED?

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.h
(right):

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.h:56:
bool isPaintable() const override { return m_image.get(); }
override -> final

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h:86:
bool isPaintable() const override;
override -> final

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h
(right):

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1121: bool
isPaintable() const override { return drawingBuffer(); }
override -> final

[xlai (Olivia), 2016-08-19 17:11:33]

xlai@chromium.org changed reviewers:
+ kbr@chromium.org

[xlai (Olivia), 2016-08-19 17:11:34]

+kbr to review changes in WebglRenderingContextBase.

Fix the parts commented by junov@.

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:85: virtual
bool isPaintable() const { NOTREACHED(); return false; }
On 2016/08/19 16:49:39, Justin Novosad wrote:
> could you make this pure virtual instead of using NOTREACHED?

Done.

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.h
(right):

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.h:56:
bool isPaintable() const override { return m_image.get(); }
On 2016/08/19 16:49:39, Justin Novosad wrote:
> override -> final

Done.

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h:86:
bool isPaintable() const override;
On 2016/08/19 16:49:39, Justin Novosad wrote:
> override -> final

Done.

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h
(right):

https://codereview.chromium.org/2255683005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1121: bool
isPaintable() const override { return drawingBuffer(); }
On 2016/08/19 16:49:39, Justin Novosad wrote:
> override -> final

Done.

[Justin Novosad, 2016-08-19 17:30:01]

https://codereview.chromium.org/2255683005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h (right):

https://codereview.chromium.org/2255683005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h:56: const int
PLATFORM_EXPORT MaxCanvasArea = 32768 * 8192; // Maximum canvas area in CSS
pixels
Defining a const in a header can lead to symbol clashes, we use "extern" to
avois this.  But that is moot anyways: the style guide encourages the use of
enums for integer constants.  And we now us a "k" prefix.  Just use an anonymous
enum, like this.

enum {
    kMaxCanvasArea = 32768 * 8192,
    kMaxSkiaDim = 32767
}

[xlai (Olivia), 2016-08-19 17:42:03]

https://codereview.chromium.org/2255683005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h (right):

https://codereview.chromium.org/2255683005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/skia/SkiaUtils.h:56: const int
PLATFORM_EXPORT MaxCanvasArea = 32768 * 8192; // Maximum canvas area in CSS
pixels
On 2016/08/19 17:30:01, Justin Novosad wrote:
> Defining a const in a header can lead to symbol clashes, we use "extern" to
> avois this.  But that is moot anyways: the style guide encourages the use of
> enums for integer constants.  And we now us a "k" prefix.  Just use an
anonymous
> enum, like this.
> 
> enum {
>     kMaxCanvasArea = 32768 * 8192,
>     kMaxSkiaDim = 32767
> }

Done.

[Ken Russell (switch to Gerrit), 2016-08-19 20:31:01]

LGTM in general but one comment.

I assume tests will be added when follow-on CLs land?

https://codereview.chromium.org/2255683005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/2255683005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:116: return
ImageBuffer::canCreateImageBuffer(m_size);
How reliable is this query, and how much does that matter?

This might return true but fail to allocate the image buffer for several
reasons:

 - It tries to allocate an OpenGL texture but the canvas's width or height is
over the OpenGL maximum texture limit (I'm not sure whether this can happen or
whether e.g. Skia would fall back to software in this case)
 - It might run out of CPU-side memory if the ImageBuffer's backing store is
allocated there
 - More?

[Justin Novosad, 2016-08-19 21:06:34]

On 2016/08/19 20:31:01, Ken Russell wrote:
> LGTM in general but one comment.
> 
> I assume tests will be added when follow-on CLs land?
> 
>
https://codereview.chromium.org/2255683005/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp
(right):
> 
>
https://codereview.chromium.org/2255683005/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:116: return
> ImageBuffer::canCreateImageBuffer(m_size);
> How reliable is this query, and how much does that matter?
> 
> This might return true but fail to allocate the image buffer for several
> reasons:
> 
>  - It tries to allocate an OpenGL texture but the canvas's width or height is
> over the OpenGL maximum texture limit (I'm not sure whether this can happen or
> whether e.g. Skia would fall back to software in this case)
>  - It might run out of CPU-side memory if the ImageBuffer's backing store is
> allocated there
>  - More?

Yep, when allocation fails, we end-up silently failing later on. 
HTMLCanvasElement has had similar logic for a while and it been Okay.


lgtm

[xlai (Olivia), 2016-08-19 22:17:54]

On 2016/08/19 20:31:01, Ken Russell wrote:
> LGTM in general but one comment.
> 
> I assume tests will be added when follow-on CLs land?
> 

Yes, this is part of toBlob implementation. I guess the first test
may come only after the whole implementation takes shape.
To avoid dead code being staying in codebase for too long,
I can delay this CL for a while and continue to write
 dependent patches on it and eventually lump all CLs in once,
though that might cause some rebasing pain.

[Ken Russell (switch to Gerrit), 2016-08-19 22:18:40]

On 2016/08/19 22:17:54, xlai (Olivia) wrote:
> On 2016/08/19 20:31:01, Ken Russell wrote:
> > LGTM in general but one comment.
> > 
> > I assume tests will be added when follow-on CLs land?
> > 
> 
> Yes, this is part of toBlob implementation. I guess the first test
> may come only after the whole implementation takes shape.
> To avoid dead code being staying in codebase for too long,
> I can delay this CL for a while and continue to write
>  dependent patches on it and eventually lump all CLs in once,
> though that might cause some rebasing pain.

It seems fine to me to land this now.

[xlai (Olivia), 2016-08-22 14:15:50]

The CQ bit was checked by xlai@chromium.org

[commit-bot: I haz the power, 2016-08-22 14:17:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 16:04:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-22 16:05:53]

Description was changed from

==========
Determine if OffscreenCanvas is paintable in different rendering contexts

This CL adds isPaintable() to OffscreenCanvas which mimics HTMLCanvasElement::
isPaintable() with some adjustments. Certain common constants and utility
functions shared by OffscreenCanvas and HTMLCanvasElement have been moved to
platform/graphics/. This CL is to facilitate toBlob implementation.

BUG=563830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Determine if OffscreenCanvas is paintable in different rendering contexts

This CL adds isPaintable() to OffscreenCanvas which mimics HTMLCanvasElement::
isPaintable() with some adjustments. Certain common constants and utility
functions shared by OffscreenCanvas and HTMLCanvasElement have been moved to
platform/graphics/. This CL is to facilitate toBlob implementation.

BUG=563830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/aaa6eca6791478e854193a8da307449cd6af0256
Cr-Commit-Position: refs/heads/master@{#413450}
==========

[commit-bot: I haz the power, 2016-08-22 16:05:54]

Patchset 5 (id:??) landed as
https://crrev.com/aaa6eca6791478e854193a8da307449cd6af0256
Cr-Commit-Position: refs/heads/master@{#413450}