Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Issue #22489: Remove "Fennec to Fenix" migration code #23651

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Issue #22489: Remove "Fennec to Fenix" migration code #23651

merged 1 commit into from
Mar 28, 2022

Conversation

gabrielluong
Copy link
Member

@gabrielluong gabrielluong commented Feb 9, 2022 •
edited
Loading

Fixes #22489. This is a rebase of #22507 mainly to address any conflicts and bump strings removedIn version.

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@gabrielluong gabrielluong requested review from a team as code owners February 9, 2022 04:50
@gabrielluong
Copy link
Member Author

@grigoryk @csadilek @jonalmeida Let's figure out what we need to do to finally remove this, but feel free to take over.

@gabrielluong gabrielluong added the needs:review PRs that need to be reviewed label Feb 9, 2022
@csadilek csadilek mentioned this pull request Feb 9, 2022
Closed
@csadilek
Copy link
Contributor

csadilek commented Feb 9, 2022

@gabrielluong all that's left is review and land. we have sign-off to land when ready.

@gabrielluong
Copy link
Member Author

gabrielluong commented Feb 9, 2022 •
edited
Loading

@gabrielluong all that's left is review and land. we have sign-off to land when ready.

Based on #22507 (comment), I imagine we also want to test thoroughly, but I am not quite sure what I should be looking for. I leave this up to yall, let's get this done!

@grigoryk grigoryk self-assigned this Feb 9, 2022
@mergify
Copy link
Contributor

mergify bot commented Feb 14, 2022

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

grigoryk
grigoryk reviewed Feb 18, 2022
View reviewed changes
Copy link
Contributor

@grigoryk grigoryk left a comment

Choose a reason for hiding this comment

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

Just looking through the changes, it all looks fine at first glance. I think we mostly just need to make ensure that nothing odd happens w/ things such as shortcuts, PWAs, search widget, etc.

What kind of local testing did you do for this?

To get started, I'm going to try the following cases locally:

  • verify that home screen shortcuts, installed PWA, search widget continue to work (when installed from fenix, and when installed from fennec and then migrated)
  • install fennec release variant, upgrade to fenix (w/ migration code in place), upgrade to fenix (w/o migration code)
  • do the same for other channels (beta, nightly, i think that's it?)

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt Outdated
::dismissTip
)
)
emptyList()
Copy link
Contributor

Choose a reason for hiding this comment

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

You should just do tip = null, or omit tip entirely since it's defined as val tip: Tip? = null. getTip will just return null here.

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt Outdated
::dismissTip
)
)
emptyList()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, omit tip or set it to null.

@gabrielluong gabrielluong force-pushed the remove-migration branch 2 times, most recently from 19181fa to 5b85943 Compare February 22, 2022 17:27
@mergify
Copy link
Contributor

mergify bot commented Feb 23, 2022

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

@mergify
Copy link
Contributor

mergify bot commented Mar 1, 2022

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

@mergify
Copy link
Contributor

mergify bot commented Mar 10, 2022

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

@gabrielluong gabrielluong force-pushed the remove-migration branch 2 times, most recently from 27e8dfe to a85ff7f Compare March 11, 2022 14:38
@mergify
Copy link
Contributor

mergify bot commented Mar 14, 2022

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

@gabrielluong gabrielluong force-pushed the remove-migration branch 2 times, most recently from 5c636df to 7712e1e Compare March 16, 2022 04:18
@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2022

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

@gabrielluong gabrielluong force-pushed the remove-migration branch 2 times, most recently from 5c636df to b73b654 Compare March 16, 2022 04:24
@gabrielluong gabrielluong force-pushed the remove-migration branch 2 times, most recently from 7712e1e to b73b654 Compare March 16, 2022 04:46
grigoryk
grigoryk approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@grigoryk grigoryk left a comment

Choose a reason for hiding this comment

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

👍 🚢

app/build.gradle
@@ -177,14 +177,6 @@ android {
androidTest {
resources.srcDirs += ['src/androidTest/resources']
}
beta {
java.srcDirs = ['src/migration/java']
manifest.srcFile "src/migration/AndroidManifest.xml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Without these overrides, we'll end-up using the standard build variant manifest you've added, e.g. app/app/src/beta/AndroidManifest.xml, right? (which will be merged w/ the main manifest file)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

@gabrielluong gabrielluong added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Mar 28, 2022
@mergify mergify bot merged commit 834d0ee into mozilla-mobile:main Mar 28, 2022
@gabrielluong gabrielluong deleted the remove-migration branch March 28, 2022 14:20
@Amejia481 Amejia481 mentioned this pull request Jul 13, 2022
Merged
4 tasks
@csadilek csadilek mentioned this pull request Feb 14, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@grigoryk grigoryk grigoryk approved these changes

@csadilek csadilek Awaiting requested review from csadilek

@jonalmeida jonalmeida Awaiting requested review from jonalmeida

Assignees

@grigoryk grigoryk

Labels
needs:review PRs that need to be reviewed pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate removing Fennec -> Fenix migration code
4 participants
@gabrielluong @csadilek @grigoryk @pocmo