Closed Bug 1259078 Opened 6 years ago Closed 6 years ago

Remove the migrated reading-list panel in 49


(Firefox for Android Graveyard :: Awesomescreen, defect)

Not set


(firefox48 affected, firefox49 verified)

Firefox 49
Tracking Status
firefox48 --- affected
firefox49 --- verified


(Reporter: ahunt, Assigned: ahunt)




(2 files)

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.
Assignee: nobody → ahunt
Blocks: migrate-RL
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).
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:

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).
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).
Summary: Do we want to hide the "migrated" reading-list panel for new users, and/or users without reading-list items? → Remove the migrated reading-list panel in 49
Attachment #8748910 - Flags: review?(liuche) → review+
Comment on attachment 8748910 [details]
MozReview Request: Bug 1259078 - Remove reading list panel from HomePanels r?liuche

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/
(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/
(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 on attachment 8748911 [details]
MozReview Request: Bug 1259078 - Post: remove unneeded reading list panel resources and code r?liuche

Nice catching all the references.
Attachment #8748911 - Flags: review?(liuche) → review+
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.
Bug 1259078 - Remove reading list panel from HomePanels r=liuche
Bug 1259078 - Post: remove unneeded reading list panel resources and code r=liuche
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.