Add rendering context and rendering 2D context to OffscreenCanvas

Add rendering context and rendering 2D context to OffscreenCanvas

This patch implements the getContext() function in OffscreenCanvas.
It also implements the skeleton of  OffscreenCanvasRenderingContext2D, an object type
returned by getContext("2d").

BUG=563826
Committed: https://crrev.com/a9fbf504f0677ae17f568914138d7709aa72c126
Cr-Commit-Position: refs/heads/master@{#379740}

[xlai (Olivia), 2016-03-01 22:24:46]

Description was changed from

==========
Add rendering context and rendering 2D context to OffscreenCanvas



BUG=563826
==========

to

==========
Add rendering context and rendering 2D context to OffscreenCanvas



BUG=563826
==========

[xlai (Olivia), 2016-03-01 22:24:46]

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

[xlai (Olivia), 2016-03-01 22:25:59]

Patchset #1 (id:1) has been deleted

[xlai (Olivia), 2016-03-01 22:35:43]

Adding a function getContext to OffscreenCanvas.

In core/, the newly added OffscreenCanvasRenderingContext will share some common
attributes and functions with the existing CanvasRenderingContext. Their common
parent class is CanvasRenderingContextBase.

In modules/, The newly added OffscreenCanvasRenderingContext2D will share common
attributes and functions with CanvasRenderingContext2D. Their common parent
class is the existing BaseRenderingContext2D.

OffscreenCanvasRenderingContextFactory is added in a similar fashion as
CanvasRenderingContextFactory.

Note that OffscreenCanvasRenderingContext2D.idl is added despite a lack of
corresponding item in the WHATWG. I put it as a TODO item in comments.

The testing of getContext function is integrated into one existing layout test
that tests the constructor of OffscreenCanvas on main thread. I tried do the
same integration for another existing layout test on worker but find it quite
inelegant; so I wrote a new layout test for create2Dcontext on worker.

Adding imageBuffer to offscreenCanvas will be in the next patch.

[Justin Novosad, 2016-03-01 23:05:57]

To start, I have a high level comment. Please take care of that first, then
we'll get into details.

https://codereview.chromium.org/1748163003/diff/10023/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContextBase.h
(right):

https://codereview.chromium.org/1748163003/diff/10023/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContextBase.h:14:
class CanvasRenderingContextBase : public
NoBaseWillBeGarbageCollectedFinalized<CanvasRenderingContextBase>, public
ScriptWrappable {
You should remove this class because it is not needed. HTMLCanvasElement can
only have a CanvasRenderingContext and OffscreenCanvas can only have an
OffscreenCanvasRenderingContext.  There is no need to make the two related.
isOffscreenCanvasRenderingContext() is not used anywhere.

[Justin Novosad, 2016-03-02 15:41:50]

Awesome work! Really happy to see this taking form.

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html
(right):

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html:15:
shouldNotThrow("aCanvas.getContext('2d')");
You should test the return value. Something like this:
var ctx;
shouldNotThrow("ctx = aCanvas.getContext('2d')");
shouldBe("'[object OffscreenCanvasRenderingContext2D]'", "toString.call(ctx)");

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/OffscreenCanvas.cpp:44: return
nullptr;
This is correct, but it needs to be tested in a layout test. Should check that
getContext does not throw and returns null.  I know the spec is not written yet,
but let's base ourselved on the canvas element spec for this, for now.

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/OffscreenCanvas.cpp:48: return
m_context.get();
This case needs to be covered in a layout test.

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/OffscreenCanvas.cpp:51: return
nullptr;
This case need to be covered in a layout test.

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/OffscreenCanvas.cpp:56: return
nullptr;
This is not necessary. The return statement below is already going to return
null if m_context is null.

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/OffscreenCanvas.h (right):

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/OffscreenCanvas.h:17: class
CanvasContextCreationAttributes;
The Blink core has no dependency on OffscreenCanvas.  So You should put this in
a new module 'offscreencanvas'

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/canvas/OffscreenCanvasRenderingContext.cpp
(right):

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/OffscreenCanvasRenderingContext.cpp:1:
// Copyright 201666666 The Chromium Authors. All rights reserved.
You fell asleep on the keyboard? Stop working so hard ;-)

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/canvas/OffscreenCanvasRenderingContext.h
(right):

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/OffscreenCanvasRenderingContext.h:10:
class CORE_EXPORT OffscreenCanvasRenderingContext : public
NoBaseWillBeGarbageCollectedFinalized<OffscreenCanvasRenderingContext>, public
ScriptWrappable {
This class should also go in the new 'offscreencanvas' module.

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/OffscreenCanvasRenderingContext.h:19:
ContextImageBitmap = 3,
ImageBitmap context will not be supported with OffscreenCanvas

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/canvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/OffscreenCanvasRenderingContext2D.h:12:
class OffscreenCanvasRenderingContext2D final : public
OffscreenCanvasRenderingContext, public BaseRenderingContext2D {
This should go into yet another module 'offscreencanvas2d'

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/OffscreenCanvasRenderingContext2D.h:39:
bool originClean() const final;
For a future CL, stuff that is shared between OCRC2D and CRC2D should be pushed
up into BaseRenderingContext2D so that we don't have to implement twice.  Ian K
made this stuff virtual because it is not needed for Houdini, but some of these
choices need to be re-visited.  I some cases, we could keep the methods virtual
and to stub them out in PRC2D, but, still have a shared implementation in BRC2D.

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/canvas2d/OffscreenCanvasRenderingContext2D.idl
(right):

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/OffscreenCanvasRenderingContext2D.idl:5:
// TODO: Propose OffscreenCanvasRenderingContext2D to WHATWG spec
Instead of this comment put a link to the unofficial spec, with a note that
official spec is WIP.

[xlai (Olivia), 2016-03-03 22:50:07]

@junov: Move everything to modules/ folder. Would it be better if I create
OWNERS file in the two newly created folders "offscreencanvas" and
"offscreecanvas2d" in modules/ and then add you as an Owner? Of course, for the
sake of landing this patch we need an owner of the modules/ root folder.

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html
(right):

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html:15:
shouldNotThrow("aCanvas.getContext('2d')");
On 2016/03/02 15:41:50, Justin Novosad wrote:
> You should test the return value. Something like this:
> var ctx;
> shouldNotThrow("ctx = aCanvas.getContext('2d')");
> shouldBe("'[object OffscreenCanvasRenderingContext2D]'",
"toString.call(ctx)");

Done.

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/OffscreenCanvas.cpp:44: return
nullptr;
On 2016/03/02 15:41:50, Justin Novosad wrote:
> This is correct, but it needs to be tested in a layout test. Should check that
> getContext does not throw and returns null.  I know the spec is not written
yet,
> but let's base ourselved on the canvas element spec for this, for now.

Done for here and elsewhere.

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/canvas/OffscreenCanvasRenderingContext.h
(right):

https://codereview.chromium.org/1748163003/diff/30001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/OffscreenCanvasRenderingContext.h:19:
ContextImageBitmap = 3,
On 2016/03/02 15:41:50, Justin Novosad wrote:
> ImageBitmap context will not be supported with OffscreenCanvas

Done.

[xlai (Olivia), 2016-03-03 23:04:23]

Description was changed from

==========
Add rendering context and rendering 2D context to OffscreenCanvas



BUG=563826
==========

to

==========
Add rendering context and rendering 2D context to OffscreenCanvas

This patch implements the getContext() function in OffscreenCanvas. It also
implements the skeleton of  OffscreenCanvasRenderingContext2D, an object type
returned by getContext("2d"). 

BUG=563826
==========

[xlai (Olivia), 2016-03-03 23:04:23]

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

[Justin Novosad, 2016-03-04 06:04:45]

After you pointed out that HTMLCanvasElement.getContext() did not conform to the
spec's IDL for the return type, I decided to go and fix it.  See here:
https://codereview.chromium.org/1761003003/

You should follow that pattern here to use a typedef as the return type of
getContext. In the case of this CL, it will be much simpler because everything
is already in modules.

[xlai (Olivia), 2016-03-04 16:11:49]

junov@: I've made use of typedef in OffscreenCanvas.idl; as a result, OCRC.idl
is no longer needed to make the IDL binding between OCRC and OCRC2 works so it's
removed. 

haraken@: We are creating two new subfolders in modules/-- "offscreencanvas",
and "offscreencanvas2d" --and would need your approval on that.

Thanks for reviewing!

[haraken, 2016-03-04 16:22:15]

LGTM. I just reviewed the CL from the implementation perspective. Please wait
for junov's approval.

https://codereview.chromium.org/1748163003/diff/110001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1748163003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp:10:
#define UNIMPLEMENTED ASSERT_NOT_REACHED

Do you want to use platform/NotImplemented.h ?

https://codereview.chromium.org/1748163003/diff/110001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/1748163003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h:21:
WTF_MAKE_NONCOPYABLE(Factory);

This won't be needed since the parent class has the macro.

[xlai (Olivia), 2016-03-06 22:47:36]

Summary of latest changes: Corrections based on Haraken's feedback and Rebase
with master.

https://codereview.chromium.org/1748163003/diff/110001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/1748163003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h:21:
WTF_MAKE_NONCOPYABLE(Factory);
On 2016/03/04 16:22:15, haraken wrote:
> 
> This won't be needed since the parent class has the macro.

Done for here and elsewhere.

[Justin Novosad, 2016-03-07 15:30:27]

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D-in-worker.html
(right):

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D-in-worker.html:32:
switch (msg.data.outcome) {
This test is more complex than necessary. Instead of posting a structure back
from the worker, you should just post a single string that either contains
"success" or an error message. Then this function could just be:
shouldBe('success', msg.data);

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html
(right):

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html:7:
var aCanvas = new OffscreenCanvas(50, 50);
Not new to this CL, but you should also test the edge cases.  What happens if
one of the dimensions is 0 or is of the wrong type?  Same thing for setting
width and height directly.  Also, you should use different values for width and
height, otherwise the test is not verifying that the two aren't inverted.

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html:23:
shouldBeNull("ctx2");
This is good, because this test will still be correct once we start supporting
webgl.

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html:32:
var ctx4 = bCanvas.getContext("webgl");
This test will break the day we support WebGL, might as well use "bogus"
instead.

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp
(right):

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.cpp:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
Not sure why the diff is not tracking this file as a move.  You used "git mv",
right? Perhaps you should re-upload with a lower similarity threshold to help
reitveld pick up on this? Same thing for the other files in this directory.

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h
(right):

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:33:
OffscreenCanvasRenderingContext* getContext(const String&, const
CanvasContextCreationAttributes&);
Until the day we actually use a union, this should return
OffscreenCanvasRenderingContext2D.

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.h:34:
OffscreenCanvasRenderingContext* renderingContext() const { return
m_context.get(); }
Same here.

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.idl
(right):

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.idl:7: typedef
OffscreenCanvasRenderingContext2D OffscreenCanvasRenderingContext;
We should call this enum just "OffscreenRenderingContext" to avoid confusing it
with the class of the same name. FWIW, the enum for canvas elements is just
called "RenderingContext".

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.idl
(right):

https://codereview.chromium.org/1748163003/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.idl:5:
// Note: The official spec is WIP.
Add a URL for spec proposal this is based on.

[xlai (Olivia), 2016-03-07 19:47:57]

Summary of changes: 
- Layout Tests: changed according to feedback
- typedef name change to avoid name clash with existing class
- Upload the patch with similarity threshold of 20 (because with a similarity
threshold of 30, codeReview still can't detect the association between the old
OffscreenCanvas.cpp and the new one).

[Justin Novosad, 2016-03-07 19:59:44]

lgtm

[xlai (Olivia), 2016-03-07 23:41:42]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-03-07 23:41:42]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1748163003/#ps210001
(title: "Upload based on an even lower similarity threshold (20)")

[commit-bot: I haz the power, 2016-03-07 23:41:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748163003/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748163003/210001

[xlai (Olivia), 2016-03-07 23:50:53]

Description was changed from

==========
Add rendering context and rendering 2D context to OffscreenCanvas

This patch implements the getContext() function in OffscreenCanvas. It also
implements the skeleton of  OffscreenCanvasRenderingContext2D, an object type
returned by getContext("2d"). 

BUG=563826
==========

to

==========
Add rendering context and rendering 2D context to OffscreenCanvas

This patch implements the getContext() function in OffscreenCanvas. 
It also implements the skeleton of  OffscreenCanvasRenderingContext2D, an object
type 
returned by getContext("2d"). 

BUG=563826
==========

[commit-bot: I haz the power, 2016-03-08 00:23:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-08 00:23:24]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[xlai (Olivia), 2016-03-08 01:05:00]

Patchset #10 (id:190001) has been deleted

[xlai (Olivia), 2016-03-08 01:42:58]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-03-08 01:42:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1748163003/#ps230001
(title: "Make OffscreenCanvasRenderingContext Modules exportable"")

[commit-bot: I haz the power, 2016-03-08 01:43:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748163003/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748163003/230001

[commit-bot: I haz the power, 2016-03-08 04:41:10]

Description was changed from

==========
Add rendering context and rendering 2D context to OffscreenCanvas

This patch implements the getContext() function in OffscreenCanvas. 
It also implements the skeleton of  OffscreenCanvasRenderingContext2D, an object
type 
returned by getContext("2d"). 

BUG=563826
==========

to

==========
Add rendering context and rendering 2D context to OffscreenCanvas

This patch implements the getContext() function in OffscreenCanvas. 
It also implements the skeleton of  OffscreenCanvasRenderingContext2D, an object
type 
returned by getContext("2d"). 

BUG=563826
==========

[commit-bot: I haz the power, 2016-03-08 04:41:11]

Committed patchset #11 (id:230001)

[commit-bot: I haz the power, 2016-03-08 04:43:43]

Description was changed from

==========
Add rendering context and rendering 2D context to OffscreenCanvas

This patch implements the getContext() function in OffscreenCanvas. 
It also implements the skeleton of  OffscreenCanvasRenderingContext2D, an object
type 
returned by getContext("2d"). 

BUG=563826
==========

to

==========
Add rendering context and rendering 2D context to OffscreenCanvas

This patch implements the getContext() function in OffscreenCanvas.
It also implements the skeleton of  OffscreenCanvasRenderingContext2D, an object
type
returned by getContext("2d").

BUG=563826

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

[commit-bot: I haz the power, 2016-03-08 04:43:45]

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