Bug 217002 – Remove support for setting CMYKA fill and stroke colors in 2D canvas

• Component: Canvas
• Status: RESOLVED FIXED

[Wenson Hsieh]

`CanvasRenderingContext2D.setFillColor(c, m, y, k, a)` and `CanvasRenderingContext2D.setStrokeColor(c, m, y, k, a)` are both non-standard, and unsupported in both Chrome and Firefox.

(Additionally, `setFillColor` and `setStrokeColor` are also nonstandard, though this patch will only track removing support for setting CMYKA fill and stroke colors via setFillColor and setStrokeColor).

[Wenson Hsieh]

Created attachment 409789 [details]
For EWS

[Wenson Hsieh]

Created attachment 409791 [details]
Rebaseline more tests

[Sam Weinig]

Do you have any telemetry indicating this is ok to remove (kind of rhetorical, I expect the answer is no). There is quite a bit of WebKit only canvas code. While I would love to remove a ton of these non-standard APIs we have, it doesn't seem to safe to do until there is some indication of usage.

[Darin Adler]

Comment on attachment 409791 [details]
Rebaseline more tests

View in context: https://bugs.webkit.org/attachment.cgi?id=409791&action=review

> Source/WebCore/ChangeLog:17
> +        In lieu of adding new DisplayList items to support setting fill and stroke colors with CMYKA components,
> +        we can take this opportunity to just drop support for this canvas API entirely. Fixes the following tests
> +        when using the GPU Process to render canvas:

I understand this feature is WebKit-only; WebKit’s canvas was the original and predates the standard. How did you determine this feature is not used?

> Source/WebCore/html/canvas/CanvasStyle.h:107
> +    return serializationForHTML(WTF::holds_alternative<Color>(m_style) ? WTF::get<Color>(m_style) : Color());

This should just call get<Color>, no need to call holds_alternative<>. The function had a precondition before that it m_style needed to be either Color or CMYKAColor, and it would assert if that was not so. No reason we have to add support for the case where it’s neither just because we are removing CMYKAColor.

[Wenson Hsieh]

(In reply to Sam Weinig from comment #3)
> Do you have any telemetry indicating this is ok to remove (kind of
> rhetorical, I expect the answer is no). There is quite a bit of WebKit only
> canvas code. While I would love to remove a ton of these non-standard APIs
> we have, it doesn't seem to safe to do until there is some indication of
> usage.

I verified that at least no Apple-internal clients use the CMYKA (i.e. 5 argument) variant of setFillColor or setStrokeColor, but unfortunately, I don’t have telemetry for the usage of these CMYKA setters on the web :(. I did, however, find one internal use of the RGBA (4 argument) version of setFillColor, which does make me believe that we should hold off on removing the setFillColor or setStrokeColor methods entirely.

I do think it’s unlikely enough that any websites would be using these 5-argument CMYKA methods, such that we could _probably_ remove them without risk of breaking anything.

I originally considered implementing these CMYKA setters using new display list items for GPUP, but after talking with Dean and Simon last year, we came to the conclusion that it would be better to just remove these instead…

[Darin Adler]

(In reply to Wenson Hsieh from comment #5)
> I do think it’s unlikely enough that any websites would be using these
> 5-argument CMYKA methods, such that we could _probably_ remove them without
> risk of breaking anything.

Sure, there probably aren’t a lot of websites that are intentionally WebKit-only, and it seems unlikely someone would do this by mistake.

We do need to consider apps with embedded HTML content. The <canvas> element was originally designed for use by Dashboard widgets, which, like HTML content in apps, might only be tested with WebKit.

I’m not saying we can’t "play the odds" here, but there is a chance this breaks an iOS app, for example. On the other hand, that’s true almost any time we change WebKit’s behavior, whether our goal is fixing a bug or changing to be more like other web engines for better interoperability and standards compliance.

[Wenson Hsieh]

(In reply to Darin Adler from comment #6)
> (In reply to Wenson Hsieh from comment #5)
> > I do think it’s unlikely enough that any websites would be using these
> > 5-argument CMYKA methods, such that we could _probably_ remove them without
> > risk of breaking anything.
> 
> Sure, there probably aren’t a lot of websites that are intentionally
> WebKit-only, and it seems unlikely someone would do this by mistake.
> 
> We do need to consider apps with embedded HTML content. The <canvas> element
> was originally designed for use by Dashboard widgets, which, like HTML
> content in apps, might only be tested with WebKit.

I see!

> 
> I’m not saying we can’t "play the odds" here, but there is a chance this
> breaks an iOS app, for example. On the other hand, that’s true almost any
> time we change WebKit’s behavior, whether our goal is fixing a bug or
> changing to be more like other web engines for better interoperability and
> standards compliance.

That’s true — 3rd party app compatibility is a potential risk, though it still seems super unlikely that any apps will be affected. If we do see any reports, I suppose we could enable this at runtime behind a linked-on-or-after check...

[Wenson Hsieh]

Created attachment 409800 [details]
Patch for landing

[EWS]

Committed r267645: <https://trac.webkit.org/changeset/267645>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409800 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69641490>