Refactor FXIOS-7596 [v121] remove legacy store #17224

OrlaM · 2023-11-07T20:56:01Z

📜 Tickets

Jira ticket
Github issue

💡 Description

Fully removes the legacy tab store code

📝 Checklist

You have to check all boxes before merging

Filled in the above information (tickets numbers and description of your work)
Updated the PR name to follow our PR naming guidelines
Wrote unit tests and/or ensured the tests suite is passing
When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
If needed I updated documentation / comments for complex code and public methods

mobiletest-ci-bot · 2023-11-07T21:07:26Z

	Messages
📖	Project coverage: 33.79%
📖	Edited 7 files
📖	Created 0 files

Client.app: Coverage: 32.4

File	Coverage
LegacyTabManager.swift	25.67%	⚠️
LegacyTabTrayViewController.swift	41.93%	⚠️
TabManagerImplementation.swift	49.39%	⚠️
UITestAppDelegate.swift	0.0%	⚠️
BrowserViewController+TabToolbarDelegate.swift	11.03%	⚠️
LegacyGridTabViewController.swift	31.4%	⚠️

Generated by 🚫 Danger Swift against cea3eb1

isabelrios · 2023-11-08T15:04:47Z

@OrlaM all the tests failed, I can't get the details because they timed out but looks like the failure is the same or similar error @skhamis had

lmarceau

UI tests failed again, but LGTM

OrlaM · 2023-11-09T14:26:44Z

@OrlaM all the tests failed, I can't get the details because they timed out but looks like the failure is the same or similar error @skhamis had

I fixed the crash but now any test that opens a new tab from the tab tray fails, for some reason the tests don't open a new tab. I have no idea why that is happening, it works when running the test cases manually.
This PR does free up a good bit of work on the main thread so things could be running faster where the tests may have previously relied on the delay but I tried adding some delays in spots in the tests to confirm this theory but didn't succeed.

mergify · 2023-11-09T14:31:22Z

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

isabelrios · 2023-11-10T09:45:38Z

@OrlaM all the tests failed, I can't get the details because they timed out but looks like the failure is the same or similar error @skhamis had

I fixed the crash but now any test that opens a new tab from the tab tray fails, for some reason the tests don't open a new tab. I have no idea why that is happening, it works when running the test cases manually. This PR does free up a good bit of work on the main thread so things could be running faster where the tests may have previously relied on the delay but I tried adding some delays in spots in the tests to confirm this theory but didn't succeed.

oh, let me take a look.. if tests fail because the app runs faster, that's a good thing and easy to fix.. cc @clarmso

isabelrios · 2023-11-10T10:58:45Z

@OrlaM I know where the problem is but I don't have a solution yet... the UI tests are failing when trying to open a new tab from Tab Tray, we use that action a lot(OpenNewTabFromTabTray)
Looks like as you said, tests run faster and we miss the tap in the '+' icon to open a new tab from there...
I tried to wait for that element and now the test can click on it. However, I'm seeing this weird state after clicking on '+' where it shows the keyboard as if we were in the new tab but the view has not actually changed...

This will require further investigation .. I will keep you posted cc @clarmso

mergify · 2023-11-21T13:47:05Z

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

OrlaM · 2023-11-24T17:11:51Z

It's finally passing 🥳

OrlaM · 2023-11-24T17:13:42Z

Client/Frontend/Browser/Tabs/Legacy/LegacyGridTabViewController.swift

+        DispatchQueue.main.async {
+            self.navigationController?.dismiss(animated: true, completion: nil)
+            TelemetryWrapper.recordEvent(category: .action, method: .close, object: .tabTray)
+        }


This is not an ideal solution, but since the select tab that happens prior to this is now an async event it is causing crashes on the home page when the view is dismissed since the tab data is not fully resolved after deleting tabs. I'm comfortable putting this solution here because the legacy tab tray is going away soon anyway.

OrlaM · 2023-11-24T18:49:10Z

@Mergifyio backport release/v121

mergify · 2023-11-24T18:49:13Z

backport release/v121

✅ Backports have been created

#17468 Refactor FXIOS-7596 [v121] remove legacy store (backport #17224) has been created for branch release/v121 but encountered conflicts

lmarceau · 2023-11-24T18:52:03Z

Client/Frontend/Browser/BrowserViewController/BrowserViewController+TabToolbarDelegate.swift

+        overlayManager.openNewTab(url: nil,
+                                  newTabSettings: NewTabAccessors.getNewTabPage(profile.prefs))


I'm trying to understand why we need to add a call to the overlayManager in this particular PR to remove the legacy store? I guess this relates to the tests that were failing right?

Yup, for some reason it no longer triggers overlay mode at the right time. There is likely a superfluous call at the wrong time somewhere else but I was afraid to start messing with it because it's spagetified into other code paths.

* Move select tab * Update isRestoringTabs * Remove legacy store * Fix start up issue * Remove store * Clear more dead code * Fix UI tests * Fix tests * Fix tests * Fix tests * Remove print (cherry picked from commit 21d5c81) # Conflicts: # Client.xcodeproj/project.pbxproj

