Mozilla/5.0 (Windows NT 6.1; rv:12.0a2) Gecko/20120220 Firefox/12.0a2 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0a2) Gecko/20120220 Firefox/12.0a2 Mozilla/5.0 (X11; Linux i686; rv:12.0a2) Gecko/20120220 Firefox/12.0a2 STR: 1. Open a page that contains an HTML5 video (e.g. http://www.robwinters.co.uk/lab/html5/video/drag-and-resize.htm) 2. Resize the video to less than 64px width 3. Resize the video back to more than 64px width Actual result: The play button from the center of the video is not re-enabled Expected result: After step 3, the center play button should be displayed again

To fix this bug, in /toolkit/content/widgets/videocontrols.xml the adjustControlSize function will need to be updated to make visible the clickToPlay element if it is currently hidden and the dimensions of the video are large enough to display it. The changes should be made in an |else if (!this.clickToPlay.hidden)| block after this check: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#1247

Based on bug 666306 comment 68: The play button should not be displayed if video size is smaller than 85px height, since it won't match the video (control bar will overlap the button).

This patch takes cares of both of the issues mentioned. Please check if this is the appropriate way to fix the bug. Thanks. One issue I noticed, after the video is resized, it takes a while before the playback controls reappear. Perhaps a refresh issue? (I am testing on Ubuntu.)

Comment on attachment 604825 [details] [diff] [review] Bug 729111: Patch-1 Can you reupload with 8 lines of context for the patch? See here for more details: https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Resubmit the patch with 8 lines of context.

Comment on attachment 605280 [details] [diff] [review] Bug 729111: Patch-2 Review of attachment 605280 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the extra context. ::: toolkit/content/widgets/videocontrols.xml @@ +1251,5 @@ > _durationLabelWidth : 0, > _muteButtonWidth : 0, > _fullscreenButtonWidth : 0, > _controlBarHeight : 0, > + _overlayPlayButtonHeight : 85, Please leave this at 64. We can use the above _controlBarHeight to get our height of 85. @@ +1260,5 @@ > > let videoHeight = this.video.clientHeight; > let videoWidth = this.video.clientWidth; > > if (this._overlayPlayButtonHeight > videoHeight || this._overlayPlayButtonWidth > videoWidth) At this line we would just do: > (this._overlayPlayButtonHeight + this._controlBarHeight) > videoHeight || ...

(In reply to Jared Wein [:jaws] from comment #6) > Please leave this at 64. We can use the above _controlBarHeight to get our > height of 85. Fixed, please review again. Thanks.

Comment on attachment 605290 [details] [diff] [review] Bug 729111: Patch-3 Review of attachment 605290 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ +1250,5 @@ > _playButtonWidth : 0, > _durationLabelWidth : 0, > _muteButtonWidth : 0, > _fullscreenButtonWidth : 0, > + _controlBarHeight : 21, We shouldn't set the height here. It's already set at https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#447. Just assume that it has already been set. @@ +1260,5 @@ > > let videoHeight = this.video.clientHeight; > let videoWidth = this.video.clientWidth; > > + if ((this._overlayPlayButtonHeight + this._controlBarHeight) > videoHeight || this._overlayPlayButtonWidth > videoWidth) nitpick: Lines should be < 80 characters long. Can you reformat this conditional to have a line break after the ||

1) Line reformatted. 2) Regarding the feedback: > We shouldn't set the height here. It's already set at > https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#447. > Just assume that it has already been set. Not sure if I completely understood clearly. Hopefully I interpreted it correctly. ;)

