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

Issue #21642: Remove in-progress media tab from homescreen #21670

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

jonalmeida
Copy link
Contributor

Not removing the other logic because we're going to re-add this after some refinement in the future.

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

@jonalmeida jonalmeida added the needs:review PRs that need to be reviewed label Oct 3, 2021
@jonalmeida jonalmeida requested review from a team as code owners October 3, 2021 03:19
@@ -24,16 +24,9 @@ import kotlin.math.max
fun BrowserState.asRecentTabs(): List<RecentTab> {
return mutableListOf<RecentTab>().apply {
val lastOpenedNormalTab = lastOpenedNormalTab
val inProgressMediaTab = inProgressMediaTab
Copy link
Member

Choose a reason for hiding this comment

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

We could also wrap this behind a feature flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure to what extent we should be "removing it". So I opted for this solution so we can land it before the beta cut.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another reason I forgot about, is that we get intermittent test failures when we have feature flags here. See #21557.

Copy link
Contributor

@csadilek csadilek Oct 4, 2021

Choose a reason for hiding this comment

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

Let's land as is. Thought about this last week and it's a trade-off. We don't want feature flags to be too fine-grained, so arguably this is a change in behaviour of an existing feature, as opposed to a feature itself. We can revert this to enable again when we want it (have time to add media controls).

@gabrielluong gabrielluong self-assigned this Oct 4, 2021
@gabrielluong gabrielluong added pr:approved PR that has been approved and removed needs:review PRs that need to be reviewed labels Oct 4, 2021
@@ -24,16 +24,9 @@ import kotlin.math.max
fun BrowserState.asRecentTabs(): List<RecentTab> {
return mutableListOf<RecentTab>().apply {
val lastOpenedNormalTab = lastOpenedNormalTab
val inProgressMediaTab = inProgressMediaTab
Copy link
Contributor

@csadilek csadilek Oct 4, 2021

Choose a reason for hiding this comment

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

Let's land as is. Thought about this last week and it's a trade-off. We don't want feature flags to be too fine-grained, so arguably this is a change in behaviour of an existing feature, as opposed to a feature itself. We can revert this to enable again when we want it (have time to add media controls).

@csadilek csadilek merged commit 8c2cbb4 into mozilla-mobile:main Oct 4, 2021
@jonalmeida jonalmeida deleted the issue-21642 branch October 4, 2021 22:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:approved PR that has been approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants