Bug 219088

Created attachment 414566 [details] Patch

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.

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.

OK, not worth fixing then.

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

Created attachment 414663 [details] Patch

Committed r270102: <https://trac.webkit.org/changeset/270102> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414663 [details].

<rdar://problem/71622345>

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?

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

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