Enable font functions on OffscreenCanvas for main-thread

• Component: Canvas
• Status: RESOLVED FIXED

[Chris Lord]

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.

[Chris Lord]

Created attachment 414566 [details]
Patch

[Simon Fraser (smfr)]

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 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)]

OK, not worth fixing then.

[Chris Lord]

(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]

Created attachment 414663 [details]
Patch

[EWS]

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]

<rdar://problem/71622345>

[Don Olmstead]

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]

(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]

(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.