1305430 - Favicon is displayed on media control notification only after the "pause" button is tapped

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P2)

Description

[Sorina Florean [:sflorean]]

Environment: 
Device: Nexus 5 (Android 6.0.1);
Build: Nightly 52.0a1 (2016-09-24);

Steps to reproduce:
1. Start Firefox with a clean profile;
2. Go to www.guardian.com;
3. Go to the android notification bar;
4. Tap the pause button from the media control notification.

Expected result:
Favicon is displayed on media control when the video starts playing. 

Actual result:
Favicon is displayed on media control only after the "pause" button is tapped.

Notes:
Please see the video: https://youtu.be/XAWcpn4oYBw

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

This seems to happen for the first load of the page only, right? After the icon has been loaded and displayed once, it works for subsequent visits of the page, right?

Comment 2

[Sorina Florean [:sflorean]]

(In reply to Sebastian Kaspari (:sebastian) from comment #1)
> This seems to happen for the first load of the page only, right? After the
> icon has been loaded and displayed once, it works for subsequent visits of
> the page, right?

Yes, and with clean profile it's reproducible.
If I tap on play/pause button from media control notification the favicon is displayed for that media content every time I visit the page.

Comment 3

[Alastor Wu [:alwu]]

Attachment: Bug 1305430 - part1 : send the notification again after tab got its favicon.

Sometimes the tab doesn't get its favicon yet when we send the notification.
Therefore, we would listen the FAVICON event and then re-send the notification again if we used the default cover icon in previous notification.

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

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1305430 - part2 : replace action checking with isMediaPlaying().

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

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8797098 [details]
Bug 1305430 - part1 : send the notification again after tab got its favicon.

https://reviewboard.mozilla.org/r/82734/#review82198

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:61
(Diff revision 1)
>  
>      private static WeakReference<Tab> mTabReference = new WeakReference<>(null);
>  
>      private int minCoverSize;
>      private int coverSize;
> +    private boolean mUseDefaultCoverIcon;

I wonder if we actually need this or if we should always update the notification if we receive a new icon for the playing tab. What do you think?

If we keep it then maybe we should rename it: From the name I'd assume that if this is 'true' then we always want to only use the default cover.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8797099 [details]
Bug 1305430 - part2 : replace action checking with isMediaPlaying().

https://reviewboard.mozilla.org/r/82736/#review82200

Comment 7

[Alastor Wu [:alwu]]

(In reply to Sebastian Kaspari (:sebastian) from comment #5)
> I wonder if we actually need this or if we should always update the
> notification if we receive a new icon for the playing tab. What do you think?

Good idea, I'll remove that and update the notification everytime when the tab gets the new favicon.

Comment 8

[Alastor Wu [:alwu]]

Comment on attachment 8797098 [details]
Bug 1305430 - part1 : send the notification again after tab got its favicon.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82734/diff/1-2/

Comment 9

[Alastor Wu [:alwu]]

Comment on attachment 8797099 [details]
Bug 1305430 - part2 : replace action checking with isMediaPlaying().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82736/diff/1-2/

Comment 10

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db41ca6b1b8cae9af55eb33e3fa3cea4cc685c88&selectedJob=28484716

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf58d76c8f1d
part1 : send the notification again after tab got its favicon. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/85ce273e1c6b
part2 : replace action checking with isMediaPlaying(). r=sebastian

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf58d76c8f1d
https://hg.mozilla.org/mozilla-central/rev/85ce273e1c6b

Comment 13

[Sorina Florean [:sflorean]]

Verified as fixed in build 52.0a1 2016-10-09;
Device: LG G4 (Android 5.1) and Nexus 5 (Android 6.0.1).