Skip to content

Conversation

@whsieh
Copy link
Member

@whsieh whsieh commented Feb 2, 2025

75ce30b

[visionOS] Web process hangs under createDragImageFor{Link|Selection} when starting a drag for the first time
https://bugs.webkit.org/show_bug.cgi?id=286819
rdar://141448897

Reviewed by Abrar Rahman Protyasha.

On visionOS, when starting a drag for the first time on a webpage, we end up spending a large amount
of time underneath `+[UIScreen initialize]` within the web process, while trying to render the drag
image using the helpers in `DragImageIOS.mm` (which instantiate `UIGraphicsImageRenderer`). This
causes the web process to hang briefly, which in turn may cause the drag to fail entirely.

To mitigate this, we refactor these helpers such that they no longer rely (indirectly) on
instantiating `UIScreen` via `UIGraphicsImageRenderer`, and instead use `WebCore::ImageBuffer` and
`sinkIntoNativeImage`. See below for more details.

* Source/WebCore/dom/DataTransfer.cpp:
(WebCore::DataTransfer::setDragImage):
(WebCore::DataTransfer::updateDragImage):
(WebCore::DataTransfer::createDragImage const):
(WebCore::DragImageLoader::DragImageLoader):
(WebCore::DragImageLoader::imageChanged):
* Source/WebCore/dom/DataTransfer.h:
* Source/WebCore/dom/DataTransferMac.mm:
(WebCore::DataTransfer::createDragImage const):

Plumb the `Document` through here, so that we can get the `deviceScaleFactor` / `HostWindow` and
pass them through to `createDragImageFromImage`.

* Source/WebCore/page/DragController.cpp:
(WebCore::DragController::startDrag):
(WebCore::DragController::doImageDrag):

Pass the device scale factor and host window into `createDragImageFromImage`.

* Source/WebCore/platform/DragImage.cpp:
(WebCore::createDragImageFromImage):
* Source/WebCore/platform/DragImage.h:
* Source/WebCore/platform/cocoa/DragImageCocoa.mm:
(WebCore::createDragImageFromImage):
* Source/WebCore/platform/gtk/DragImageGtk.cpp:
(WebCore::createDragImageFromImage):
* Source/WebCore/platform/ios/DragImageIOS.mm:
(WebCore::scaleDragImage):

Make this helper function a no-op. This logic was actually unnecessary from the start, since the
drag image will always perform lift and cancel animations using targeted previews which scale to fit
the bounds of the dragged element. As such, there's no need to artificially clamp the drag image to
an arbitrary maximum size of 400x400.

(WebCore::createDragImageFromImage):

Make this take both a device scale factor and `HostWindow`, which allows us to create a new
`ImageBuffer`, paint the given image into the buffer (scaling down if needed), and finally extract
a native image from the image buffer. This avoids the need for `UIGraphicsImageRenderer` entirely.

(WebCore::deleteDragImage):
(WebCore::cgImageFromTextIndicator):
(WebCore::createDragImageForLink):

Remove logic for creating a drag image representing a link (URL) platter. This has been unnecessary
ever since we (1) adopted `+[UIDragPreview previewForURL:title:]`, and (2) use text indicator for
the lift preview. Instead, just return the content image of the text indicator here.

(WebCore::createDragImageForSelection):

Adjust these helper methods, so that they simply return the content image of the text indicator.
Note that we just use the text indicator itself for the targeted preview anyways, so this image is
effectively unused. In a future patch, we should refactor `DragController` to not bail if the drag
image's `image` is `nullptr` (but the `DragImage` is instead backed by a `TextIndicator`), which
would allow us to avoid some of this extra work.

(WebCore::cascadeForSystemFont): Deleted.
* Source/WebCore/platform/win/DragImageWin.cpp:
(WebCore::createDragImageFromImage):
* Source/WebCore/platform/win/cairo/DragImageWinCairo.cpp:
(WebCore::createDragImageFromImage):

Canonical link: https://commits.webkit.org/289740@main

6270ff4

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
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@whsieh whsieh requested review from a team, magomez and zdobersek as code owners February 2, 2025 02:14
@whsieh whsieh self-assigned this Feb 2, 2025
@whsieh whsieh requested review from cdumez and rniwa as code owners February 2, 2025 02:14
@whsieh whsieh added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Feb 2, 2025
@whsieh whsieh marked this pull request as draft February 2, 2025 07:39
@whsieh whsieh marked this pull request as ready for review February 2, 2025 21:48
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 2, 2025
@whsieh whsieh removed the merging-blocked Applied to prevent a change from being merged label Feb 3, 2025
@whsieh
Copy link
Member Author

