Skip to content

Follow up fixes to "Refactor RenderThemeMac::iconForAttachment" #46225

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

Follow up fixes to "Refactor RenderThemeMac::iconForAttachment" #46225

wants to merge 1 commit into from

Conversation

igower
Copy link
Contributor

@igower igower commented Jun 2, 2025 •
edited by webkit-early-warning-system
Loading

db8cba4

Follow up fixes to "Refactor RenderThemeMac::iconForAttachment"
https://bugs.webkit.org/show_bug.cgi?id=293919
rdar://152453766

Reviewed by NOBODY (OOPS!).

Updated syntax as according to comments on the last commit. Reverted "auto size = image.size" back to "auto size = [image size]" as this caused a compilation error. I did not implement the change of "convertPlatformImageToBitmap() to accept a RenderThemeCocoa::IconAndSize" as iconAndSize does not contain the same value as iconSize. (Previous Commit: 295710@main)

* Source/WebCore/rendering/mac/RenderThemeMac.mm:
(WebCore::RenderThemeMac::iconForAttachment):
* Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:
(WebKit::WebPageProxy::iconForAttachment):

db8cba4

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

@igower igower requested a review from cdumez as a code owner June 2, 2025 22:17
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on previous version of this PR (hash 4b510ed)

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 Jun 2, 2025
@igower igower force-pushed the eng/Follow-up-fixes-to-Refactor-RenderThemeMac-iconForAttachment branch from 4b510ed to 7358164 Compare June 2, 2025 23:16
@webkit-early-warning-system
Copy link
Collaborator

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

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

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

@igower igower force-pushed the eng/Follow-up-fixes-to-Refactor-RenderThemeMac-iconForAttachment branch from 7358164 to c2a1e91 Compare June 2, 2025 23:18
@webkit-early-warning-system
Copy link
Collaborator

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

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

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

pxlcoder
pxlcoder reviewed Jun 2, 2025
View reviewed changes
Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm
@@ -418,9 +418,7 @@ static bool exceedsRenderTreeSizeSizeThreshold(uint64_t thresholdSize, uint64_t
#else
auto iconAndSize = RenderThemeMac::iconForAttachment(fileName, contentType, title);
#endif
auto icon = iconAndSize.icon;
size = iconAndSize.size;
Copy link
Member

Choose a reason for hiding this comment

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

size is an out parameter, is this really safe to delete?

@pxlcoder
Copy link
Member

pxlcoder commented Jun 2, 2025

Also, when doing a follow-up it's nice to reference the commit identifier (in this case, 295710@main) rather than the title (which is not unique, and less easy to search for).

@igower
Follow up fixes to "Refactor RenderThemeMac::iconForAttachment"
db8cba4 
https://bugs.webkit.org/show_bug.cgi?id=293919
rdar://152453766

Reviewed by NOBODY (OOPS!).

Updated syntax as according to comments on the last commit. Reverted "auto size = image.size" back to "auto size = [image size]" as this caused a compilation error. I did not implement the change of "convertPlatformImageToBitmap() to accept a RenderThemeCocoa::IconAndSize" as iconAndSize does not contain the same value as iconSize. (Previous Commit: 295710@main)

* Source/WebCore/rendering/mac/RenderThemeMac.mm:
(WebCore::RenderThemeMac::iconForAttachment):
* Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:
(WebKit::WebPageProxy::iconForAttachment):
@igower igower force-pushed the eng/Follow-up-fixes-to-Refactor-RenderThemeMac-iconForAttachment branch from c2a1e91 to db8cba4 Compare June 5, 2025 00:19
@webkit-early-warning-system
Copy link
Collaborator

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

EWS run on current version of this PR (hash db8cba4)

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

@igower igower requested a review from pxlcoder June 5, 2025 23:00
@igower igower added safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks and removed merging-blocked Applied to prevent a change from being merged labels Jun 26, 2025
@webkit-ews-buildbot
Copy link
Collaborator

@igower does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

If you do have committer permissions, please ensure that your GitHub username is added to contributors.json.

Rejecting db8cba4 from merge queue.

Safe-Merge-Queue: Build #61511.

@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Jun 26, 2025
@webkit-ews-buildbot
Copy link
Collaborator

Safe-Merge-Queue: Build #61511.

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

@pxlcoder pxlcoder pxlcoder approved these changes

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

Assignees

@igower igower

Labels
merging-blocked Applied to prevent a change from being merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@igower @webkit-early-warning-system @pxlcoder @webkit-ews-buildbot