Helper app opens different video on m.youtube.com

RESOLVED FIXED in Firefox 30

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: cos_flaviu, Assigned: esawin)

Tracking

Trunk
Firefox 30
All
Android
Points:
---

Firefox Tracking Flags

(firefox26 affected, firefox27 affected, firefox28 affected, firefox29 affected, fennec+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Environment: 
Device: Asus Transformer Tab (Android 4.0.3);
Build: Nightly 29.0a1 (2014-01-12).

Steps to reproduce:
1. Go to m.youtube.com;
2. Tap on any video from the list;
3. Tap on the video to launch the youtube app;
4. Tap on the android back button to return to firefox;
5. Tap on the android back button to go back to the main page of the m.youtube.com;
6. Tap on a different video from the list;
7. Tap on the helper app icon in the url bar.

Expected result:
The video from the step 6 is started in the youtube app.

Actual result:
The youtube app is launched with the video from the step 3.
(Reporter)

Updated

5 years ago
status-firefox26: --- → affected
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected

Updated

5 years ago
tracking-fennec: --- → ?

Updated

5 years ago
tracking-fennec: ? → +
Assignee: nobody → esawin
Created attachment 8380965 [details] [diff] [review]
Update page action on location change

Currently, the page actions for helper apps are updated on |pageshow| events, which are not triggered for dynamic in-document content loads, even if the URL changes.

Moving the page action updates into the |onLocationChange| handler solves this.

Issues:
It's possibly unrelated, but going back in history to the m.youtube.com front page and clicking the helper app link opens the YouTube app and additionally launches Chrome for m.youtube.com in my case.

STR:
1. Apply patch.
2. Goto m.youtube.com (loads m.youtube.com and redirects to m.youtube.com/home).
3. Go back using browser or android button (loads m.youtube.com).
4. Click on helper app icon in the URL bar.

Expected result:
YouTube app launches.

Actual result:
YouTube app launches and Google Chrome launches on m.youtube.com.
Attachment #8380965 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8380965 [details] [diff] [review]
Update page action on location change

Redirecting review to wesj as he's more familiar with this code.
Attachment #8380965 - Flags: review?(lucasr.at.mozilla) → review?(wjohnston)
Comment on attachment 8380965 [details] [diff] [review]
Update page action on location change

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

Location change events will probably happen during page load. We don't want to slow it down with this query, so we'll need to find some other way to do this thats a bit more careful.
Attachment #8380965 - Flags: review?(wjohnston)
Created attachment 8381677 [details] [diff] [review]
Update page action URL on location change

With this patch, we just update the page action URL on location change without querying for helper apps, which happens, as before, on |pageshow|.

That way, the page action  will always launch the selected apps for the current URL.

Issues:
There is an unrelated issue with the intent filtering for YouTube, m.youtube.com qualifies for the YouTube app, while m.youtube.com/home does not. Since YouTube redirects to /home, |pageshow| is never fired for its root domain. The effect is, that the helper app icon only appears after clicking on a video.
Attachment #8380965 - Attachment is obsolete: true
Attachment #8381677 - Flags: review?(wjohnston)
Comment on attachment 8381677 [details] [diff] [review]
Update page action URL on location change

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

I like this :) Can we clean up the clickCallback a little while we're here, and this will be good. Thanks :)

::: mobile/android/chrome/content/browser.js
@@ +7909,5 @@
> +  },
> +
> +  getPageActionUri: function getPageActionUri() {
> +    return this._pageActionUri;
> +  },

If all these do is get/set a value, I'd just access it directly.

@@ +7923,5 @@
>        title: Strings.browser.GetStringFromName("openInApp.pageAction"),
>        icon: "drawable://icon_openinapp",
>        clickCallback: (function() {
>          let callback = function(app) {
> +          app.launch(ExternalApps.getPageActionUri());

We can be trickier here and use fat-arrow functions to help. Also I hate this inner callback function (that I added). i.e. change this to:

clickCallback: () => {
  if (apps.length > 1) {
      // pick an app...
      apps[result.icongrid0].launch(this.pageActionUri);
  } else {
      apps[0].launch(this.pageActionUri);
  }
}
Attachment #8381677 - Flags: review?(wjohnston) → feedback+
Created attachment 8382528 [details] [diff] [review]
Update page action URL on location change

Updated patch according to review feedback.

Patch for check-in.
Attachment #8381677 - Attachment is obsolete: true
Attachment #8382528 - Flags: review+
Attachment #8382528 - Flags: review+ → review?(wjohnston)
Attachment #8382528 - Flags: review?(wjohnston) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/e3ce8fb65c5f
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e3ce8fb65c5f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Duplicate of this bug: 983640
You need to log in before you can comment on or make changes to this bug.