RESOLVED FIXED 219088
Enable font functions on OffscreenCanvas for main-thread
https://bugs.webkit.org/show_bug.cgi?id=219088
Summary Enable font functions on OffscreenCanvas for main-thread
Chris Lord
Reported 2020-11-18 08:07:17 PST
There is still a lot of work to do (I think) to make text usable on 2d contexts for OffscreenCanvas in a Worker, but that shouldn't stop us from enabling those functions for use on the main thread. This bug tracks enabling the functions on the main thread only.
Attachments
Patch (122.65 KB, patch)
2020-11-19 05:06 PST, Chris Lord
no flags
Patch (122.64 KB, patch)
2020-11-20 01:38 PST, Chris Lord
no flags
Chris Lord
Comment 1 2020-11-19 05:06:40 PST
Simon Fraser (smfr)
Comment 2 2020-11-19 09:54:12 PST
Comment on attachment 414566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414566&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2255 > +String CanvasRenderingContext2DBase::font() const This should be called fontStyle or something. It doesn't return a Font* > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2484 > +#if USE(CG) Sad to see #if USE(CG) here. > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:384 > + // Therefore, all font operations must pass through the State. Not clear from this comment what "the State" refers to.
Darin Adler
Comment 3 2020-11-19 11:17:52 PST
Comment on attachment 414566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414566&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2255 >> +String CanvasRenderingContext2DBase::font() const > > This should be called fontStyle or something. It doesn't return a Font* The name font here comes from the HTML specification of CanvasTextDrawingStyles <https://html.spec.whatwg.org/#dom-context-2d-font>. So it needs to be "font" in the IDL file. We can make it different in C++ by renaming it using [ImplementedAs]. I’m not sure we should do that, but if we do we sometimes use names like fontForBindings, or could use your suggested fontStyle or serializedFont or fontString.
Simon Fraser (smfr)
Comment 4 2020-11-19 11:18:50 PST
OK, not worth fixing then.
Chris Lord
Comment 5 2020-11-20 01:35:06 PST
(In reply to Simon Fraser (smfr) from comment #2) > Comment on attachment 414566 [details] > Patch > > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:2484 > > +#if USE(CG) > > Sad to see #if USE(CG) here. This is pre-existing. Reading the code, I don't know why that behaviour is specific to USE(CG), probably worth digging into... I'd rather not touch it with this patch though, which was just moving this from CanvasRenderingContext2D to Base. > > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:384 > > + // Therefore, all font operations must pass through the State. > > Not clear from this comment what "the State" refers to. Also pre-existing, but I've changed 'the State' to 'the proxy', because I don't see what else it could be referring to. Maybe this was a comment that missed some name-changes...
Chris Lord
Comment 6 2020-11-20 01:38:46 PST
EWS
Comment 7 2020-11-20 03:40:30 PST
Committed r270102: <https://trac.webkit.org/changeset/270102> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414663 [details].
Radar WebKit Bug Importer
Comment 8 2020-11-20 03:41:19 PST
Don Olmstead
Comment 9 2020-11-27 09:44:10 PST
This patch managed to use isSpaceThatNeedsReplacing which is defined statically in CanvasRenderingContext2D.cpp so it breaks non-unified builds. Things seem happy if you move it into the CanvasRenderingContext2DBase.h but is there a better place for that?
Chris Lord
Comment 10 2020-11-27 09:48:13 PST
(In reply to Don Olmstead from comment #9) > This patch managed to use isSpaceThatNeedsReplacing which is defined > statically in CanvasRenderingContext2D.cpp so it breaks non-unified builds. > > Things seem happy if you move it into the CanvasRenderingContext2DBase.h but > is there a better place for that? CanvasRenderingContext2DBase makes sense to me, and sorry I missed that... Is there a bug open about this?
Don Olmstead
Comment 11 2020-11-27 11:36:35 PST
(In reply to Chris Lord from comment #10) > (In reply to Don Olmstead from comment #9) > > This patch managed to use isSpaceThatNeedsReplacing which is defined > > statically in CanvasRenderingContext2D.cpp so it breaks non-unified builds. > > > > Things seem happy if you move it into the CanvasRenderingContext2DBase.h but > > is there a better place for that? > > CanvasRenderingContext2DBase makes sense to me, and sorry I missed that... > Is there a bug open about this? Actually Adrian got to it in https://trac.webkit.org/changeset/270197/webkit right after I mentioned it here.
Note You need to log in before you can comment on or make changes to this bug.