959108 - Helper app opens different video on m.youtube.com

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Flaviu Cos]

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.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 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.

Comment 2

[Lucas Rocha (:lucasr)]

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.

Comment 3

[Wesley Johnston (:wesj)]

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.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 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.

Comment 5

[Wesley Johnston (:wesj)]

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);
  }
}

Comment 6

[Eugen Sawin [:esawin]]

Attachment: Update page action URL on location change

Updated patch according to review feedback.

Patch for check-in.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e3ce8fb65c5f

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e3ce8fb65c5f