1332955 - Previously open bookmark folder no longer restored when going back

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Jan Henning [:JanH]]

STR:
0. Sync with desktop (or copy browser.db from some other profile) so you've got some bookmark folders available.
1. Open a new tab.
2. Go down a few levels through your bookmark folders and subfolders
3. Click on a bookmark to open a link.
4. Go back.

Expected:
about:home opens at the bookmark folder you had open at the end of step 2. (as implemented through bug 1060544)

Actual:
about:home shows the top of the bookmarks hierarchy containing the "Desktop Bookmarks" folder and your mobile bookmarks.

It's still working in Firefox 50, but broken on Nightly. I haven't had time for a deeper look yet, but a quick look in the debugger shows that on going back, we're still passing the correct saved bookmarks stack through here (https://dxr.mozilla.org/mozilla-central/rev/3cedab21a7e65e6a1c4c2294ecfb5502575a46e3/mobile/android/base/java/org/mozilla/gecko/home/BookmarksListAdapter.java#141), only somehow we then fail to make correct use of it.

Comment 1

[Jan Henning [:JanH]]

Also it looks like the correct folder might sill be visible for a split second, only to be immediately replaced by the bookmark folder root.

Comment 2

[Jan Henning [:JanH]]

Mozregression narrowed it down to [2017-01-19, 2017-01-20] (https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a3978751f45108ff1a
e002ecebdc0fa23fc52b84&tochange=3cedab21a7e65e6a1c4c2294ecfb5502575a46e3). Unfortunately the builds in between are missing, so I have to continue with a manual bisection.

Comment 3

[Jan Henning [:JanH]]

Seems like the changes in HomePager.java are to blame here.

Comment 4

[Nevin Chen(Not active on Bugzilla)]

I guess I should consider restoreData here http://searchfox.org/mozilla-central/rev/ed04f7a44925dace34f2d2e15ef9c0f2809d70b0/mobile/android/base/java/org/mozilla/gecko/home/HomePager.java#229 as well. I'll check this more tomorrow.

Comment 5

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1332955 - Consider restore data( for bookmark parent stack) when reloading home panels.

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

Comment 6

[Jan Henning [:JanH]]

Comment on attachment 8830138 [details]
Bug 1332955 - Consider restore data( for bookmark parent stack) when reloading home panels.

https://reviewboard.mozilla.org/r/107036/#review108354

Seems to work, but I can't really say whether that's the best solution (including for the original problem) or not.

::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:228
(Diff revision 1)
>  
>          // Don't show the tabs strip until we have the
>          // list of panels in place.
>          mTabStrip.setVisibility(View.INVISIBLE);
>  
> -        // If HomeConfigLoader already exist, force load to select the current item
> +        // If HomeConfigLoader already exist and there's no restoreData(for bookmark's parentStack) , force load to select the current item

Nit: No space required before the comma and more importantly, needs a line break.

Comment 7

[Gary Sanders]

