837815 - Reading list button opens Top Sites, not Reading List

Status: RESOLVED FIXED

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

Description

[Deb Richardson [:dria] (plz NEEDINFO)]

Summary pretty much covers it -- in today's nightly (and yesterday's) the Reading List button in Readability Mode opens the Top Sites list, not the Reading List.

Comment 1

[Aaron Train [:aaronmt]]

Looks like this regression happened some time ago and not recently.

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v1)

Not sure how this might have originally worked ...

Setting mAwesomeTabs.setCurrentTabByTag() in AwesomeBar.java doesn't appear to have an effect, where setting the mViewPager.setCurrentItem(1) seems to produce the correct response.

(How would setting current tab of TabHost have selected current item of ViewPager?)

fyi log:
   D/XYZZY   (22595): AwesomeBar.java - onCreate() - step 083 bookmarksTab.getTag(): bookmarks
   D/XYZZY   (22595): AwesomeBar.java - onCreate() - step 084 mAwesomeTabs.getCurrentTab(): -1
   D/XYZZY   (22595): AwesomeBar.java - onCreate() - step 085 mAwesomeTabs.getCurrentTab(): -1
   D/XYZZY   (22595): AwesomeBarTabs.java - XYZZY() *2* - mViewPager.getCurrentItem(): 0
   D/XYZZY   (22595): AwesomeBarTabs.java - XYZZY() *2* - mViewPager.getCurrentItem(): 1
   D/XYZZY   (22595): AwesomeBar.java - onCreate() - step 085 mAwesomeTabs.getCurrentTab(): -1

Comment 3

[Ioana Chiorean]

There is already a TC in Moztrap for this: https://moztrap.mozilla.org/manage/case/1267/ 

Should there be created one for negative behaviour? "Make sure the reader List button goes to Bookmarks -> Reading list not to Top Sites nor History"

I think the existing one is ok.

Comment 4

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

Comment on attachment 710474 [details] [diff] [review]
Patch (v1)

Sending to Wes since he has more knowledge about the viewpager changes

Comment 5

[Mark Capella [:capella]]

Attachment: Patch (v2)

Ok, and the previous comment of mine wasn't very clear, I was wondering if this is really what we need ...

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to Mark Capella [:capella] from comment #5)
> Created attachment 711415 [details] [diff] [review]
> Patch (v2)
> 
> Ok, and the previous comment of mine wasn't very clear, I was wondering if
> this is really what we need ...

Drive-by: according to Android's source code, this is exactly what TabHost's setCurrentTabByTag() does. I would be surprised if overriding this method with with the exact same code would make any difference. Does the patch fix the bug for you?

Comment 7

[Mark Capella [:capella]]

yes

Comment 8

[Mark Capella [:capella]]

somethings not the same ... see comment 2 ... mAwesomeTabs.getCurrentTab(): returns -1 prior to and after the call to the un-overridden method

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 711415 [details] [diff] [review]
Patch (v2)

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

I have no idea why this patch would work bug 750167 removes this filter("") call which might help somewhat... but really AwesomeBarTabs.java should just die. Its a useless abstraction for us at this point.

Comment 10

[Alex Keybl [:akeybl]]

Our final build of FF19 is tomorrow - we can't risk regression here. We'll release note instead.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 711415 [details] [diff] [review]
Patch (v2)

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

Oh wait. I didn't realize what was going on here. So we used to actually use this TabHost, and I think maybe I had to keep it around in order to have the TabWidget (TabWidget MUST be inside a TabHost or it throws). But you're right, it has no connection to the ViewPager really. Sorry for the confusion.

Comment 12

[Mark Capella [:capella]]

push to TRY
https://tbpl.mozilla.org/?tree=Try&rev=1a794a872a09

Comment 13

[Mark Capella [:capella]]

And on to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/64df2203966e

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64df2203966e

Comment 15

[Andreea Pod]

Verified fixed on:
-build: Firefox for Android 21.0a1 (2013-02-17)
-device: Samsung Galaxy Nexus
-OS: Android 4.2.1

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Let's get this uplifted to FF 20 on Beta, if low risk.

Comment 17

[Mark Capella [:capella]]

Comment on attachment 711415 [details] [diff] [review]
Patch (v2)

[Approval Request Comment] - This was lost somewhere along the way due to related code changes and overlooked.
Bug caused by (feature/regressing bug #): Not identified ... just corrected.
User impact if declined: Less friendly screen navigation.
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): None - low level UI change ... easy to backout / repair.
String or UUID changes made by this patch: None.

Comment 18

[bhavana bajaj [:bajaj]]

Comment on attachment 711415 [details] [diff] [review]
Patch (v2)

low risk well baked patch which helps with expected user behavior on screen navigation. Approving for uplift.

Comment 19