whsieh commented Feb 3, 2025

(macOS failures are unrelated)

@whsieh
Copy link
Member Author

whsieh commented Feb 3, 2025

Thanks for the review!

@whsieh whsieh added the merge-queue Applied to send a pull request to merge-queue label Feb 3, 2025
… when starting a drag for the first time

https://bugs.webkit.org/show_bug.cgi?id=286819
rdar://141448897

Reviewed by Abrar Rahman Protyasha.

On visionOS, when starting a drag for the first time on a webpage, we end up spending a large amount
of time underneath `+[UIScreen initialize]` within the web process, while trying to render the drag
image using the helpers in `DragImageIOS.mm` (which instantiate `UIGraphicsImageRenderer`). This
causes the web process to hang briefly, which in turn may cause the drag to fail entirely.

To mitigate this, we refactor these helpers such that they no longer rely (indirectly) on
instantiating `UIScreen` via `UIGraphicsImageRenderer`, and instead use `WebCore::ImageBuffer` and
`sinkIntoNativeImage`. See below for more details.

* Source/WebCore/dom/DataTransfer.cpp:
(WebCore::DataTransfer::setDragImage):
(WebCore::DataTransfer::updateDragImage):
(WebCore::DataTransfer::createDragImage const):
(WebCore::DragImageLoader::DragImageLoader):
(WebCore::DragImageLoader::imageChanged):
* Source/WebCore/dom/DataTransfer.h:
* Source/WebCore/dom/DataTransferMac.mm:
(WebCore::DataTransfer::createDragImage const):

Plumb the `Document` through here, so that we can get the `deviceScaleFactor` / `HostWindow` and
pass them through to `createDragImageFromImage`.

* Source/WebCore/page/DragController.cpp:
(WebCore::DragController::startDrag):
(WebCore::DragController::doImageDrag):

Pass the device scale factor and host window into `createDragImageFromImage`.

* Source/WebCore/platform/DragImage.cpp:
(WebCore::createDragImageFromImage):
* Source/WebCore/platform/DragImage.h:
* Source/WebCore/platform/cocoa/DragImageCocoa.mm:
(WebCore::createDragImageFromImage):
* Source/WebCore/platform/gtk/DragImageGtk.cpp:
(WebCore::createDragImageFromImage):
* Source/WebCore/platform/ios/DragImageIOS.mm:
(WebCore::scaleDragImage):

Make this helper function a no-op. This logic was actually unnecessary from the start, since the
drag image will always perform lift and cancel animations using targeted previews which scale to fit
the bounds of the dragged element. As such, there's no need to artificially clamp the drag image to
an arbitrary maximum size of 400x400.

(WebCore::createDragImageFromImage):

Make this take both a device scale factor and `HostWindow`, which allows us to create a new
`ImageBuffer`, paint the given image into the buffer (scaling down if needed), and finally extract
a native image from the image buffer. This avoids the need for `UIGraphicsImageRenderer` entirely.

(WebCore::deleteDragImage):
(WebCore::cgImageFromTextIndicator):
(WebCore::createDragImageForLink):

Remove logic for creating a drag image representing a link (URL) platter. This has been unnecessary
ever since we (1) adopted `+[UIDragPreview previewForURL:title:]`, and (2) use text indicator for
the lift preview. Instead, just return the content image of the text indicator here.

(WebCore::createDragImageForSelection):

Adjust these helper methods, so that they simply return the content image of the text indicator.
Note that we just use the text indicator itself for the targeted preview anyways, so this image is
effectively unused. In a future patch, we should refactor `DragController` to not bail if the drag
image's `image` is `nullptr` (but the `DragImage` is instead backed by a `TextIndicator`), which
would allow us to avoid some of this extra work.

(WebCore::cascadeForSystemFont): Deleted.
* Source/WebCore/platform/win/DragImageWin.cpp:
(WebCore::createDragImageFromImage):
* Source/WebCore/platform/win/cairo/DragImageWinCairo.cpp:
(WebCore::createDragImageFromImage):

Canonical link: https://commits.webkit.org/289740@main
@webkit-commit-queue
Copy link
Collaborator

Committed 289740@main (75ce30b): https://commits.webkit.org/289740@main

Reviewed commits have been landed. Closing PR #39867 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 75ce30b into WebKit:main Feb 3, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

New Bugs Unclassified bugs are placed in this component until the correct component can be determined.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants