Last Comment Bug 726904 - nsVideoFrame::GetVideoIntrinsicSize setting wrong size
: nsVideoFrame::GetVideoIntrinsicSize setting wrong size
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Chris DeCairos (:cade)
:
Mentors:
Depends on: 754860
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-13 20:14 PST by Chris DeCairos (:cade)
Modified: 2012-05-14 08:23 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Makes GetVideoIntrinsicSize prefer video size over poster size (2.84 KB, patch)
2012-02-14 16:29 PST, Chris DeCairos (:cade)
roc: feedback-
Details | Diff | Splinter Review
Makes GetVideoIntrinsicSize prefer video size over poster size (2.33 KB, patch)
2012-02-19 19:29 PST, Chris DeCairos (:cade)
no flags Details | Diff | Splinter Review
Makes GetVideoIntrinsicSize prefer video size over poster size (2.71 KB, patch)
2012-02-20 14:53 PST, Chris DeCairos (:cade)
no flags Details | Diff | Splinter Review
Changes as per review (2.69 KB, patch)
2012-02-20 15:17 PST, Chris DeCairos (:cade)
no flags Details | Diff | Splinter Review
Fixes as per review, first run on tests for the bug (57.98 KB, patch)
2012-02-20 19:07 PST, Chris DeCairos (:cade)
no flags Details | Diff | Splinter Review
Reduces poster image data URI size, adds check for video/ogg (5.48 KB, patch)
2012-02-23 17:25 PST, Chris DeCairos (:cade)
no flags Details | Diff | Splinter Review
Changes to use GetPlayableVideo in tests (6.29 KB, patch)
2012-03-05 20:42 PST, Chris DeCairos (:cade)
no flags Details | Diff | Splinter Review
Modified reftests (13.06 KB, patch)
2012-03-06 21:28 PST, Chris DeCairos (:cade)
cpearce: review+
Details | Diff | Splinter Review
Changes to patch, as per review (13.48 KB, patch)
2012-03-18 14:58 PDT, Chris DeCairos (:cade)
cade: review+
Details | Diff | Splinter Review
Fixes to reftests, minor bugfix to nsVideoFrame.cpp (20.44 KB, patch)
2012-04-15 17:53 PDT, Chris DeCairos (:cade)
cpearce: review-
Details | Diff | Splinter Review
Changes to patch, as per review (23.52 KB, patch)
2012-04-18 00:05 PDT, Chris DeCairos (:cade)
cpearce: review+
Details | Diff | Splinter Review
carrying forward r=cpearce (23.52 KB, patch)
2012-04-18 21:25 PDT, Chris DeCairos (:cade)
cade: review+
Details | Diff | Splinter Review
fixed failing tests on OSX carrying forward r=cpearce (23.51 KB, patch)
2012-05-10 23:01 PDT, Chris DeCairos (:cade)
cade: review+
Details | Diff | Splinter Review

Description Chris DeCairos (:cade) 2012-02-13 20:14:10 PST
according to 
http://www.whatwg.org/specs/web-apps/current-work/#dom-video-videowidth

"The intrinsic width of a video element's playback area is the intrinsic width of the video resource, if that is available; otherwise it is the intrinsic width of the poster frame, if that is available; otherwise it is 300 CSS pixels.
The intrinsic height of a video element's playback area is the intrinsic height of the video resource, if that is available; otherwise it is the intrinsic height of the poster frame, if that is available; otherwise it is 150 CSS pixels."

nsVideoFrame::GetVideoIntrinsicSize will set the video size to the Poster Frame  even if the video size and width might be available.

Shouldn't be too hard to fix. I hope there aren't any reftests that this will break.
Comment 1 Chris DeCairos (:cade) 2012-02-13 20:15:59 PST
To correct myself:

nsVideoFrame::GetVideoIntrinsicSize will set the video size to the Poster Frame  even if the video's intrinsic height and width might be available.
Comment 2 Chris DeCairos (:cade) 2012-02-13 22:21:31 PST
This might be correct but I'd like to record it here:

When GetVideoIntrinsicSize calls GetVideoSize on a video element that has preload="none" specified, it's going to return it's default values: nsSize(300,150). If the element is preload="auto" it will return the correct dimensions.

This seems like expected behavior and is the reason that there are fallback procedures in the spec. We can't expect to know the size of the video if we don't have any metadata. This would make it a logical next step to use the defaults if there is no Poster.
Comment 3 Chris DeCairos (:cade) 2012-02-14 16:29:12 PST
Created attachment 597238 [details] [diff] [review]
Makes GetVideoIntrinsicSize prefer video size over poster size