Comment on attachment 605301 [details] [diff] [review] Bug 729111: Patch-4 Review of attachment 605301 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for sticking with this. ::: toolkit/content/widgets/videocontrols.xml @@ -1250,5 @@ > _playButtonWidth : 0, > _durationLabelWidth : 0, > _muteButtonWidth : 0, > _fullscreenButtonWidth : 0, > - _controlBarHeight : 0, Sorry, my wording before was pretty confusing. Basically, just leave this line as it was before. In other words, this line should be unchanged. @@ +1260,5 @@ > let videoHeight = this.video.clientHeight; > let videoWidth = this.video.clientWidth; > > + if ((this._overlayPlayButtonHeight + this._controlBarHeight) > videoHeight || > + this._overlayPlayButtonWidth > videoWidth) { nit: The style in this file only uses curly brackets on if-statements if there is more than one line in the if-block. Remove the curly brackets in this if-elseif.

(In reply to Jared Wein [:jaws] from comment #10) > > - _controlBarHeight : 0, > > Sorry, my wording before was pretty confusing. Basically, just leave this > line as it was before. In other words, this line should be unchanged. That was my other guess, but I couldn't figure out what would happen. By leaving the line as it was before, wouldn't it set the height to 0 and override whatever was set in line 447? > nit: The style in this file only uses curly brackets on if-statements if there > is more than one line in the if-block. Remove the curly brackets in this if- > elseif. Ouch. I was trying to find a coding standing and simply following the style established here: https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#1004 Thanks for your help! (Working on the patch now ...)

One more time.

(In reply to Charles Chan from comment #11) > (In reply to Jared Wein [:jaws] from comment #10) > > > - _controlBarHeight : 0, > > > > Sorry, my wording before was pretty confusing. Basically, just leave this > > line as it was before. In other words, this line should be unchanged. > > That was my other guess, but I couldn't figure out what would happen. > By leaving the line as it was before, wouldn't it set the height to 0 and > override whatever was set in line 447? It's created with 0, but line 447 will happen during initialization of the controls (and as such, will be after _controlBarHeight is initialized to 0). > > nit: The style in this file only uses curly brackets on if-statements if there > > is more than one line in the if-block. Remove the curly brackets in this if- > > elseif. > > Ouch. I was trying to find a coding standing and simply following the style > established here: > https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/ > videocontrols.xml#1004 Good catch :) This file is odd in that it uses four spaces for indentation, whereas most files in the codebase use two spaces. Below is a link to the general coding style in case you are interested, but 99% of the time we just ask for consistency with the surrounding code. https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

Comment on attachment 605303 [details] [diff] [review] Bug 729111: Patch-5 rs=me, assuming Jared's ok with it.

Comment on attachment 605303 [details] [diff] [review] Bug 729111: Patch-5 Review of attachment 605303 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Thanks for the patch Charles :)

(In reply to Charles Chan from comment #3) > One issue I noticed, after the video is resized, it takes a while before the > playback controls reappear. Perhaps a refresh issue? (I am testing on > Ubuntu.) We only update the controls on mouseout (bug 462117) because we don't have the onresize event for elements yet (bug 227495). Sorry, I didn't see this question earlier.

(In reply to Jared Wein [:jaws] from comment #16) > (In reply to Charles Chan from comment #3) > > One issue I noticed, after the video is resized, it takes a while before the > > playback controls reappear. Perhaps a refresh issue? (I am testing on > > Ubuntu.) > > We only update the controls on mouseout (bug 462117) because we don't have > the onresize event for elements yet (bug 227495). Sorry, I didn't see this > question earlier. I forgot to add that the resize event for media controls is tracked by bug 696593.

I removed an extra whitespace character from the patch and pushed it to fx-team: https://hg.mozilla.org/integration/fx-team/rev/c7e4db80d280

Hello Phil, I have a quick glance at bug 735574, but not really sure how the (simple) changes in this ticket could cause a memory leak. Could you help? Basically, should I worry about it and possibly revert the changes for this ticket? Thanks.

> Basically, should I worry about it and possibly revert the changes for this ticket? The patch seems innocuous to me, to be sure, we're going to back it out and see if the leak goes away. But this patch needs to be merged to mozilla-inbound, first.

Backed out on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/114c4148c6f3 Charles, it would be helpful if you'd watch bug 735574 and let us know in this bug whether you still see the test failure on mozilla-inbound after this backout.

Sure. (I will also learn more about the mechanics of those automated tests.)

It looks like the build was still failing after the changes were backed out: 114c4148c6f3 Justin Lebar – Back out bug 729111 (rev c7e4db80d280) due to suspected randomorange (bug 735574). https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=114c4148c6f3 But the next build seems to pass: d067c50e01dc Ehsan Akhgari – Backout changeset ea6be5f60c42 (bug 722946) for breaking Windows builds https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d067c50e01dc

Update: Based on the comment in bug 735574 (https://bugzilla.mozilla.org/show_bug.cgi?id=735574#c17), the issue is not related to this change.

Sounds good; I'll re-land.

Re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/672cda014328

Does the target milestone need to be updated due to the backout/relanding?

(In reply to Jared Wein [:jaws] from comment #29) > Does the target milestone need to be updated due to the backout/relanding? No (although I thought we were using status-firefox-n, not target milestones?) because it made it in while trunk was still FF13 and was never backed out of FF13. We backed out and re-landed on the FF14 branch only.

I'd like to back this out of Aurora. We still have time to fix the regressions on Nightly though. [Approval Request Comment] Regression caused by (bug #): bug 729111 User impact if declined: bug 737774 and bug 738401 have been reported so far Testing completed (on m-c, etc.): n/a Risk to taking this patch (and alternatives if risky): this will undo the fix for bug 729111, but regressions caused by this patch aren't worth the bug fix. String changes made by this patch: none

Comment on attachment 608435 [details] [diff] [review] Backout of patch from Fx13 Aurora [Triage Comment] Once this has an r+, a=akeybl for this backout. Agreed that the new regression is worse than the previous.

Backed out on mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/15fb08cbc70d

Due to lack of activity on this bug and the regressions it introduced, we should back out this patch on Nightly14 until a better patch comes along that doesn't carry with it these regressions.

Agreed.

Backed out on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e4ac6159f8

Backout merged: https://hg.mozilla.org/mozilla-central/rev/f1e4ac6159f8

I tried fixing this bug. This patch only seems to work if you move your mouse over the video. To properly fix it we would need to some how listen for when the video element changes sizes. As I mentioned in bug 696593 I'm not sure there is a good way to do that.

Uploaded wrong patch file.

Comment on attachment 705188 [details] [diff] [review] Patch 6.1 Review of attachment 705188 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine, but I still think we'll have to wait until we have better resize notifications.

Owen, the patch for bug 876426 added a "resizevideocontrols" event that we can use here. Can you pick this bug back up?

Hi, this is my first time contributing to the Mozilla codebase. This patch should avoid the control bar height issue mentioned in Comment 2. I haven't experienced the need to mouse-over the video as in Comment 38.

Comment on attachment 8398855 [details] [diff] [review] Bug 729111: Patch 7 Review of attachment 8398855 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ +1390,3 @@ > this.clickToPlay.hidden = true; > + else if (this.clickToPlay.hidden && this.video.currentTime === 0) > + this.clickToPlay.hidden = false; This works good, but it has a couple issues. 1. If the user starts playing the video, returns it back to time=0, and then resizes the video, the click-to-play button will reappear. 2. We allow the user to drag the scrubber of the video before playing, and the click to play button should stay visible. If the user resizes the video to small, then to large, the play button won't reappear. 3. Similar to (2), the video can have a non-zero starting time, and we still want to have the same behaviors. You can check this.video.played.length to see if the video has been played. With that, this else-if condition can be changed to: `else if (this.clickToPlay.hidden && !this.video.played.length)`

Okay, I've made the change. I didn't realize that attribute existed.

Comment on attachment 8399604 [details] [diff] [review] Patch 8 Review of attachment 8399604 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, it's a pretty cool attribute and I think it's relatively new. There is more information about the API here, http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#media-elements Congrats on your first patch. You can now reupload this patch with r=jaws at the end of the commit message and then set the "checkin-needed" keyword on the bug :)

Danny, now that this patch has landed in the mozilla-central repository, it should appear in tomorrow's (4/3) Nightly builds. Thanks for your help on this bug, do you have another bug that you are interested in helping out on?

The following changeset is now in Firefox Nightly: > c23e2ff6c65e Bug 729111 - Redisplay centered play video button after video is resized. r=jaws Nightly Build Information: ID: 20140402030201 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6 Version: 31.0a1 TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786 URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central Download Links: > Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2 > Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2 > Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2 > Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg > Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe > Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe Previous Nightly Build Information: ID: 20140401030203 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4 Version: 31.0a1 TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8 URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central