897247 - [fig] Closing a background tab does not update "Switch to tab" UI in new about:home

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

STR:
1) open some page
2) open a new tab (this will go to about:home, I suppose you may need bug 895816 to land to test this)
3) swipe to VISITED page
4) open tabs tray and close the tab created in step 1
5) close tabs tray and notice "Switch to tab" UI doesn't go away immediately

Comment 1

[:Margaret Leibovic]

Attachment: patch

This appears to work well. I added some logging to Tabs to verify that the number of listeners doesn't explode.

Brian, I'll let you review this, since I already talked to you about one part of it :)

Comment 2

[:Margaret Leibovic]

Comment on attachment 784582 [details] [diff] [review]
patch

Redirecting to wesj, so that bnicholson can enjoy his Aruba vacation.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 784582 [details] [diff] [review]
patch

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

I don't like some of the naming, but looks good otherwise.

::: mobile/android/base/home/TwoLinePageRow.java
@@ +79,5 @@
> +    @Override
> +    public void onTabChanged(final Tab tab, final Tabs.TabEvents msg, final Object data) {
> +        switch(msg) {
> +            case CLOSED:
> +            case LOCATION_CHANGE:

Does ADDED always fire a LOCATION_CHANGE for us?

@@ +122,5 @@
>  
> +    /**
> +     * Replaces the URL with "Switch to tab" if there is already a tab open with the URL.
> +     */
> +    private void updateSwitchToTab(String url) {

This function name doesn't imply to me that this will set the url if this isn't a switch to tab url. Can we just call it "updateDisplayedUrl()"

@@ +124,5 @@
> +     * Replaces the URL with "Switch to tab" if there is already a tab open with the URL.
> +     */
> +    private void updateSwitchToTab(String url) {
> +        int tabId = Tabs.getInstance().getTabIdForUrl(url);
> +        if (!mShowIcons || tabId < 0) {

Likewise, mShowIcons is a pretty horrible name for what this controls (whether or not we allow 'switch to tab').

@@ +156,5 @@
> +        updateSwitchToTab(url);
> +
> +        // Store the page URL, so that we can use it to replace "Switch to tab"
> +        // if the open tab changes or is closed.
> +        mPageUrl = url;

Any reason not to do this in updateSwitchToTab()?

Comment 4

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #3)
> Comment on attachment 784582 [details] [diff] [review]
> patch
> 
> Review of attachment 784582 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't like some of the naming, but looks good otherwise.
> 
> ::: mobile/android/base/home/TwoLinePageRow.java
> @@ +79,5 @@
> > +    @Override
> > +    public void onTabChanged(final Tab tab, final Tabs.TabEvents msg, final Object data) {
> > +        switch(msg) {
> > +            case CLOSED:
> > +            case LOCATION_CHANGE:
> 
> Does ADDED always fire a LOCATION_CHANGE for us?

Tabs.addTab (the method that fires the ADDED event) is only called in two places, when we get a "Tab:Added" message from Gecko, and when we call Tabs.loadUrl. In most cases, we'll also get a location change event, but we won't for tabs that have a delayed load, so we should listen for the ADDED event here. Good catch.

> @@ +122,5 @@
> >  
> > +    /**
> > +     * Replaces the URL with "Switch to tab" if there is already a tab open with the URL.
> > +     */
> > +    private void updateSwitchToTab(String url) {
> 
> This function name doesn't imply to me that this will set the url if this
> isn't a switch to tab url. Can we just call it "updateDisplayedUrl()"

I agree that's better, I'll update the patch.

> @@ +124,5 @@
> > +     * Replaces the URL with "Switch to tab" if there is already a tab open with the URL.
> > +     */
> > +    private void updateSwitchToTab(String url) {
> > +        int tabId = Tabs.getInstance().getTabIdForUrl(url);
> > +        if (!mShowIcons || tabId < 0) {
> 
> Likewise, mShowIcons is a pretty horrible name for what this controls
> (whether or not we allow 'switch to tab').

I didn't question what was already in the code, but you're right that's bad. I can update that as well while I'm here.

> @@ +156,5 @@
> > +        updateSwitchToTab(url);
> > +
> > +        // Store the page URL, so that we can use it to replace "Switch to tab"
> > +        // if the open tab changes or is closed.
> > +        mPageUrl = url;
> 
> Any reason not to do this in updateSwitchToTab()?

I initially did this, but then decided against it for some reason. Maybe because it was weird to pass in mUrl from the call in the event listener? I think it feels safter to set mPageUrl where we got the data for it, and keep the helper method just about updating the UI.

Comment 5

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #4)
> (In reply to Wesley Johnston (:wesj) from comment #3)

> > @@ +124,5 @@
> > > +     * Replaces the URL with "Switch to tab" if there is already a tab open with the URL.
> > > +     */
> > > +    private void updateSwitchToTab(String url) {
> > > +        int tabId = Tabs.getInstance().getTabIdForUrl(url);
> > > +        if (!mShowIcons || tabId < 0) {
> > 
> > Likewise, mShowIcons is a pretty horrible name for what this controls
> > (whether or not we allow 'switch to tab').
> 
> I didn't question what was already in the code, but you're right that's bad.
> I can update that as well while I'm here.

This flag is also used to determine whether or not we show bookmark/reading list icons, so I'm not sure of a better name for it and decided to punt on changing it, since it's not part of this bug anyway.

Pushed to fig with other comments addressed:

https://hg.mozilla.org/projects/fig/rev/847f396b0dde

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/847f396b0dde

Comment 7

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed on:

Builds:
Nightly (2014-05-16)
Aurora (2014-05-16)
Beta 30.0
Release 29.0.1

Device:
Asus Transformer Pad TF300T (Android 4.2.1)