WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(122.64 KB, patch)
2020-11-20 01:38 PST
,
Chris Lord
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Chris Lord
Comment 1
2020-11-19 05:06:40 PST
Created
attachment 414566
[details]
Patch
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
Created
attachment 414663
[details]
Patch
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
<
rdar://problem/71622345
>
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.
Top of Page
Format For Printing
XML
Clone This Bug