Closed Bug 1263571 Opened 8 years ago Closed 8 years ago

"Switch-to-tab" should not be displayed in "Reading List" folder when the page is not opened in another tab

Categories

(Firefox for Android Graveyard :: Reader View, defect)

48 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox48 verified, fennec48+)

VERIFIED FIXED
Firefox 48
Tracking Status
firefox48 --- verified
fennec 48+ ---

People

(Reporter: TeoVermesan, Assigned: ahunt)

References

Details

Attachments

(4 files)

Tested using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 48.0a1 (2016-04-10)

Steps to reproduce:
1. Open some pages from "Reading list" folder from Bookmarks panel
2. Close them
3. Go back to the "Reading list" folder

Actual results:
- "Switch to tab" is displayed.

Expected results:
- "Switch to tab" should not be displayed.
Blocks: migrate-RL
Assignee: nobody → ahunt
tracking-fennec: --- → ?
I can only reproduce this if I keep the bookmarks panel open while closing opened reader view bookmarks. It seems we do receive tab-closing events in the Bookmarks panel (if you close a normal bookmark, the "switch to tab" disappears), but they're not handled correctly for reader view bookmarks.

I think the issue is that we don't store the about:reader url when showing bookmarks, we convert into an about:reader url when actually opening a reader view bookmark. But tab-close events will contain the about:reader url, the two don't match, and we don't change the state of these reader view bookmarks.
We're always using this as a String, we might as well make this explicit to avoid having to cast
anywhere. (We very rarely use the parameter, but some new code in the main part of Bug 1263571
would have to cast this to a String. We can avoid that if we just use the correct type.)

Review commit: https://reviewboard.mozilla.org/r/45869/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45869/
Status: NEW → ASSIGNED
tracking-fennec: ? → 48+
Comment on attachment 8740664 [details]
MozReview Request: Bug 1263571 - Update switch-to-tab for reader view pages too r?liuche

https://reviewboard.mozilla.org/r/45871/#review45009
Attachment #8740664 - Flags: review?(liuche) → review+
Comment on attachment 8740663 [details]
MozReview Request: Bug 1263571 - Pre: onTabChanged data is always a String, declare it as such r?liuche

https://reviewboard.mozilla.org/r/45869/#review45001

I'm a little ambivalent about this change because it seems like a lot of churn for something really small but - this is probably named this way to match JS convention of listeners and interfaces, and if it hasn't been used otherwise for this long, I'm okay switching it to a String. If that changes, we could always use a JSON object, or some Java object. In general casting is jest not very explicit and often just leads to exceptions.

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:584
(Diff revision 1)
>              }
>          });
>      }
>  
>      public interface OnTabsChangedListener {
> -        public void onTabChanged(Tab tab, TabEvents msg, Object data);
> +        public void onTabChanged(Tab tab, TabEvents msg, String data);

IntelliJ tells me this public modifier is unnecessary, so might as well remove it while you're here.
Attachment #8740663 - Flags: review?(liuche) → review+
Switch-to-tab" is not displayed anymore in "Reading List" folder if the page is not opened in another tab 
Tested using:
Device: Samsung Galaxy S7 Edge (Android 6.0)
Build: Firefox for Android 49.0a1 (2016-05-03)
I'm changing the status to verified fixed considering that the bug is verified.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.