1029046 - Disable recent tabs panel in migration if all panels are disabled

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

This is something we discussed in bug 1004850, but which failed to make it into the migration patch.

Unfortunately, this will have already affected Nightly users who have updated, but we can fix this before this migration moves to Aurora and beyond.

Comment 1

[:Margaret Leibovic]

Actually, we would still want to add the new panel to the config, since it's a built-in panel, but we would just want to make it disabled like all the other panels.

Comment 2

[:Margaret Leibovic]

Attachment: WIP

This is kinda tricky, since we need to first iterate through all the existing panels to make sure that they're all disabled before we decide to make the new panel disabled. This shouldn't be too bad, since users generally don't have more than 10 panels (even that seems extreme), and this only runs once.

I didn't test this, but this should take us in the right direction.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8447520 [details] [diff] [review]
WIP

Review of attachment 8447520 [details] [diff] [review]:
-----------------------------------------------------------------

This is likely to fix bug 1030141 too because we were unconditionally adding the new built-in panel without account for the case where all existing panels were disabled (in which case the new built-in panel would have to be set as default).

::: mobile/android/base/home/HomeConfigPrefsBackend.java
@@ +150,5 @@
> +                    for (int i = 0; i < count; i++) {
> +                        final JSONObject jsonPanelConfig = originalJsonPanels.getJSONObject(i);
> +
> +                        // If any panel is enabled, then we should make the recent tabs panel enabled.
> +                        if (!jsonPanelConfig.optBoolean(JSON_KEY_DISABLED, false)) {

Does this patch even build? JSON_KEY_DISABLED is not currently accessible from the backend.

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Expose HomeConfig JSON keys to other 'home' classes (r=mfinkle)

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: /1030141 - Conditionally enable new recent tabs panel in config migration (r=mfinkle)

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8449425 [details] [diff] [review]
Expose HomeConfig JSON keys to other 'home' classes (r=mfinkle)

We need access to those keys in HomeConfig's backend code.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8449426 [details] [diff] [review]
/1030141 - Conditionally enable new recent tabs panel in config migration (r=mfinkle)

Here's a more complete version of Margaret's patch. Only enable the new built-in panel if the user hasn't disabled all panels.

Comment 8

[Lucas Rocha (:lucasr)]

Steps to verify this bug:
1. Install a nightly built before bug 1030141 landed with a fresh profile.
2. Disable all built-in panels in Settings -> Customize -> Home.
3. about:home will only show the Firefox watermark now.
3. Install new nightly with this fix included.
4. You should still see the same Firefox watermark in about:home.

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to Lucas Rocha (:lucasr) from comment #8)
> Steps to verify this bug:
> 1. Install a nightly built before bug 1030141 landed with a fresh profile.

Err, I meant bug 1004850.

Comment 10

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/f93d3036c5a4
https://hg.mozilla.org/integration/fx-team/rev/34e8a8a7603b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f93d3036c5a4
https://hg.mozilla.org/mozilla-central/rev/34e8a8a7603b

Comment 12

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: LG Nexus 4
OS: Android 4.4.2
I have installed the 22-06 build, disabled all panels (the watermark appears in about:home), update to 04-07 build and the Firefox watermark is still present in about:home.