Closed Bug 1263800 Opened 4 years ago Closed 4 years ago

New install issue, Bookmark actions stop working after Browser dismiss

Categories

(Firefox for Android :: General, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox45 --- unaffected
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 --- verified
fennec 48+ ---

People

(Reporter: capella, Assigned: ahunt)

References

Details

(Keywords: regression)

Attachments

(1 file)

On my test device, with current m-c repo, clobber, full/non-artifact build and fresh install, toggling Bookmarks star in the menu works properly, until after I swipe FF closed and then re-start it.

After that, I seem to lose Bookmarks toggle action.

[0] https://www.dropbox.com/s/7lilmociia06834/bug_newInstall.mp4?dl=0
Duplicate of this bug: 1264261
It seems like this only affects small tablets.
tracking-fennec: --- → ?
Assignee: nobody → ahunt
FWIW I can reproduce this on an N9 too (large tablet, which shows the full tablet UI).

I have a suspicion that this is a side effect of:
https://hg.mozilla.org/mozilla-central/rev/870df3ae9853

Which interacts with us disabling menu items if the tab == null at:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#3188
If tab data is initially null then we disable the bookmark star. We need to explicitly
reenable it when we load an actual page. All other menu items seem to be explicitly
enabled as needed below, only bookmarks was omitted (since it's expected to be enabled
~all the time) - but we still could have disabled it previously.

Review commit: https://reviewboard.mozilla.org/r/46495/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46495/
Attachment #8741450 - Flags: review?(gkruglov)
Status: NEW → ASSIGNED
Priority: -- → P2
Comment on attachment 8741450 [details]
MozReview Request: Bug 1263800 - Ensure bookmark star is enabled if tab data available r?grisha

https://reviewboard.mozilla.org/r/46495/#review43093

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3188
(Diff revision 1)
>                                  !PrefUtils.getStringSet(GeckoSharedPrefs.forProfile(this),
>                                                          ClearOnShutdownPref.PREF,
>                                                          new HashSet<String>()).isEmpty();
>          aMenu.findItem(R.id.quit).setVisible(visible);
>  
>          if (tab == null || tab.getURL() == null) {

I would add a comment here stating explicitely that when we don't have tab data, we disable bunch of things and early return out of this function. Return being the key bit of information.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3210
(Diff revision 1)
>              MenuUtils.safeSetEnabled(aMenu, R.id.add_search_engine, false);
>              MenuUtils.safeSetEnabled(aMenu, R.id.add_to_launcher, false);
>  
>              return true;
>          }
>  

And here, to make code easier to parse quickly, I'd add a comment stating that if we DO have tab data, let's re-enable things as necessary and set proper checked/visible/etc values.

Otherwise, since the above `if` clause returns and doesn't have an `else`, if you miss that little return statement, you're spending some amount (small yet i'll never get it back!!) time trying to figure what the hell is this doing.
Attachment #8741450 - Flags: review?(gkruglov) → review+
(In reply to Andrzej Hunt :ahunt from comment #4)
> FWIW I can reproduce this on an N9 too (large tablet, which shows the full
> tablet UI).
> 
> I have a suspicion that this is a side effect of:
> https://hg.mozilla.org/mozilla-central/rev/870df3ae9853
> 
> Which interacts with us disabling menu items if the tab == null at:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/BrowserApp.java#3188

Oops I'm wrong, the cause is much simpler, I removed the setEnabled(!isReaderView) call instead of making it setEnabled(true).
https://hg.mozilla.org/integration/fx-team/rev/1a22155b5544b1690270b7a3f9e93e495ed4cdd8
Bug 1263800 - Ensure bookmark star is enabled if tab data available r=grisha
https://hg.mozilla.org/mozilla-central/rev/1a22155b5544
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
tracking-fennec: ? → 48+
Verified as fixed in build 48.0a1 2016-04-21;
Device: Asus ZenPad 8 (Android 5.0.2);
Status: RESOLVED → VERIFIED
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.