Skip to content

Remove non-standard legacy drawImageFromRect (CanvasDrawImage) #38135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove non-standard legacy drawImageFromRect (CanvasDrawImage) #38135

wants to merge 1 commit into from

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Dec 18, 2024 •
edited by webkit-early-warning-system
Loading

31137a0

Remove non-standard legacy `drawImageFromRect` (CanvasDrawImage)
https://bugs.webkit.org/show_bug.cgi?id=284878
rdar://141681635

Reviewed by NOBODY (OOPS!).

This patch is to align WebKit with Gecko / Firefox and Blink / Chromium.

We have our current implementation based on standards (drawImage) so this patch
aims to get rid of legacy non-standard `drawImageFromRect`.

Blink removed this as well in 2014 in below commit:

Commit: chromium/chromium@ce07cef

From MDN data, Safari / WebKit has supported standard alternatives since Safari 2,
so this is about time to try to get rid of non-standard legacy `drawImageFromRect`.

* Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:
* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:
(WebCore::CanvasRenderingContext2DBase::drawImageFromRect): Deleted.
* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:
* Source/WebInspectorUI/UserInterface/Models/NativeFunctionParameters.js:
* Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:
(WI.RecordingAction.prototype.getImageParameters):
* Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:
(WI.RecordingActionTreeElement._classNameForAction):
* LayoutTests/fast/canvas/canvas-overloads-drawImageFromRect-expected.txt: Removed.
* LayoutTests/fast/canvas/canvas-overloads-drawImageFromRect.html: Removed.
* LayoutTests/fast/canvas/drawImageFromRect_withToDataURLAsSource-expected.txt: Removed.
* LayoutTests/fast/canvas/drawImageFromRect_withToDataURLAsSource.html: Removed.
* LayoutTests/fast/canvas/image-object-in-canvas-expected.txt:
* LayoutTests/fast/canvas/image-object-in-canvas.html:
* LayoutTests/inspector/canvas/recording-2d-full-expected.txt:

31137a0

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 self-assigned this Dec 18, 2024
@Ahmad-S792 Ahmad-S792 added the Canvas Bugs related to the canvas element. label Dec 18, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Dec 18, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 90a58f3)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ❌ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 requested a review from nt1m December 18, 2024 16:52
@Ahmad-S792 Ahmad-S792 force-pushed the eng/Remove-non-standard-legacy-drawImageFromRect-CanvasDrawImage branch from 90a58f3 to 3edfe49 Compare May 8, 2025 22:38
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 8, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 3edfe49)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 requested a review from kkinnunen-apple May 9, 2025 00:51
@kkinnunen-apple
Copy link
Contributor

You're leaving some references to it in WebInspectorUI on purpose?
It's unclear to me what's the policy how to remove these. Would be good to get rid of them, I suppose..

@dcrousso
Copy link
Member

dcrousso commented May 9, 2025

Web Inspector supports inspecting back to iOS 13, so unless this was not possible to use back then it's best to keep the code in Web Inspector until there's no supported versions of iOS that have this feature

@kkinnunen-apple
Copy link
Contributor

kkinnunen-apple commented May 9, 2025 •
edited
Loading

Web Inspector supports inspecting back to iOS 13, so unless this was not possible to use back then it's best to keep the code in Web Inspector until there's no supported versions of iOS that have this feature

If anybody was Inspecting anybody using the feature, it'd be illogical to remove it in here?

@Ahmad-S792
Copy link
Contributor Author

I didn't remove it from Web Inspector js files due to Devin's comment that we support old iOS releases.

@Ahmad-S792 Ahmad-S792 force-pushed the eng/Remove-non-standard-legacy-drawImageFromRect-CanvasDrawImage branch from 3edfe49 to f79203e Compare May 13, 2025 21:16
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 13, 2025 •
edited
Loading

EWS run on previous version of this PR (hash f79203e)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 13, 2025
@dcrousso
Copy link
Member

