1245516 - launch from homescreen bookmark always opens new tab and sometimes does nothing

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

On my android M nexus 5x I try to use web pages bookmarked to the homescreen as much as possible.  One I use heavily is twitter.

In the past twitter would open in the same tab unless I had navigated the previous twitter tab to a different URL.

After upgrading to 44 I now seem to get a new tab every time I click the homescreen bookmark icon regardless of previous tab URLs.

Also, I have noticed that if fennec is left open long enough, sometimes click my twitter bookmark does nothing.  It opens fennec to its last screen, but does not open a new tab.  I have to kill fennec and restart it to get out of that state.

Comment 1

[:Margaret Leibovic]

This changeset looks suspicious:
http://hg.mozilla.org/mozilla-central/rev/5a6b63cd300e

Comment 2

[Andrzej Hunt :ahunt]

Uggh, I shadowed originHost there - this should hopefully be a very quick fix.

Comment 3

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

Sounds like this needs to be uplifted to Fx44?

Comment 4

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1245516 - Don't shadow originHost, so we can process appOrigin correctly r=margaret

Review commit: https://reviewboard.mozilla.org/r/33503/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33503/

Comment 5

[Andrzej Hunt :ahunt]

My patch fixes part 1, i.e. we can now open homescreen shortcuts in the existing tab if available.

We still have some issues, which I suspect are unrelated to my previous changes:

1. Ben reported that we don't always open a tab if needed (I have no idea what kind of state would cause this)
2. We unncessarily open a new tab if Fennec isn't running when the shortcut is selected (either kill the app manually, or use "Don't keep activities").

(Note to self: we should look into writing tests for these issues too.)

Comment 6

[:Margaret Leibovic]

[Tracking Requested - why for this release]: We shipped a regression in homescreen shortcuts in 44. I'm not sure that this merits a dot release, but it would be a good ridealong if we do happen to spin another release (e.g. for the Adjust problem).

Comment 7

[:Margaret Leibovic]

Comment on attachment 8715543 [details]
MozReview Request: Bug 1245516 - Don't shadow originHost, so we can process appOrigin correctly r=margaret

https://reviewboard.mozilla.org/r/33503/#review30295

Sigh... I'm embarassed I missed this in my original review.

Comment 8

[Andrzej Hunt :ahunt]

Comment on attachment 8715543 [details]
MozReview Request: Bug 1245516 - Don't shadow originHost, so we can process appOrigin correctly r=margaret

Approval Request Comment
[Feature/regressing bug #]: Regression caused in Bug 1208525 
[User impact if declined]: Homescreen shortcuts always open new tab, instead of reusing the existing tab if available.
[Describe test coverage new/current, TreeHerder]: Manual testing.
[Risks and why]: Low risk - variable is no longer shadowed, meaning the correct value is used again.
[String/UUID change made/needed]: none.

Comment 9

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/21ff9b18942e1ef3572de8eed08efd2ee92202b2
Bug 1245516 - Don't shadow originHost, so we can process appOrigin correctly r=margaret

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I will most likely take this fix (if it is verified in time) before I gtb Fennec 44.0.1 tomorrow or Friday.

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/21ff9b18942e

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8715543 [details]
MozReview Request: Bug 1245516 - Don't shadow originHost, so we can process appOrigin correctly r=margaret

Fix a regression, taking it.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Should be in 45 beta 4.

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0e8b8e610d8f

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8715543 [details]
MozReview Request: Bug 1245516 - Don't shadow originHost, so we can process appOrigin correctly r=margaret

Let's take it to do a potential build 3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/05920ec06360

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Is it possible to write an automated test that would catch this regression in the future?

Comment 19

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Is it possible to write an automated test that would catch this regression
> in the future?

Yes, it should be possible.

Comment 20

[u421692]

Verified as fixed on Nexus 9, on Firefox 44.0.2 build 4

Comment 21

[u421692]

Verified as fixed on Nexus 9, on Firefox 44.0.2 build 4

Comment 22

[u421692]

Verified as fixed on Nexus 9, on Firefox 45 Beta 6

Comment 23

[Teodora Vermesan (:TeoVermesan)]

Verified as fixed using:
Device: One A2001 (Android 5.1.1) 
Build: Firefox for Android 46.0a2 (2016-02-16)

Comment 24

[Teodora Vermesan (:TeoVermesan)]

Verified as fixed using:
Device: One A2001 (Android 5.1.1) 
Build: Firefox for Android 47.0a1 (2016-02-18)

Comment 25

[Andrzej Hunt :ahunt]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Is it possible to write an automated test that would catch this regression
> in the future?

I've filed a followup bug to add the tests, and also fix this for all cases (since we still open a new tab if Firefox isn't running), see Bug 1245516.