Implement Canvas API #380

delima02 · 2019-03-21T22:01:53Z

Implementation of the Canvas API in the worker thread.
This will work for Chrome and FireFox (the latter if the user explicitly enables OffscreenCanvas).
04/16/2019: OffscreenCanvas Polyfill has been added to this PR. Most CanvasRenderingContext2D methods will now work on all browsers.

@choumx @jridgewell

This reverts commit ca5b033.

kristoferbaxter · 2019-03-22T04:39:01Z

Going to request this change holds off until the buffer branch lands since it will change the messaging format.

I'll review the changes tomorrow.

src/test/htmlcanvaselement/context.ts

src/test/tsconfig.json

src/test/htmlcanvaselement/context.ts

…alls

…for each fn

delima02 · 2019-03-30T15:31:58Z

@jridgewell ready for review 👍

dreamofabear

Is there a test page I can try this code out on? I'm not quite sure I follow the control flow.

package.json

src/worker-thread/dom/HTMLCanvasElement.ts

src/worker-thread/CanvasRenderingContext2D.ts

dreamofabear · 2019-05-01T16:15:38Z

src/main-thread/worker.ts

    if (DEBUG_ENABLED) {
      console.info('debug', 'messageToWorker', readableMessageToWorker(this.nodeContext, message));
    }
    if (this.config.onSendMessage) {
      this.config.onSendMessage(message);
    }
-    this.worker.postMessage(message);
+    this.worker.postMessage(message, transferables);


Should this be an optional parameter instead of forcing all messages without transferables to pass an empty array?

src/test/htmlcanvaselement/context.test.ts

dreamofabear · 2019-05-01T16:28:56Z

src/test/htmlcanvaselement/context.test.ts

+});
+
+describe('clearRect', () => {
+  test('context calls clearRect', t => {


Using a fake describe() is mixing testing paradigms. Better to just use comment blocks and stick to Ava style.

Discussion thread if you're curious: avajs/ava#222

dreamofabear · 2019-05-01T16:30:01Z

src/test/htmlcanvaselement/context.test.ts

+  });
+});
+
+describe('rota


s/spy/setter

A "spy" isn't the right name to describe this function.

dreamofabear · 2019-05-01T16:30:12Z

src/test/htmlcanvaselement/context.test.ts

+  });
+});
+
+describe('rota


src/worker-thread/CanvasRenderingContext2D.ts

dreamofabear · 2019-05-01T17:29:12Z

src/main-thread/main-thread.d.ts

@@ -20,6 +20,10 @@ declare interface Sanitizer {
  mutateProperty(node: Node, prop: string, value: string): boolean;
 }

+declare interface HTMLCanvasElement {


Add a comment explaining why we need to declare this here. I assume it's because OffscreenCanvas types is not available in TS yet?

Correct.transferControlToOffscreen is not detected as a method because of this.

dreamofabear · 2019-05-01T18:06:03Z

src/worker-thread/CanvasRenderingContext2D.ts

+      }
+    };
+
+    // TODO: This should only happen in test environemnet. Otherwise, we should throw.


Fix and remove TODOs.

Will include this in a later PR (there is not a simple way to do this currently, and it does not affect the operation of this API outside of testing environment)

dreamofabear · 2019-05-01T18:07:03Z

src/worker-thread/CanvasRenderingContext2D.ts

+    this.delegate('restore', [...arguments], false);
+  }
+
+  // canvas property is readonly. We don't want to implement getters, but this must be here


We don't want to implement getters

What does this mean?

Nvm, getters are included now.

src/worker-thread/DOMTypes.ts

dreamofabear · 2019-05-01T18:14:48Z

src/main-thread/commands/offscreen-polyfill-calls.ts

