Skip to content

Refactor FXIOS-13463 #29256 [Swift 6 Migration] Final changes to web engine package #29660

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
Merged
merged 7 commits into from
Oct 1, 2025
Merged

Refactor FXIOS-13463 #29256 [Swift 6 Migration] Final changes to web engine package #29660

merged 7 commits into from
Oct 1, 2025

Conversation

lmarceau
Copy link
Contributor

📜 Tickets

Jira ticket
Github issue

💡 Description

Final changes to the WebEngine package, so strict concurrency can be turned on.

  • A lot of the protocols are now @MainActor. WKWebView objects are mostly marked @MainActor, meaning any interactions with it's configuration, view or actions needs to be called from the main actor. For that reason, the engine, session and view are now main actor. I think if we pick this project back again, we could analyze whether we want some part @concurrent once we are on Swift 6.2. Note that the engine, session and view are not used inside Client at all. Because of those reasons, I think it makes sense for the sake of the migration to just have all of those marked as main actor.

📝 Checklist

  • I filled in the ticket numbers and a description of my work
  • I updated the PR name to follow our PR naming guidelines
  • I ensured unit tests pass and wrote tests for new code
  • If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver)
  • If adding telemetry, I read the data stewardship requirements and will request a data review
  • If adding or modifying strings, I read the guidelines and will request a string review from l10n
  • If needed, I updated documentation and added comments to complex code

@lmarceau lmarceau requested a review from a team as a code owner September 26, 2025 19:04
ih-codes
ih-codes reviewed Sep 26, 2025
View reviewed changes
Copy link
Collaborator

@ih-codes ih-codes left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me! Just a couple small thoughts! 🥇

BrowserKit/Sources/WebEngine/WKWebview/Scripts/Metadata/MetadataFetcherHelper.swift

/// Helper to get the metadata fetched out of a `WKEngineSession`
@MainActor
protocol MetadataFetcherHelper {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should something that "fetches" data really be fully main actor isolated? I imagine it has to do some background stuff... 🤔 (same concern for the annotation below also)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, theoritically yes I agree, but the thing is that metadata is fetched with evaluateJavascript method, and that's marked as @MainActor in WKWebView package.
Screenshot 2025-09-26 at 4 57 31 PM

And also, since to other changes, this whole class screams red if it's not main actor 😆
Screenshot 2025-09-26 at 4 56 31 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh myyyy 😆 Ok yep keep on as you were then hahaha!! Just ignore me 😂

@lmarceau lmarceau force-pushed the lm/FXIOS-13463-#29256-WebEngine-final branch from 9d63f13 to 83576e8 Compare September 26, 2025 21:02
Base automatically changed from epic-branch/swift-migration to main September 26, 2025 21:05
@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 26, 2025

This pull request has conflicts when rebasing. Could you fix it @lmarceau? 🙏

@ih-codes
Copy link
Collaborator

ih-codes commented Sep 26, 2025 •
edited
Loading

@Mergifyio rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented Sep 26, 2025

rebase

☑️ Nothing to do, the required conditions are not met

  • -conflict [📌 rebase requirement]
  • -closed [📌 rebase requirement]
  • queue-position = -1 [📌 rebase requirement]
  • any of:
    • #commits > 1 [📌 rebase requirement]
    • #commits-behind > 0 [📌 rebase requirement]
    • -linear-history [📌 rebase requirement]

@lmarceau lmarceau force-pushed the lm/FXIOS-13463-#29256-WebEngine-final branch from 3c09474 to 6c2a42c Compare September 26, 2025 21:23
ih-codes
ih-codes approved these changes Sep 30, 2025
View reviewed changes
Copy link
Collaborator

@ih-codes ih-codes left a comment

Choose a reason for hiding this comment

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

Pushed a fix for a Bitrise compile error. Let's see if it builds now!

Cramsden and others added 7 commits October 1, 2025 15:18
@Cramsden @lmarceau
Refactor FXIOS-13511 [Swift 6] Move Tab to MainActor and resolve stri…
9c15020 
…ct concurrency warnings (#29497)

* Refactor FXIOS-13511 [Swift 6] Remove Equatable from AppState and other redux states (#29495)

* Remove unneeded equatable conformance on redux states

* Fix tests

* Fix test

* Refactor FXIOS-13532 [Swift 6 Migration] Fix strict concurrency errors related to Notifications  (#29514)

Fun stuff

* first pass of isolating tab to the main actor

* Clean up tests and push commented out code

* fix additional warnings

* Clean up remaining warnings

* fix tests

* add missing ticket numbers

* resolve introduced warning

---------

Co-authored-by: lmarceau <[email protected]>
@Cramsden @lmarceau
Refactor FXIOS-13511 [Swift 6] Additional Strict Concurrency Clean up…
b6d4674 
… for Tab and Friends (#29628)

* Additional tab clean up

* Isolate UserScriptManager to main actor

* fix tests

* turn off strict concurrency

* more main actor isolation

* resolve introduced warnings
@lmarceau
Part 3
98ebffc
@lmarceau
Final touches to WKEngineWebView
baacdaf
@lmarceau
Adjust comment
b71cf8b
@lmarceau
Fixing warnings related to nonisolated functions from MenuHelperWebVi…
1d99e45 
…ewInterface
@ih-codes @lmarceau
Fix "implicit use of 'self' in closure" error.
17110ed
@lmarceau lmarceau force-pushed the lm/FXIOS-13463-#29256-WebEngine-final branch from 94e7fe8 to 17110ed Compare October 1, 2025 19:18
@mobiletest-ci-bot
Copy link

Warnings
⚠️ Changes detected in files: firefox-ios/Client/TabManagement/Tab.swift. Ensure that necessary updates are also ported to the WebEngine project if required (cc @lmarceau).
Messages
📖 Project coverage: 38.44%

💪 Quality guardian

3 tests files modified. You're a champion of test coverage! 🚀

🥇 Perfect PR size

Smaller PRs are easier to review. Thanks for making life easy for reviewers! ✨

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

Client.app: Coverage: 37.34

File Coverage
Tab.swift 59.09% ✅

Generated by 🚫 Danger Swift against 17110ed

@lmarceau lmarceau merged commit 29332c6 into main Oct 1, 2025
10 checks passed
@lmarceau lmarceau deleted the lm/FXIOS-13463-#29256-WebEngine-final branch October 1, 2025 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ih-codes ih-codes ih-codes approved these changes

@FilippoZazzeroni FilippoZazzeroni Awaiting requested review from FilippoZazzeroni

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lmarceau @ih-codes @mobiletest-ci-bot @Cramsden