1259078 - Remove the migrated reading-list panel in 49

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Andrzej Hunt :ahunt]

I can't remember if we made this decision anywhere:

In Bug 1234316 we'll show a migration message in the reading list panel.

(1) Do we want to hide the reading-list panel by default for new users? The migration message seems unnecessary in this case.
(2) Do we want to hide the reading-list panel for existing users if their reading-list panel was empty?

I don't have much knowledge of our homepanel config code yet, so I have no idea how complicated this might be.

Comment 1

[Andrzej Hunt :ahunt]

This can be landed after the main migration code if needed, so there's no rush to decide this immediately. The main issue is around testing scenario (2) to ensure it doesn't have issues (if we land this separately most of the nightly population will have already run the migration, meaning we get less testing).

Comment 2

[:Margaret Leibovic]

Ideally I think we should hide the panel for users who don't have any reading list items, especially for new users.

However, the HomeConfig code is a bit tricky, so I'm not sure if there's a straightforward way to do this. Another issue to consider is that even if we hide the panel, it may still appear in the list of panels in the home panel settings. We should try to avoid that if possible.

The first idea that comes to mind is that we could set a pref to indicate whether or not we should continue to show the panel while we're performing the DB migration, then check that pref where we add the panel to the default config here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java#85

However, this won't affect users who have customized their home panels. For this case, we'll need to create a home panel migration, but the logic can be similar in that case. However, that means we'll need yet another migration for the next release, when we decide to remove the code for the panel (but we'll need this no matter what).

Comment 3

[Andrzej Hunt :ahunt]

I doubt I'll have time to get this done in time for 48. Given the complexities of the HomeConfig code it would probably be a bad idea to try and rush this: I'll transform this bug to instead remove the reading-list panel for 49 (we'll probably want plenty of testing to cover all the edge-cases there).

Comment 4

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1259078 - Remove reading list panel from HomePanels r?liuche

Review commit: https://reviewboard.mozilla.org/r/50605/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50605/

Comment 5

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1259078 - Post: remove unneeded reading list panel resources and code r?liuche

Review commit: https://reviewboard.mozilla.org/r/50607/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50607/

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8748910 [details]
MozReview Request: Bug 1259078 - Remove reading list panel from HomePanels r?liuche

https://reviewboard.mozilla.org/r/50605/#review47681

One big error on ids, but this is okay once you fix that.

I'd like you to run through the upgrade chains (to make sure this works in the cases of hidden, default, normal reading list). And a try push would be appreciated.

::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfig.java:1605
(Diff revision 1)
> -    private static final String READING_LIST_PANEL_ID = "20f4549a-64ad-4c32-93e4-1dcef792733b";
>      private static final String HISTORY_PANEL_ID = "f134bf20-11f7-4867-ab8b-e8e705d7fbe8";
>      private static final String COMBINED_HISTORY_PANEL_ID = "4d716ce2-e063-486d-9e7c-b190d7b04dc6";
>      private static final String RECENT_TABS_PANEL_ID = "5c2601a5-eedc-4477-b297-ce4cef52adf8";
>      private static final String REMOTE_TABS_PANEL_ID = "72429afd-8d8b-43d8-9189-14b779c563d0";
> +    private static final String DEPRECATED_READING_LIST_PANEL_ID = "20f4549a-64ad-4c32-93e4-1dcef792733b";

This is incorrect. You need to keep the old panel id, because otherwise we won't be able to associate an id that we receive with "reading list".

::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java:266
(Diff revision 1)
> +        }
> +
> +        if (wasDefault) {
> +            // This will make the bookmarks panel visible if it was previously hidden - this is desired
> +            // since this will make the new equivalent of the reading list visible by default.
> +            final JSONObject bookmarksPanelConfig = createBuiltinPanelConfig(context, PanelType.BOOKMARKS, EnumSet.of(PanelConfig.Flags.DEFAULT_PANEL)).toJSON();

This is a little more explicit (alternatively, you could getFlags from the Reading List panel if it was the default, and for this if statement check if the defaultFlags is null or not - however I prefer explicitness to saving a tiny bit of memory).

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8748911 [details]
MozReview Request: Bug 1259078 - Post: remove unneeded reading list panel resources and code r?liuche

https://reviewboard.mozilla.org/r/50607/#review49301

Nice catching all the references.

Comment 8

[Andrzej Hunt :ahunt]

I've (re-)tested the following scenarios, so I think we're safe to land:
- default (i.e. top-sites is default home-panel) -> reading list panel disappears, everything else behaves as expected
- reading-list panel visible, set as default -> reading list panel disappears, bookmarks is new default
- reading-list panel hidden, top-sites as default -> reading list panel stays hidden, no longer available in settings.

Comment 9

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/ec0ef38e685953d92aaffa76a64dc62b7e745bbd
Bug 1259078 - Remove reading list panel from HomePanels r=liuche

https://hg.mozilla.org/integration/fx-team/rev/7973ef3e90b16e1d59777148a6eea6fa296a9d09
Bug 1259078 - Post: remove unneeded reading list panel resources and code r=liuche

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ec0ef38e6859
https://hg.mozilla.org/mozilla-central/rev/7973ef3e90b1

Comment 11

[Teodora Vermesan (:TeoVermesan)]

Reading list Panel is no longer displayed, so:
Verified as fixed using:
Device: Moto X (Android 4.4)
Build: Firefox for Android 49.0a1 (2016-05-18)