All I've done here is move around the code so that it will return the video size, if it's available, over the poster image size.

This will most likely break reftests
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-14 18:59:37 PST
Comment on attachment 597238 [details] [diff] [review]
Makes GetVideoIntrinsicSize prefer video size over poster size

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

::: layout/generic/nsVideoFrame.cpp
@@ +527,2 @@
>    if (!HasVideoElement()) {
> +    if (ShouldDisplayPoster()) {

Why are you looking at the poster image only if this is *not* a video element? That seems wrong.

You need to check whether there is a video element and it has found an intrinsic size for the resource. If so, use it, otherwise check for a poster and use its size. The code should look like the spec.

@@ +545,3 @@
>  
> +    nsHTMLVideoElement* element = static_cast<nsHTMLVideoElement*>(GetContent());
> +    size = element->GetVideoSize(size);

this size is never used since we return something else below
Comment 5 Chris DeCairos (:cade) 2012-02-19 19:29:13 PST
Created attachment 598746 [details] [diff] [review]
Makes GetVideoIntrinsicSize prefer video size over poster size

I've taken into account your feedback. How does this look?
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-19 20:15:58 PST
Comment on attachment 598746 [details] [diff] [review]
Makes GetVideoIntrinsicSize prefer video size over poster size

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

This is very close...

::: layout/generic/nsVideoFrame.cpp
@@ +538,5 @@
>      return nsSize(nsPresContext::CSSPixelsToAppUnits(size.width), prefHeight);
>    }
>  
>    nsHTMLVideoElement* element = static_cast<nsHTMLVideoElement*>(GetContent());
> +  size = element->GetVideoSize(nsIntSize(0, 0));

I think you should probably pass -1,-1 here or something like that. A zero-sized video doesn't make much sense but probably is legal.
Comment 7 Chris DeCairos (:cade) 2012-02-20 14:53:56 PST
Created attachment 598978 [details] [diff] [review]
Makes GetVideoIntrinsicSize prefer video size over poster size

I'm pretty confident this patch should cover all the cases we have, and follow the spec at the same time.

It's likely going to break some reftests.. I should probably apply for level 1 commit access for the try server.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-20 15:00:33 PST
Comment on attachment 598978 [details] [diff] [review]
Makes GetVideoIntrinsicSize prefer video size over poster size

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

::: layout/generic/nsVideoFrame.cpp
@@ +539,5 @@
>  
>    nsHTMLVideoElement* element = static_cast<nsHTMLVideoElement*>(GetContent());
> +  size = element->GetVideoSize(nsIntSize(-1, -1));
> +
> +  if (size.height == -1 && size.width == -1 && ShouldDisplayPoster()) {

You can just test size.height <= 0. No point in checking the width as well.

@@ +541,5 @@
> +  size = element->GetVideoSize(nsIntSize(-1, -1));
> +
> +  if (size.height == -1 && size.width == -1 && ShouldDisplayPoster()) {
> +    // Use the poster image frame's size.
> +    nsIFrame *child = mFrames.FirstChild();

This isn't quite right, we don't want to depend on the poster image frame being first in the list. Use mPosterImage->GetPrimaryFrame() instead.

@@ +543,5 @@
> +  if (size.height == -1 && size.width == -1 && ShouldDisplayPoster()) {
> +    // Use the poster image frame's size.
> +    nsIFrame *child = mFrames.FirstChild();
> +    if (child && child->GetType() == nsGkAtoms::imageFrame) {
> +      nsImageFrame* imageFrame = static_cast<nsImageFrame*>(child);

Use do_QueryFrame instead.
Comment 9 Chris DeCairos (:cade) 2012-02-20 15:17:56 PST
Created attachment 598987 [details] [diff] [review]
Changes as per review

I've made the changes you mentioned. I was curious as to why the poster was being assumed as the first child frame.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-20 15:21:26 PST
Comment on attachment 598987 [details] [diff] [review]
Changes as per review

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

::: layout/generic/nsVideoFrame.cpp
@@ +543,5 @@
> +  if (size.height == -1 && ShouldDisplayPoster()) {
> +    // Use the poster image frame's size.
> +    nsIFrame *child = mPosterImage->GetPrimaryFrame();
> +    if (child && child->GetType() == nsGkAtoms::imageFrame) {
> +      nsImageFrame* imageFrame = do_QueryFrame(child);

With this, you don't need an explicit check of GetType(). If it's not an image frame, do_QueryFrame will return null.
Comment 11 Chris DeCairos (:cade) 2012-02-20 15:25:35 PST
Ok, cool. I'll make that change quickly and check it's all working still. I should probably write some mochi-tests for this shouldn't I?
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-20 15:44:59 PST
A mochitest in content/media/tests is probably the way to go.

It's a bit tricky to test the case where there's no video frame displaying yet but we do have a poster image and we have determined the size of the video resource (we should be testing that the instrinsic size is the size of the resource, not the poster image). You could make the poster image a data URI so it loads really fast, hopefully faster than the video first frame, and of course a different size from the video, then just run a fast setTimeout loop until the video reaches the loadeddata state, constantly checking that the size of the <video> matches the size of the video resource if we're in a state >= HAVE_METADATA.
Comment 13 Chris DeCairos (:cade) 2012-02-20 19:07:34 PST
Created attachment 599030 [details] [diff] [review]
Fixes as per review, first run on tests for the bug

I'm not sure if these tests are perfect, but what they do is check that one video which preloads the video data will be 320x240 and that the other which has no preload, takes on the size of the poster ( 400x400 )
Comment 14 Chris DeCairos (:cade) 2012-02-20 19:10:10 PST
darn, copy-pasta leftovers in the test case... D:
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-20 20:03:50 PST
Can you make the poster image a solid black PNG, so that it compresses better? Those huge data URLs are a bit scary :-)
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-20 20:07:43 PST
Comment on attachment 599030 [details] [diff] [review]
Fixes as per review, first run on tests for the bug

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

One other thing is that we try to make our media tests backend-independent. Here it's enough to add a test for canPlayType("video/ogg") and if that returns false, do todo(false, "Ogg not supported") and finish immediately.
Comment 17 Chris Pearce (:cpearce) 2012-02-20 20:10:57 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #16)
> Comment on attachment 599030 [details] [diff] [review]
> Fixes as per review, first run on tests for the bug
> 
> Review of attachment 599030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One other thing is that we try to make our media tests backend-independent.
> Here it's enough to add a test for canPlayType("video/ogg") and if that
> returns false, do todo(false, "Ogg not supported") and finish immediately.

Or call getPlayableVideo(), like we do in this test:
http://mxr.mozilla.org/mozilla-central/source/content/media/test/test_mixed_principals.html?force=1#27
Comment 18 Chris DeCairos (:cade) 2012-02-23 17:25:32 PST
Created attachment 600258 [details] [diff] [review]
Reduces poster image data URI size, adds check for video/ogg

I took a look at getPlayableVideo(), but it didn't seem like I could be sure what dimensions the video I received would be, so I opted instead for the canPlayType method. The poster image is now a grayscale black png, so it's _much_ smaller.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-24 03:15:07 PST
Comment on attachment 600258 [details] [diff] [review]
Reduces poster image data URI size, adds check for video/ogg

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

::: content/media/test/test_bug726904.html
@@ +29,5 @@
> +var v1 = document.getElementById( "v1" ),
> +    v2 = document.getElementById( "v2" );
> +
> +if( !v1.canPlayType( "video/ogg" ) ) {
> +  todo(false, "Ogg not supported");

This isn't quite right because if Ogg is disabled you just go ahead and run the test anyway. You need to skip the rest of the test.
Comment 20 Chris Pearce (:cpearce) 2012-02-26 13:37:14 PST
(In reply to Chris DeCairos (:cadecairos) from comment #18)
> Created attachment 600258 [details] [diff] [review]
> Reduces poster image data URI size, adds check for video/ogg
> 
> I took a look at getPlayableVideo(), but it didn't seem like I could be sure
> what dimensions the video I received would be, so I opted instead for the
> canPlayType method. The poster image is now a grayscale black png, so it's
> _much_ smaller.

If you add the dimensions of the video to the test entry in manifest.js (like we do here http://mxr.mozilla.org/mozilla-central/source/content/media/test/manifest.js#10) then you know the dimensions.

Then in your test, do the following:

var resource = getPlayableVideo(gSmallTests);
// And then the values are:
resource.name // the file name for the test
resource.width // the width
resource.height // the height

So you'd need to add the width and height for seek.webm to gSmallTests to do this (it's also 320x240 I think).
Comment 21 Chris DeCairos (:cade) 2012-03-05 20:42:14 PST
Created attachment 603154 [details] [diff] [review]
Changes to use GetPlayableVideo in tests

I've changed this around to use GetPlayableVideo, which will get either webm or ogv, or fail with a todo.
Comment 22 Chris Pearce (:cpearce) 2012-03-06 14:07:25 PST
Comment on attachment 603154 [details] [diff] [review]
Changes to use GetPlayableVideo in tests

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

::: content/media/test/test_bug726904.html
@@ +23,5 @@
> +    poster = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAZAAAAGQCAAAAACl1GkQAAAAAXNSR0IArs4c6QAAAAlwSFlzAAALEwAACxMBAJqcGAAAALJJREFUeNrtwQENAAAAwqD3T20ON6AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHg0cq4AATRk8BYAAAAASUVORK5CYII",
> +    resource = getPlayableVideo(gSeekTests);
> +
> +function v1_metaDataLoaded(){
> +  is( v1.videoWidth, 320, "Intrinsic width should be 320" );

Should that be:
is( v1.videoWidth, resource.width, "Intrinsic width should be match video dimensions");
?

And similar with videoHeight?

Not every resource returned by GetPlayableVideo() is guaranteed to be 320x240; that's why we have width/height stored in the manifest.
Comment 23 Chris Pearce (:cpearce) 2012-03-06 16:15:24 PST
I pushed the patch to try, and unfortunately the reftests fail:
https://tbpl.mozilla.org/?tree=Try&rev=5d9d90851e42
Comment 24 Chris DeCairos (:cade) 2012-03-06 21:28:30 PST
Created attachment 603594 [details] [diff] [review]
Modified reftests

I've modified the ref tests so that they take into account the new behaviour that <video> elements have. also, modified the mochitests to use gSmallTests, which it should've been doing originally. Now I can get height/width.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-07 18:00:07 PST
Comment on attachment 603594 [details] [diff] [review]
Modified reftests

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

Over to cpearce to review the test changes from now on :-)
Comment 26 Chris Pearce (:cpearce) 2012-03-18 14:07:27 PDT
Comment on attachment 603594 [details] [diff] [review]
Modified reftests

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

I made a few comments, r=cpearce with them addressed.

We'd better land this and bug 517363 together, else we'll be rendering the poster image with incorrect aspect ratio while scaling, which would look weird. And for bug 517363 you'll need to update the tests again to reflect that the poster image should be scaled WRT the aspect ratio to fit inside the video element's intrinsic size.

::: content/media/test/test_bug726904.html
@@ +23,5 @@
> +    poster = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAZAAAAGQCAAAAACl1GkQAAAAAXNSR0IArs4c6QAAAAlwSFlzAAALEwAACxMBAJqcGAAAALJJREFUeNrtwQENAAAAwqD3T20ON6AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHg0cq4AATRk8BYAAAAASUVORK5CYII",
> +    resource = getPlayableVideo(gSmallTests);
> +
> +function v1_metaDataLoaded(){
> +  is( v1.videoWidth, resource.width, "Intrinsic width should be 320" );

Nit: "Intrinsic width should match video width". resource.width may not be 320. ;) Ditto for height.

::: layout/reftests/ogg-video/poster-1.html
@@ +1,4 @@
>  <!DOCTYPE HTML>
>  <html>
>  <body style="background:white;">
>  <!-- Test if poster frame displays correctly when poster is different size. -->

Change comment to: "Ensure video element displays at poster size when video's intrinsic size isn't available."

::: layout/reftests/ogg-video/poster-15.html
@@ +5,5 @@
>       them etc. -->
>  <body style="background:white;">
>  <video src="black140x100.ogv"
>         poster="green70x30.png"
> +       preload="none"

This test is supposed to test that backgrounds are drawn correctly when the poster is too small, but with preload="none" that won't be tested. For this patch, leave this as preload="none", but make sure you remove this preload="none" when you fix bug 517363, and adjust the test to ensure that backgrounds are properly rendered.

::: layout/reftests/ogg-video/poster-7.html
@@ +7,2 @@
>         id="v"
> +       onload="document.getElementById('v').poster = 'red140x100.png'; setTimeout(function(){document.documentElement.className = '';}, 0);"

When you land bug 517363, revert this to use red160x120.png, so we can test letter boxing etc. This means there's not much point in removing poster-ref-red160x120.html, since we'll need it again.

::: layout/reftests/ogg-video/poster-ref-blue140x100.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<body style="background:white;">
> +<img src="blue140x100.png" alt="poster">

You need to include blue140x100.png in this patch, or make it a datauri.

::: layout/reftests/ogg-video/poster-ref-red140x100.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<body style="background:white;">
> +<img src="red140x100.png" alt="poster">

You need to include red140x100.png in this patch, or make it a datauri.
Comment 27 :Ms2ger 2012-03-18 14:13:13 PDT
Comment on attachment 603594 [details] [diff] [review]
Modified reftests

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

Some style nits:

::: content/media/test/test_bug726904.html
@@ +26,5 @@
> +function v1_metaDataLoaded(){
> +  is( v1.videoWidth, resource.width, "Intrinsic width should be 320" );
> +  is( v1.videoHeight, resource.height, "Intrinsic height should be 240" );
> +  is( v2.clientWidth, 400, "clientWidth should be 400" );
> +  is( v2.clientHeight, 400, "clientHeight should be 400" );  

'is(foo', not 'is( foo' (note space), 4 times.

@@ +30,5 @@
> +  is( v2.clientHeight, 400, "clientHeight should be 400" );  
> +  SimpleTest.finish();
> +}
> +
> +if (resource != null ) {

'if (resource)'

@@ +46,5 @@
> +  document.body.appendChild(v2);
> +  
> +} else {
> +
> +  todo(false, "No types supported");

Get rid of the empty lines above
Comment 28 Chris DeCairos (:cade) 2012-03-18 14:58:29 PDT
Created attachment 607011 [details] [diff] [review]
Changes to patch, as per review

carrying forward. r=cpearce
Comment 29 Chris DeCairos (:cade) 2012-04-15 17:53:11 PDT
Created attachment 615212 [details] [diff] [review]
Fixes to reftests, minor bugfix to nsVideoFrame.cpp

I fixed up all the reftests, and fixed a bug in my change to nsVideoFrame.cpp. here's the TBPL for this: https://tbpl.mozilla.org/?tree=Try&rev=699aec48de21

Now that I have this working I'll get Bug 517363 working on top of this patch, so that we can land them at the same time :)
Comment 30 Chris Pearce (:cpearce) 2012-04-17 15:24:39 PDT
Comment on attachment 615212 [details] [diff] [review]
Fixes to reftests, minor bugfix to nsVideoFrame.cpp

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

::: layout/generic/nsVideoFrame.cpp
@@ +539,4 @@
>    }
>  
>    nsHTMLVideoElement* element = static_cast<nsHTMLVideoElement*>(GetContent());
> +  size = element->GetVideoSize(nsIntSize(-1, -1));

This code smells bad. Because you removed |size|'s initialization to (300,150), you now have to include the default width of 300 twice below. If someone changes one, they need to remember to the other. This makes the code code harder to maintain.

Also, since GetVideoSize() returns the default value on failure and overwriting size's initialized value, you need to add more conditional logic to handle its failure, complicating the code's logic.

So let's refactor to clean things up. Since GetVideoSize() isn't used anywhere else, lets change GetVideoSize() to return NS_ERROR_FAILURE when there's no video size, and pass in a reference to size which is filled upon success.

Then your "if (size.height==-1..." can become:
  if (NS_FAILED(element->GetVideoSize(size)) && ShouldDisplayPoster()) ...
... and you don't need to remove the default initialization, or repeat the default values, or add extra conditional logic either.
Comment 31 Chris DeCairos (:cade) 2012-04-18 00:05:05 PDT
Created attachment 616035 [details] [diff] [review]
Changes to patch, as per review

I've made the changes to GetVideoSize, and modified the two places it's used. Looks a lot better now too!

I've pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=29a15c65cf0a
Comment 32 Chris Pearce (:cpearce) 2012-04-18 16:00:07 PDT
Comment on attachment 616035 [details] [diff] [review]
Changes to patch, as per review

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

Nice. Please remove the blank line I mention below, and re-upload the patch. No need to request review again, just mark it as r+ when you upload, and say something like "carrying forward r=cpearce". Thanks!

::: layout/generic/nsVideoFrame.cpp
@@ +540,5 @@
>      return nsSize(nsPresContext::CSSPixelsToAppUnits(size.width), prefHeight);
>    }
>  
>    nsHTMLVideoElement* element = static_cast<nsHTMLVideoElement*>(GetContent());
> +

Can you remove this blank line?
Comment 33 Chris DeCairos (:cade) 2012-04-18 21:25:53 PDT
Created attachment 616450 [details] [diff] [review]
carrying forward r=cpearce

Removed extra line.

cpearce: I guess we now need to get Bug 517363 good to go? I'm waiting on a review on that one from roc. The patch is pretty ugly though, it'll need some work to get it just right.
Comment 34 Chris Pearce (:cpearce) 2012-05-10 17:57:06 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=cfb9b2418c2b
Comment 35 Chris DeCairos (:cade) 2012-05-10 23:01:26 PDT
Created attachment 623049 [details] [diff] [review]
fixed failing tests on OSX carrying forward r=cpearce

pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=8e1c95032e9b
Comment 37 Matt Brubeck (:mbrubeck) 2012-05-11 11:44:13 PDT
https://hg.mozilla.org/mozilla-central/rev/559fdd1b5e07

Note You need to log in before you can comment on or make changes to this bug.