Closed Bug 1214624 Opened 4 years ago Closed 4 years ago

tab-sound-icon disappears when buffering is stopped despite during playback

Categories

(Firefox :: Tabbed Browser, defect)

34 Branch
Unspecified
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 45
Tracking Status
firefox42 --- unaffected
firefox43 + verified
firefox44 + fixed
firefox45 + fixed

People

(Reporter: alice0775, Assigned: baku)

References

Details

(Keywords: regression)

Attachments

(1 file)

Reproduced on Windows7, Windows10 and Ubuntu14.04, Nightly44.0a1, Aurora43.a2.
It seems to be unaffected to Beta42.b6.

tab-sound-icon disappears when buffering is stopped(completed) 

Steps to reproduce:
1. Open https://videos.cdn.mozilla.net/uploads/mozillaorg/
2. Click "A different kind of browser.webm" link 
   --- observe, tab-sound-icon indicator

Actual results:
 tab-sound-icon suddenly disappear when buffering is stopped despite during playback
Tracking since this is a bug in a new feature. 
Ehsan, can you help find someone to take a look at this?
Flags: needinfo?(ehsan)
Let me bisect to see what broke this.
Flags: needinfo?(ehsan)
Bug 1193085 has regressed this.
Blocks: 1193085
Keywords: regression
Looks like this is still affecting 43 (I just tried it).  
Andrew is there someone on your team who can have a look?
Flags: needinfo?(overholt)
Flags: needinfo?(overholt) → needinfo?(amarchesini)
Attached patch audio.patchSplinter Review
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8687668 - Flags: review?(jaws)
Comment on attachment 8687668 [details] [diff] [review]
audio.patch

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

This looks fine to me but I'd like to get a second opinion from Marco.
Attachment #8687668 - Flags: review?(mak77)
Attachment #8687668 - Flags: review?(jaws)
Attachment #8687668 - Flags: review+
Comment on attachment 8687668 [details] [diff] [review]
audio.patch

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

the new high-level location to do the removal sounds better than the old low-level one.

::: browser/base/content/tabbrowser.xml
@@ +756,5 @@
>                  if (this.mBrowser.userTypedClear > 0 ||
>                      ((aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) &&
>                       aLocation.spec != "about:blank") ||
>                       aFlags && Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT)
>                    this.mBrowser.userTypedValue = null;

please while here brace this if body, the indentation in this area is not enough to figure the flow at first sight...
Attachment #8687668 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/72cf0adcfb27
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Baku, do we need to uplift this to Aurora44?
Flags: needinfo?(amarchesini)
Yes, this should be uplifted to 44. Ideally 43, but it's likely too late for that.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> Yes, this should be uplifted to 44. Ideally 43, but it's likely too late for
> that.

Thanks Jared. Would you be able to nominate the patch for uplift to Aurora in that case and fill up the risk assessment and test coverage info?
Flags: needinfo?(jaws)
Baku can handle that.
Flags: needinfo?(jaws) → needinfo?(amarchesini)
We still have beta 9. I'd love to have this fix in 43 even if it is last-minute.
Comment on attachment 8687668 [details] [diff] [review]
audio.patch

Approval Request Comment
[Feature/regressing bug #]: Audio icon per tab
[User impact if declined]: sometimes the icon is not shown when buffering stopped
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: We don't have a valid test for this patch but it's a very simple code move from OnStopRequest to OnLocationChanged.
[String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8687668 - Flags: approval-mozilla-aurora?
Alice0775 White, could you please verify this fix works as expected on a latest Nightly build? Thanks!
Flags: needinfo?(alice0775)
Comment on attachment 8687668 [details] [diff] [review]
audio.patch

This fix has been on Nightly for almost a week, let's uplift to Aurora44.
Attachment #8687668 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
baku can you request uplift to beta also? Will this apply for 43?
Flags: needinfo?(amarchesini)
(In reply to Ritu Kothari (:ritu) from comment #16)
> Alice0775 White, could you please verify this fix works as expected on a
> latest Nightly build? Thanks!


I cannot reproduce the problem on Nightly45.0a1 and Aurora44.0a2 anymore.

https://hg.mozilla.org/mozilla-central/rev/31fc97d173b3e3da5de35d54018855e6c905787e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 ID:20151203030226

https://hg.mozilla.org/releases/mozilla-aurora/rev/7aad7dee85151ba464c2a5d5aa9e522bad9287e3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0 ID:20151203004003
Flags: needinfo?(alice0775)
Comment on attachment 8687668 [details] [diff] [review]
audio.patch

Approval Request Comment
Read comment 15.
This patch should apply to m-b, but I didn't test it by myself. In case I can easily upload a new version for m-b.
Flags: needinfo?(amarchesini)
Attachment #8687668 - Flags: approval-mozilla-beta?
Comment on attachment 8687668 [details] [diff] [review]
audio.patch

Minor fix for tab audio indicator, tested on aurora, please uplift to beta.
Attachment #8687668 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I've tested on Windows 7 64bit, Mac OSX 10.9.5 and Ubuntu 13.04 32bit using Firefox 43 RC (buildID: 20151208100201) and I didn't see the issue. 

Although I have one mention here: I saw this issue on Windows 7 64bit using Firefox 43 RC on the following page: https://videos.cdn.mozilla.net/uploads/mozillaorg/ -> A different kind of browser.mp4 : sometimes, tab-sound-icon disappears when buffering is completed.
You need to log in before you can comment on or make changes to this bug.