[Aaron Train [:aaronmt]]

Works now, thanks Capella; verified (03/01) m-c.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/640b31a66dfe

Comment 21

[bhavana bajaj [:bajaj]]

Kevin got to our attention Bug 847475 will be affected, if we take the patch in this bug in Fx20 beta 3(going to build today).We are requesting a backout asap of this patch here to avoid the issue.

Comment 22

[Mark Capella [:capella]]

Attachment: Backout for bug847475 regression

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 23

[Sriram Ramasubramanian [:sriram]]

Backed out: https://hg.mozilla.org/releases/mozilla-beta/rev/de200581d19b

Comment 24

[bhavana bajaj [:bajaj]]

Comment on attachment 721337 [details] [diff] [review]
Backout for bug847475 regression

marking this patch as obsolete, as sriram already helped with the backout !

Comment 25

[Sriram Ramasubramanian [:sriram]]

Backed out: https://hg.mozilla.org/releases/mozilla-aurora/rev/7ae807a17e6c

Comment 26

[Mark Capella [:capella]]

And finally backout from inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/a09c6489a9fd

Comment 27

[Ed Morley [:emorley]]

(In reply to Mark Capella [:capella] from comment #26)
> And finally backout from inbound
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a09c6489a9fd

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a09c6489a9fd

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Any new attempts to fix this issue (that do not cause a regression like in bug 847475) will have to be tested & nominated before Monday March 10th so that we can put it in beta 4 and get more bake time/testing in the wild prior to release.

Comment 29

[Mark Capella [:capella]]

Thanks ... I've got a more direct patch I'm privately testing / baking currently ...

Comment 30

[Mark Capella [:capella]]

Attachment: Patch (v3)

The original patch contained correct code to open the reading list, but over-rode the classes setCurrentTabByTag() method which accidentally impacted AwesomeBarTabs.java's filter() method.

This solution isolates the new code to the specific condition where it's invoked by the user pressing the reader list item on the readermode screen.

Comment 31

[Mark Capella [:capella]]

apk for testing
https://www.dropbox.com/s/ppuhty587lunmnr/fennec-22.0a1.en-US.android-arm.apk

Comment 32

[Aaron Train [:aaronmt]]

Build above with the patch works for me - (LG Nexus 4, Android 4.2.2).

Comment 33

[Wesley Johnston (:wesj)]

Comment on attachment 722348 [details] [diff] [review]
Patch (v3)

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

::: mobile/android/base/AwesomeBarTabs.java
@@ +20,5 @@
>  import android.widget.TabHost;
>  import android.widget.TabWidget;
>  
>  public class AwesomeBarTabs extends TabHost
> +                            implements LightweightTheme.OnChangeListener {

There are some whitespace changes in here that aren't really realted.

Comment 34

[Mark Capella [:capella]]

Pushed to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=4a4332dfad38

Comment 35

[Mark Capella [:capella]]

And on to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87e22489d39d

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

Let's get the uplift nomination ready for this new version of the patch so when it's verified on m-c we can get it uplifted in time for the Beta going to build today.

Comment 37

[Mark Capella [:capella]]

Comment on attachment 722348 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Undetermined regression.
User impact if declined: Degraded user experience.
Testing completed (on m-c, etc.): Tested locally by developer, posted build tested independently, previous regression caused by this patch corrected / verified.
Risk to taking this patch (and alternatives if risky): Should be extremely minimal, critical backout amounts to one-line change.
String or UUID changes made by this patch: None.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87e22489d39d

Comment 39

[Kevin Brosnan [Ex-Mozilla]]

Verified on Firefox for Android mozilla-central tinderbox build https://hg.mozilla.org/mozilla-central/rev/79b8e0a0bdb7

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a2a4b7086c98

Does this need to go on Aurora too?

Comment 42

[bhavana bajaj [:bajaj]]

(In reply to Mark Capella [:capella] from comment #35)
> And on to inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/87e22489d39d

Can you please nominate the patch for uplift on aurora as well ? Thanks

Comment 43

[Mark Capella [:capella]]

Comment on attachment 722348 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]: Should have been moved along with prior move to beta ... testing / verifications  look good.

Bug caused by (feature/regressing bug #): Undetermined regression.
User impact if declined: Degraded user experience.
Testing completed (on m-c, etc.): Tested locally by developer, posted build tested independently, previous regression caused by this patch corrected / verified. (See previous comments).

Risk to taking this patch (and alternatives if risky): Should be extremely minimal, critical backout amounts to one-line change.
String or UUID changes made by this patch: None.

Comment 44

[Adrian Tamas (:AdrianT)]

Verified fix on Firefox Mobile 20 beta 5 build 1 on the Samsung Galaxy Tab 2 7.0 (Android 4.1.1)

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb64688519fb