Bug 73948 – [chromium] Scale audio, video tags in MediaDocument to fit in window.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dale Curtis]

[chromium] Scale audio, video tags in MediaDocument to fit in window.

[Dale Curtis]

Created attachment 118108 [details]
Patch

[Dale Curtis]

Currently audio, video elements in MediaDocument don't scale to fit undersized containers. A painful situation for users wanting to watch high resolution videos without scroll bars. The attached patch fixes this issue by attaching CSS max-height: 100%, max-width: 100% settings to the audio and video tags for Chromium controls. A new layout test is included to verify this behavior.

Chromium Issue: http://crbug.com/26848
Chromium CL: http://codereview.chromium.org/8728018/

[Dale Curtis]

Since the test is Chromium specific, scherkus suggested I might need to add some lines to the various "Skipped" files for each platform. Is this done automatically? If not, which files should I modify?

[Andrew Scherkus]

this looks good to me -- I'm not sure what to do about the Skipped tests for other platforms

perhaps a WebKit reviewer can chime in?

[Eric Seidel (no email)]

Comment on attachment 118108 [details]
Patch

Seems reasonable to me.

[Eric Seidel (no email)]

CCing the Apple FullScreen and Media folks just in case they're curious to see this patch go by.

[Eric Carlson]

Comment on attachment 118108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118108&action=review

> LayoutTests/ChangeLog:12
> +        * media/video-scales-in-media-document-expected.txt: Added.
> +        * media/video-scales-in-media-document.html: Added.

This new test will fail on all ports that don't include mediaControlsChromium.css. You need to skip the test on every other port.

[Dale Curtis]

Thanks for the review!

@Eric Carlson: Which Skipped files do I need to modify? All of them? Something else?

$ WebKit/LayoutTests/platform $ find -name Skipped
./mac/Skipped
./qt-5.0/Skipped
./qt/Skipped
./qt-4.8/Skipped
./gtk-wk2/Skipped
./win-xp/Skipped
./efl/Skipped
./qt-mac/Skipped
./qt-win/Skipped
./qt-linux/Skipped
./mac-leopard/Skipped
./win-wk2/Skipped
./mac-snowleopard/Skipped
./wk2/Skipped
./mac-lion/Skipped
./qt-arm/Skipped
./qt-wk2/Skipped
./wincairo/Skipped
./win/Skipped
./mac-wk2/Skipped
./gtk/Skipped
./qt-wk1/Skipped

[Eric Carlson]

(In reply to comment #8)
> @Eric Carlson: Which Skipped files do I need to modify? All of them? Something else?
> 
Only the the top-level file for ports that don't already skip all media test. I think this means:

./mac/Skipped
./efl/Skipped
./win/Skipped
./gtk/Skipped

[WebKit Review Bot]

Comment on attachment 118108 [details]
Patch

Clearing flags on attachment: 118108

Committed r103489: <http://trac.webkit.org/changeset/103489>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

The test added by this patch is failing on Mac:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r103641%20(35833)/media/video-scales-in-media-document-pretty-diff.html

Is this test specific to Chromium? If so, why wasn't it added to LayoutTests/platform/chromium ?

[Dale Curtis]

@Ryosuke, the test is Chromium specific, I've put up a patch with the appropriate Skipped files here, but no one has approved it yet:

https://bugs.webkit.org/show_bug.cgi?id=75079

I'm new at this, so apologies if it's preferred to craft the platform expected files into no-ops for the platforms which can't run the test.

[Ryosuke Niwa]

(In reply to comment #13)
> @Ryosuke, the test is Chromium specific, I've put up a patch with the appropriate Skipped files here, but no one has approved it yet:
> 
> https://bugs.webkit.org/show_bug.cgi?id=75079
> 
> I'm new at this, so apologies if it's preferred to craft the platform expected files into no-ops for the platforms which can't run the test.

Can we move this test to LayoutTests/platform/chromium/media? It doesn't make much sense to put ths in LayoutTests/media if other ports shouldn't be passing this test.

[Dale Curtis]

Done. The patch in https://bugs.webkit.org/show_bug.cgi?id=75079 now moves the test into platform/chromium/media.