981483 - Helper app does not update or clear previous page actions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Eugen Sawin [:esawin]]

STR 1:
1. Go to http://m.youtube.com.
2. Push Android/Fennec back button to go back to about:home.

Expected result:
No helper app page action should be set for about:home.

Actual result:
Previously set page action (YouTube) is still set for about:home.


STR 2:
1. Go to http://techslides.com/demos/sample-videos/small.mp4.
2. Replace URL with http://m.youtube.com.

Expected result:
Helper app page action should be updated to the YouTube app.

Actual result:
Previously set page action (selection of video players) is still set.

In both cases, clicking on the helper app icon would fail to load the app, since the apps are incompatible with the current URL.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: Clear helper app page action on page load

The early bail out from bug 969835 prevents |ExternApps| from clearing the page actions for chrome: and about: URLs, this patch clears them in this case.

Also page actions always need to be cleared before updating, otherwise only the page action URL is updated, not the app. Use |updatePageActionUri| to only update the URL.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Eugen Sawin [:esawin] from comment #1)
> The early bail out from bug 969835 prevents |ExternApps| from clearing the
> page actions for chrome: and about: URLs, this patch clears them in this
> case.

--> Marking as regression from that bug. Fortunately that bug is new on Firefox 30, so we don't need to backport anything to fix this part on Aurora.

> Also page actions always need to be cleared before updating,
> otherwise only the page action URL is updated, not the app.

This (for STR 2) seems like a somewhat-separate issue. Do we know if this problem exists & needs fixing on Aurora?

(Aurora doesn't give me any URLbar helper-app icon at m.youtube.com, so I can't complete the STR; if that's true of other folks as well, then I suppose we're OK?)

Comment 4

[Daniel Holbert [:dholbert]]

(by "fortunately that bug is new on Firefox 30", I of course meant "fortunately that patch only landed on Firefox 30"; i.e. it didn't break this on other branches)

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8388327 [details] [diff] [review]
Clear helper app page action on page load

Redirecting to wesj who is more familiar with this code.

Comment 6

[Eugen Sawin [:esawin]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> (In reply to Eugen Sawin [:esawin] from comment #1)
> > Also page actions always need to be cleared before updating,
> > otherwise only the page action URL is updated, not the app.
> 
> This (for STR 2) seems like a somewhat-separate issue. Do we know if this
> problem exists & needs fixing on Aurora?
> 
> (Aurora doesn't give me any URLbar helper-app icon at m.youtube.com, so I
> can't complete the STR; if that's true of other folks as well, then I
> suppose we're OK?)

I think there are other patches of the helper app that could affect this, e.g., bug 959108, which are not on Aurora. Here is an alternative STR 3:
1. Navigate to http://goo.gl/El7MQc or any other *.mp3 source.
2. Navigate to http://goog.gl/Rp9GrO or any other video source.
3. Click on the helper app icon.

Expected result:
Helper app gives a selection of video players.

Actual result:
With patch from bug 959108, helper app launches Audio Preview and only plays the audio part of the video. Without the patch, helper app launches Audio Preview on the previous URL.

Comment 7

[Eugen Sawin [:esawin]]

Correction: in STR 3 step 2, the URL should be http://goo.gl/Rp9GrO.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 8388327 [details] [diff] [review]
Clear helper app page action on page load

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

Lets try this. I worry about flicker from removing -> adding page actions, but it should be minimal. I must have broke this during the EventDispatcher move. Thanks for the cleanup.

We needs some tests here, do you mind filing a bug for that? I have some WIP for adding some special manifest entries for robocop, we could try registering an activity there for mochitest.com urls, and then at least testing that its found correctly. Maybe we could even have it open and assert...

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Eugen Sawin [:esawin]]

(In reply to Wesley Johnston (:wesj) from comment #9)
> Comment on attachment 8388327 [details] [diff] [review]
> Clear helper app page action on page load
> 
> Review of attachment 8388327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Lets try this. I worry about flicker from removing -> adding page actions,
> but it should be minimal. I must have broke this during the EventDispatcher
> move. Thanks for the cleanup.
Can we avoid the flicker? Would we need to decouple the icon drawing from the page actions and just update the page actions?

> We needs some tests here, do you mind filing a bug for that? I have some WIP
> for adding some special manifest entries for robocop, we could try
> registering an activity there for mochitest.com urls, and then at least
> testing that its found correctly. Maybe we could even have it open and
> assert...
I've filed a general bug for that: bug 983188.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

let's get a regression range to see if we need uplift. If the regression is not 30, please renom.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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