well @kkinnunen-apple has a point. if we know for sure that this isn't used by webpages then i think it's probably fine to remove from Web Inspector as well since there's not really a need to debug it (since it's not used by webpages)

@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label May 26, 2025
@Ahmad-S792 Ahmad-S792 force-pushed the eng/Remove-non-standard-legacy-drawImageFromRect-CanvasDrawImage branch from f79203e to 4713ef8 Compare May 26, 2025 02:08
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 26, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 4713ef8)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792
Copy link
Contributor Author

@dcrousso - Updated and removed references from WebInspector as well (all which I could find). @kkinnunen-apple - FYI

@Ahmad-S792
Remove non-standard legacy drawImageFromRect (CanvasDrawImage)
31137a0 
https://bugs.webkit.org/show_bug.cgi?id=284878
rdar://141681635

Reviewed by NOBODY (OOPS!).

This patch is to align WebKit with Gecko / Firefox and Blink / Chromium.

We have our current implementation based on standards (drawImage) so this patch
aims to get rid of legacy non-standard `drawImageFromRect`.

Blink removed this as well in 2014 in below commit:

Commit: chromium/chromium@ce07cef

From MDN data, Safari / WebKit has supported standard alternatives since Safari 2,
so this is about time to try to get rid of non-standard legacy `drawImageFromRect`.

* Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:
* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:
(WebCore::CanvasRenderingContext2DBase::drawImageFromRect): Deleted.
* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:
* Source/WebInspectorUI/UserInterface/Models/NativeFunctionParameters.js:
* Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:
(WI.RecordingAction.prototype.getImageParameters):
* Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.js:
(WI.RecordingActionTreeElement._classNameForAction):
* LayoutTests/fast/canvas/canvas-overloads-drawImageFromRect-expected.txt: Removed.
* LayoutTests/fast/canvas/canvas-overloads-drawImageFromRect.html: Removed.
* LayoutTests/fast/canvas/drawImageFromRect_withToDataURLAsSource-expected.txt: Removed.
* LayoutTests/fast/canvas/drawImageFromRect_withToDataURLAsSource.html: Removed.
* LayoutTests/fast/canvas/image-object-in-canvas-expected.txt:
* LayoutTests/fast/canvas/image-object-in-canvas.html:
* LayoutTests/inspector/canvas/recording-2d-full-expected.txt:
@Ahmad-S792 Ahmad-S792 force-pushed the eng/Remove-non-standard-legacy-drawImageFromRect-CanvasDrawImage branch from 4713ef8 to 31137a0 Compare June 27, 2025 08:03
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 27, 2025 •
edited
Loading

EWS run on current version of this PR (hash 31137a0)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

kkinnunen-apple
kkinnunen-apple approved these changes Jun 27, 2025
View reviewed changes
Copy link
Contributor

@kkinnunen-apple kkinnunen-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a request to git grep one more time and remove all references if removing them makes sense.

LayoutTests/inspector/canvas/recording-2d-full-expected.txt
@@ -305,19 +305,6 @@ frames:
3: (anonymous function)
4: executeFrameFunction
15: (duration)
0: drawImageFromRect([object HTMLImageElement], 1, 2, 3, 4, 5, 6, 7, 8, "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you need to remove the calls in recording-2d.js ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkinnunen-apple kkinnunen-apple kkinnunen-apple approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@rniwa rniwa Awaiting requested review from rniwa rniwa is a code owner

@dcrousso dcrousso Awaiting requested review from dcrousso dcrousso is a code owner

@patrickangle patrickangle Awaiting requested review from patrickangle patrickangle is a code owner

@nt1m nt1m Awaiting requested review from nt1m

Assignees

@Ahmad-S792 Ahmad-S792

Labels
Canvas Bugs related to the canvas element.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ahmad-S792 @webkit-early-warning-system @kkinnunen-apple @dcrousso @webkit-ews-buildbot