+
+export function OffscreenPolyfillCallProcessor(strings: Strings, workerContext: WorkerContext): CommandExecutor {
+  return {
+    execute(mutations: Uint16Array, startPosition: number, target: RenderableElement): number {


Do we have tests that cover these processor functions?

Now we do 👍

… delima02-canvas

src/test/htmlcanvaselement/context.test.ts

dreamofabear · 2019-05-06T19:37:32Z

src/test/htmlcanvaselement/context.test.ts

+    return this.implementation;
+  }
+}
+(global as any).OffscreenCanvas = OffscreenCanvas;


Ruh roh. Globals are generally bad, especially this global since it includes the test runner.

Does this new approach seem like a good idea?

dreamofabear · 2019-05-06T19:39:41Z

src/test/htmlcanvaselement/context.test.ts

+  constructor() {
+    // this.x, y
+    const context = ({} as unknown) as CanvasRenderingContext2D;
+    OffscreenCanvas.mostRecentInstance = this;


Does this look weird to you? :)

Also, do we need it? Why can't this be referenced via canvas.getContext('2d').implementation?

From our discussion yesterday: this is actually needed for calls that happen before the upgraded (auto-synchronized) version of OffscreenCanvas is fetched from the main-thread.

src/test/htmlcanvaselement/context.test.ts

src/worker-thread/canvas/CanvasRenderingContext2D.ts

…ntext2D.

…nt ones

…xt2DShim

dreamofabear

Make sure that any code review comments are applied across all instances in your PR e.g. using globals. Comments can be addressed after merge.

src/test/htmlcanvaselement/context.test.ts

dreamofabear · 2019-05-08T22:14:56Z

src/test/htmlcanvaselement/context.test.ts

+  constructor() {
+    // this.x, y
+    const context = ({} as unknown) as CanvasRenderingContext2D;
+    FakeOffscreenCanvas.mostRecentInstance = this;


This is still an issue. Remember what we discussed was the possible solution for this?

src/test/DocumentCreation.ts

src/test/htmlcanvaselement/reflectedProperties.test.ts

dreamofabear · 2019-05-08T22:16:34Z

src/test/main-thread/offscreen-canvas.test.ts

+function createSetterStub(obj: any, property: string, spy: () => {}) {
+  obj[property] = 'existent';
+  sandbox.stub(obj, property).set(spy);
+}


src/worker-thread/canvas/OffscreenCanvasPolyfill.ts

dreamofabear · 2019-05-08T22:22:53Z

src/test/htmlcanvaselement/context.test.ts

+import { CanvasRenderingContext2D } from '../../worker-thread/canvas/CanvasTypes';
+import { createTestingDocument } from '../DocumentCreation';
+
+const test = anyTest as TestInterface<{


This test is 3K lines of code. We should first remove redundant tests (no need to test same upgrade code path more than once) and then split it up if necessary (how should we segment it?).

dreamofabear · 2019-05-08T22:36:02Z

src/test/htmlcanvaselement/context.test.ts

+  const { context2d, deferredUpgrade, implementation, sandbox } = t.context;
+  const instance = new FakeOffscreenCanvas();
+
+  const instanceStub = (instance.getContext('2d')['clearRect'] = sandbox.stub());


I don't understand how this local FakeOffscreenCanvas instance is at all related to the <canvas> element created in beforeEach.

src/test/main-thread/offscreen-canvas.test.ts

delima02 added 3 commits March 20, 2019 11:05

Merge master

ca5b033

Revert "Merge master"

9daba15

This reverts commit ca5b033.

CanvasRenderingContext2D and OffscreenCanvas

dfebb96

googlebot added the cla: yes label Mar 21, 2019

kristoferbaxter self-requested a review March 22, 2019 04:37

Add tests for CanvasRenderingContext2D

d810e14

jridgewell requested changes Mar 27, 2019

View reviewed changes

delima02 added 15 commits March 27, 2019 16:25

Remove unnecessary comments

3f20f3e

Revert allowing noImplicitAny for tests

237e34c

Remove JS files from src/test/tsconfig

797b3cb

Remove unnecessary excluded directory from test/tsconfig

3e625be

Only test what should be in second and third tests for clearRect

fca3af6

Fix implementation and tests for beginPath and closePath

b3d0a32

Change static context to most recent OffscreenCanvas instance in tests

95d3a14

Add meta-programming to reduce the number of lines while delegating c…

ad8e8bb

…alls

Add missing tests for putImageData

ade3271

Simplify ImageBitmap creation logic for tests

60e1f81

Merge conflicts with master

74bb308

Await for same promise instance needed by test, instead of new one

5fd3614

Remove unnecessary check from third test for each method tested

3f84ccd

Fix test purposes by moving assertion from third test to second test …

fe54ac6

…for each fn

Add missing tests for createPattern, drawImage, and putImageData

36f9696

delima02 changed the title ~~Implement Canvas API for browsers that support OffscreenCanvas~~ [WIP] Implement Canvas API for browsers that support OffscreenCanvas Mar 29, 2019

Remove object from delegate function definition arguments

8f5eb73

delima02 changed the title ~~[WIP] Implement Canvas API for browsers that support OffscreenCanvas~~ Implement Canvas API for browsers that support OffscreenCanvas Mar 30, 2019

dreamofabear reviewed Apr 1, 2019

View reviewed changes

Have HTMLCanvasElement actually own the context

584cba3

delima02 changed the title ~~Implement Canvas API for browsers that support OffscreenCanvas~~ [wip] Implement Canvas API for browsers that support OffscreenCanvas Apr 1, 2019

dreamofabear reviewed May 1, 2019

View reviewed changes

delima02 added 13 commits May 1, 2019 14:34

Make 'transferables' an optional argument in messageToWorker

7730c80

Indicate OffscreenCanvas not available in TS with comment

b5c0dc6

Rename DOMTypes and group all canvas files in folder

1e7b7f9

Add getters for CanvasRenderingContext2D

ce20391

Replace fake 'describe' blocks with comments

4a3c909

Missing getters and tests for getters.

67ce3b2

Rename "spy" to something more appropriate in testing helper fn.

c39a38e

Remove unneeded 'workerContext' from offscreen call processor.

09e0dbf

Tests for offscreen call processor.

3a4edf4

Merge branch 'delima02-canvas' of github.com:delima02/worker-dom into…

d1e58bd

… delima02-canvas

Increase bundle size.

1afb6ed

Merge branch 'master' into delima02-canvas

17cabfd

Increase bundle size

f0d1b3b

dreamofabear reviewed May 6, 2019

View reviewed changes

delima02 added 11 commits May 6, 2019 17:28

Have 'sandbox' be passed through test context.

a0e3276

Rename some variables.

a105d69

Make 'getOffscreenCanvasAsync' private and owned by CanvasRenderingCo…

120eec2

…ntext2D.

Split CanvasRenderingContext2D's delegate function into three differe…

37204db

…nt ones

Rename test OffscreenCanvas class to FakeOffscreenCanvas

5bb059c

Rename CanvasRenderingContext2DImplementation to CanvasRenderingConte…

283dbec

…xt2DShim

Create test stubs inline.

cae19fd

merge master

79ebff9

Remove global dependency from Canvas API

65b2c32

Fix demo.

3d8ce3d

Increase bundle size.

16a1a64

dreamofabear approved these changes May 8, 2019

View reviewed changes

dreamofabear merged commit 443f62e into ampproject:master May 9, 2019

dreamofabear mentioned this pull request Jul 23, 2019

Canvas support #255

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Canvas API #380

Implement Canvas API #380

delima02 commented Mar 21, 2019 •

edited

Loading

kristoferbaxter commented Mar 22, 2019

delima02 commented Mar 30, 2019

dreamofabear left a comment

dreamofabear May 1, 2019

delima02 May 2, 2019

dreamofabear May 1, 2019

delima02 May 6, 2019

dreamofabear May 1, 2019

dreamofabear May 1, 2019

dreamofabear May 1, 2019

delima02 May 2, 2019

dreamofabear May 1, 2019

delima02 May 6, 2019

dreamofabear May 1, 2019

delima02 May 2, 2019

dreamofabear May 1, 2019

delima02 May 6, 2019

dreamofabear May 6, 2019

delima02 May 8, 2019

dreamofabear May 6, 2019

delima02 May 8, 2019

dreamofabear left a comment

dreamofabear May 8, 2019

dreamofabear May 8, 2019

dreamofabear May 8, 2019

dreamofabear May 8, 2019

Implement Canvas API #380

Implement Canvas API #380

Conversation

delima02 commented Mar 21, 2019 • edited Loading

kristoferbaxter commented Mar 22, 2019

delima02 commented Mar 30, 2019

dreamofabear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delima02 commented Mar 21, 2019 •

edited

Loading