On Firefox Android version 50.1.0, I can confirm that the test described at the top of this report works. However, the following test does not work (perhaps it never worked, don't know). In any event, the open bookmarks folder location is NOT remembered:

[1] Open Firefox for Android and navigate to a bookmark sub folder.
[2] Open one of the links in the bookmark sub folder in a new tab.
[3] After the page loads in a new tab, click on the tab where the page has loaded.
[4] Click on the tab from which the bookmarks were natigated to in step #1. ==> The current bookmark folder location is forgotten and goes back to the root bookmark folder level. I would expect Firefox to remember the bookmark folder location.

It is annoyances like this that make Firefox for Android not so user friendly. Surely the test I just described would have just worked if a more robust solution would have been implemented originally.

Comment 8

[Jan Henning [:JanH]]

(In reply to Gary Sanders from comment #7)
> On Firefox Android version 50.1.0, I can confirm that the test described at
> the top of this report works. However, the following test does not work

Opened bug 1334919 for that issue.

Comment 9

[Nevin Chen(Not active on Bugzilla)]

(In reply to Jan Henning [:JanH] from comment #6)
> Seems to work, but I can't really say whether that's the best solution
> (including for the original problem) or not.

HI Jan
Sorry I couldn't find you on IRC so I just reply here. Could you please elaborate more what's your concern about the patch? Thanks a lot!

Comment 10

[Nevin Chen(Not active on Bugzilla)]

(In reply to Jan Henning [:JanH] from comment #6)
> Seems to work, but I can't really say whether that's the best solution
> (including for the original problem) or not.

HI Jan
Sorry I couldn't find you on IRC so I just reply here. Could you please elaborate more what's your concern about the patch? Thanks a lot!

Comment 11

[Jan Henning [:JanH]]

Just to reiterate, the original change from bug 1275662 was intended for the case where the home panels are initially hidden (the selected tab is showing some content) and then we close all open tabs from the Settings screen via "Clear private data", meaning a new about:home tab is opened in background while the Settings activity is still in the foreground.

As this bug shows however, the forced load is now triggered even when it's not strictly necessary or even potentially harmful, so this feels a bit like piling a workaround on top of another workaround. Since I'm not familiar with this code, I can't suggest a better alternative, though.

Although at least for the current situation this should be okay - the restore data is saved per tab, so when you close all tabs from the Settings screen, there won't be any restore data, allowing the force load to always proceed for the case where it is actually necessary (home panels become visible while we're in the Settings). As long as ahunt is okay with this, I've got no absolute objection here - just wondering out loud whether there might or might not be an immediate alternative solution...

Comment 12

[Nevin Chen(Not active on Bugzilla)]

Hi Jan
Whenever we hide the HomePager, we'll clear the HomePager's adapter[1]
Since we can only retrieve HomePager's adapter content by HomeConfigLoader, but coming back from another activity won't trigger the loader's onLoadFinished[2], HomePager UI's adapter will not be set[3]. That's the reason I call forceLoad().

The side effect of always calling forceLoad() is it will regardless the restoreData when navigation within the same activity. That's the solution I come up with this patch.

There were another two solutions but I didn't use them and the reasons are below:
1. Cache the configSate from the loader[2] and update the adapter directly if configSate already retrieve(e.g. call updateUiFromConfigState(mConfigState) instead of forceLoad() ).
--> But this will need to maintain configSate in HomePager. I think it's better for the loader to presver the state for encapsulation.

2. Not clearing the adapter[5] when we hide HomePager, thus we can preserve the adapter state and don't have to care if loader has called the callback(onLoadFinished) or not.
--> This will need to change some original design( bug 868553 and bug 909618)..I'll suggest creating another bug if we really want to go for this approach.

I really appreciate your comments. I've learned a lot!

[1] http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/mobile/android/base/java/org/mozilla/gecko/home/HomePager.java#261
[2] http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/mobile/android/base/java/org/mozilla/gecko/home/HomePager.java#505
[3] http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/mobile/android/base/java/org/mozilla/gecko/home/HomePager.java#406
[4] http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/mobile/android/base/java/org/mozilla/gecko/home/HomePager.java#230
[5] http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/mobile/android/base/java/org/mozilla/gecko/home/HomePager.java#264

Comment 13

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8830138 [details]
Bug 1332955 - Consider restore data( for bookmark parent stack) when reloading home panels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107036/diff/1-2/

Comment 14

[Jan Henning [:JanH]]

Thanks for taking the time and enlightening me. Yes, you're right, in view of this I've got no further objections - the main priority here is to get this regression fixed.

Comment 15

[Andrzej Hunt :ahunt]

Comment on attachment 8830138 [details]
Bug 1332955 - Consider restore data( for bookmark parent stack) when reloading home panels.

https://reviewboard.mozilla.org/r/107036/#review111268

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ff3ef5e885a
Consider restore data( for bookmark parent stack) when reloading home panels. r=ahunt

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ff3ef5e885a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 19

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8830138 [details]
Bug 1332955 - Consider restore data( for bookmark parent stack) when reloading home panels.

Approval Request Comment
[Feature/Bug causing the regression]: Should consider bookmark parent stack when loading home panels.
[User impact if declined]: When user will lose the bookmarks state.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:no
[Is the change risky?]:no
[Why is the change risky/not risky?]: only check for data to restore. Other logic is the same.
[String changes made/needed]: no

Comment 20

[Carmen Fat [:cfat] - Ecosystem QA, PTO until May 6 - NI to: [e-mail]]

Tested on latest Nightly build 54.0a1 (2017-02-08) using following devices:
- Huawei Honor 5x (Android 5.1.1);
- Nexus 9 (Android 7.1.1).

Following the STR from Description, Nightly redirects the user to the previously opened bookmark subfolder.
I'm marking this as Verified for Nightly 54.

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8830138 [details]
Bug 1332955 - Consider restore data( for bookmark parent stack) when reloading home panels.

Fix a bookmark issue and was verified. Aurora53+.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8eb9b71cc50a

Comment 23

[Carmen Fat [:cfat] - Ecosystem QA, PTO until May 6 - NI to: [e-mail]]

Tested on latest Aurora build 53.0a2 (2017-02-15) using following devices:
- Huawei MediaPad M2 (Android 5.1.1);
- Samsung Galaxy Note 4 (Android 5.0.1).

Following the STR from Description, Aurora redirects the user to the previously opened bookmark subfolder.
I'm marking this as Verified for Aurora 53.