* Move select tab * Update isRestoringTabs * Remove legacy store * Fix start up issue * Remove store * Clear more dead code * Fix UI tests * Fix tests * Fix tests * Fix tests * Remove print # Conflicts: # Client.xcodeproj/project.pbxproj

* Move select tab * Update isRestoringTabs * Remove legacy store * Fix start up issue * Remove store * Clear more dead code * Fix UI tests * Fix tests * Fix tests * Fix tests * Remove print # Conflicts: # Client.xcodeproj/project.pbxproj Co-authored-by: OrlaM <[email protected]>

OrlaM requested a review from lmarceau November 7, 2023 20:56

OrlaM requested a review from a team as a code owner November 7, 2023 20:56

lmarceau approved these changes Nov 8, 2023

View reviewed changes

OrlaM force-pushed the om/FXIOS-7596-remove-legacy-store branch from e6cabee to 9603d83 Compare November 19, 2023 00:44

OrlaM force-pushed the om/FXIOS-7596-remove-legacy-store branch 2 times, most recently from d558fba to fba1c16 Compare November 23, 2023 19:21

OrlaM added 10 commits November 24, 2023 11:29

Move select tab

be99cc1

Update isRestoringTabs

85819ca

Remove legacy store

0ac40e5

Fix start up issue

15b3d61

Remove store

1b7bfd1

Clear more dead code

a9dd8ae

Fix UI tests

e89fbec

Fix tests

d8a63a4

Fix tests

c6d0356

Fix tests

33d9686

OrlaM force-pushed the om/FXIOS-7596-remove-legacy-store branch from fba1c16 to 33d9686 Compare November 24, 2023 16:30

OrlaM commented Nov 24, 2023

View reviewed changes

Remove print

cea3eb1

OrlaM changed the title ~~Refactor FXIOS-7596 [v121] remove legacy store~~ Refactor FXIOS-7596 [v122] remove legacy store Nov 24, 2023

OrlaM changed the title ~~Refactor FXIOS-7596 [v122] remove legacy store~~ Refactor FXIOS-7596 [v121] remove legacy store Nov 24, 2023

lmarceau reviewed Nov 24, 2023

View reviewed changes

lmarceau approved these changes Nov 24, 2023

View reviewed changes

OrlaM merged commit 21d5c81 into mozilla-mobile:main Nov 24, 2023
10 checks passed

OrlaM deleted the om/FXIOS-7596-remove-legacy-store branch November 24, 2023 20:21

mergify bot mentioned this pull request Nov 24, 2023

Refactor FXIOS-7596 [v121] remove legacy store (backport #17224) #17468

Merged

clarmso mentioned this pull request Dec 1, 2023

New tab launched from context menu not reported properly #17579

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor FXIOS-7596 [v121] remove legacy store #17224

Refactor FXIOS-7596 [v121] remove legacy store #17224

OrlaM commented Nov 7, 2023

mobiletest-ci-bot commented Nov 7, 2023 •

edited

Loading

isabelrios commented Nov 8, 2023 •

edited

Loading

lmarceau left a comment

OrlaM commented Nov 9, 2023

mergify bot commented Nov 9, 2023

isabelrios commented Nov 10, 2023

isabelrios commented Nov 10, 2023

mergify bot commented Nov 21, 2023

OrlaM commented Nov 24, 2023

OrlaM Nov 24, 2023

OrlaM commented Nov 24, 2023

mergify bot commented Nov 24, 2023 •

edited

Loading

lmarceau Nov 24, 2023

OrlaM Nov 24, 2023

		overlayManager.openNewTab(url: nil,
		newTabSettings: NewTabAccessors.getNewTabPage(profile.prefs))

Refactor FXIOS-7596 [v121] remove legacy store #17224

Refactor FXIOS-7596 [v121] remove legacy store #17224

Conversation

OrlaM commented Nov 7, 2023

📜 Tickets

💡 Description

📝 Checklist

mobiletest-ci-bot commented Nov 7, 2023 • edited Loading

Client.app: Coverage: 32.4

isabelrios commented Nov 8, 2023 • edited Loading

lmarceau left a comment

Choose a reason for hiding this comment

OrlaM commented Nov 9, 2023

mergify bot commented Nov 9, 2023

isabelrios commented Nov 10, 2023

isabelrios commented Nov 10, 2023

mergify bot commented Nov 21, 2023

OrlaM commented Nov 24, 2023

OrlaM Nov 24, 2023

Choose a reason for hiding this comment

OrlaM commented Nov 24, 2023

mergify bot commented Nov 24, 2023 • edited Loading

✅ Backports have been created

lmarceau Nov 24, 2023

Choose a reason for hiding this comment

OrlaM Nov 24, 2023

Choose a reason for hiding this comment

mobiletest-ci-bot commented Nov 7, 2023 •

edited

Loading

isabelrios commented Nov 8, 2023 •

edited

Loading

mergify bot commented Nov 24, 2023 •

edited

Loading