690816 - [tabletui] When Fennec starts, remember whether the landscape sidebar was open or closed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce:
1. Open fennec in landscape tablet mode.
2. Hide the tab sidebar
3. Quit fennec
4. Open fennec in landscape tablet mode again

Expected results: Sidebar is still hidden.
Actual results: Sidebar is visible.

I think the sidebar state should be stored in a preference that is persistent across browsing sessions.  This lets people who don't want the sidebar to hide it once, instead of having to hide it every time the browser starts.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: 1/2: refactor TabsPopup show/hide methods

No behavior change; this is just a minor rearrangement of the current code to be easier to understand and modify.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: 2/2: Store the sidebar state in a persistent pref

This adds a tiny bit of code that runs at startup, so there's a slight risk of performance impact.  I haven't tested this on device yet.

Comment 3

[Matt Brubeck (:mbrubeck)]

Wes points out on IRC that the "persist" attribute has the same effect.  I'm not sure whether this would be better or worse than a pref:
https://developer.mozilla.org/en/XUL/Attribute/persist

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: 1/2: refactor TabsPopup show/hide methods (v2)

Minor update to use the new methods from browser.js.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: 1/2: refactor TabsPopup show/hide methods (v3)

No code changes, just rebased on top of bug 690973.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: 2/2: Store the sidebar state in a persistent pref (v2)

Rebased.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> Wes points out on IRC that the "persist" attribute has the same effect.  I'm
> not sure whether this would be better or worse than a pref:
> https://developer.mozilla.org/en/XUL/Attribute/persist

persist="value" would be a more XUL-ish to do it and since you are tracking an attribbute ("tablet_sidebar") it should work OK. Less code would be nice. persist might have the same "not saved when killed by OS" issues as the preference too.

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 563908 [details] [diff] [review]
2/2: Store the sidebar state in a persistent pref (v2)

I'll try this the "persist" way before requesting review.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 563906 [details] [diff] [review]
1/2: refactor TabsPopup show/hide methods (v3)

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

I wonder what are the cases where we actually need to specifically use showMenu/hideMenu or showSidebar/hideSidebar instead of the global hide/show methods. Why not simply use hide/show everywhere and hide the implementation details for portrait and landscape modes?

I'm giving a review+ here but I'd really like to have TabsPopup renamed to something less misleading before pushing this.

::: mobile/chrome/content/TabsPopup.js
@@ +141,5 @@
>        window.removeEventListener("resize", this.resizeHandler, false);
>      }
>    },
>  
> +  showSidebar: function showSidebar() {

I find it a bit awkward that something called TabsPopup has methods called showSidebar/hideSidebar. Maybe TabsPopup should have a different name like TabsUI or TabsList or something.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #9)

> I find it a bit awkward that something called TabsPopup has methods called
> showSidebar/hideSidebar. Maybe TabsPopup should have a different name like
> TabsUI or TabsList or something.

Don't forget TabsHelper! We love helpers in Fennec :)

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: 1/2: refactor TabsPopup and tab sidebar methods (v4)

This moves all of the tablet sidebar methods together into a new object, TabletSidebar.  TabsPopup once again deals only with the popup menu.

As a bonus, grabSidebar() and associated methods are now loaded lazily, so they won't even be parsed on non-tablet devices.

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: 2/2: Persist the sidebar state

Use the "persist" attribute to remember the state automatically.

Most of the changes here are necessary because "persist" won't work on removed attributes, so we need to set it to "false" instead.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 564198 [details] [diff] [review]
1/2: refactor TabsPopup and tab sidebar methods (v4)

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

This is much much nicer! :-)

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 564202 [details] [diff] [review]
2/2: Persist the sidebar state

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

Cool.

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6ca22334209
https://hg.mozilla.org/integration/mozilla-inbound/rev/290504bc9669

Comment 16

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/f6ca22334209
https://hg.mozilla.org/mozilla-central/rev/290504bc9669

Comment 17

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111004 Firefox/10.0a1 Fennec/10.0a1

Comment 18

[Andreea Pod]

Testcase added: https://litmus.mozilla.org/show_test.cgi?id=33521