Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(128)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 38313002: Remove <video> width and height attributes from intrisic size logic (Closed)

Created:
7 years, 2 months ago by philipj_slow
Modified:
6 years, 11 months ago
CC:
blink-reviews, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, zoltan1, feature-media-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Remove <video> width and height attributes from intrisic size logic The width/height content attributes already influence specified style via HTMLVideoElement::collectStyleForPresentationAttribute, to also influence the intrinsic size has never been part of the spec: http://whatwg.org/html#the-video-element The test case passes in Firefox Nightly and IE11 Release Preview, but fails in Opera Presto, which has no default intrinsic size. Opera is actually correct per spec, but fixing Blink can be done in a separate bug. BUG=310122 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160491

Patch Set 1 #

Total comments: 8

Patch Set 2 : expanded test #

Total comments: 2

Patch Set 3 : learn to indent #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -6 lines) Patch
A LayoutTests/media/video-intrinsic-width-height.html View 1 2 1 chunk +39 lines, -0 lines 0 comments Download
A LayoutTests/media/video-intrinsic-width-height-expected.txt View 1 1 chunk +8 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderVideo.cpp View 1 chunk +0 lines, -6 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
philipj_slow
7 years, 2 months ago (2013-10-23 22:00:17 UTC) #1
adamk
7 years, 2 months ago (2013-10-23 22:02:13 UTC) #2
acolwell GONE FROM CHROMIUM
https://codereview.chromium.org/38313002/diff/1/LayoutTests/media/video-no-intrinsic-width-height.html File LayoutTests/media/video-no-intrinsic-width-height.html (right): https://codereview.chromium.org/38313002/diff/1/LayoutTests/media/video-no-intrinsic-width-height.html#newcode2 LayoutTests/media/video-no-intrinsic-width-height.html:2: <script src="../resources/testharness.js"></script> nits: please add <html>,<head>, and <body> in ...
7 years, 2 months ago (2013-10-23 23:40:05 UTC) #3
philipj_slow
Thanks Aaron, PTAL. https://codereview.chromium.org/38313002/diff/1/LayoutTests/media/video-no-intrinsic-width-height.html File LayoutTests/media/video-no-intrinsic-width-height.html (right): https://codereview.chromium.org/38313002/diff/1/LayoutTests/media/video-no-intrinsic-width-height.html#newcode2 LayoutTests/media/video-no-intrinsic-width-height.html:2: <script src="../resources/testharness.js"></script> On 2013/10/23 23:40:06, acolwell ...
7 years, 2 months ago (2013-10-24 06:07:22 UTC) #4
acolwell GONE FROM CHROMIUM
lgtm % nit https://codereview.chromium.org/38313002/diff/20001/LayoutTests/media/video-intrinsic-width-height.html File LayoutTests/media/video-intrinsic-width-height.html (right): https://codereview.chromium.org/38313002/diff/20001/LayoutTests/media/video-intrinsic-width-height.html#newcode30 LayoutTests/media/video-intrinsic-width-height.html:30: { nit: {} should not be ...
7 years, 2 months ago (2013-10-24 17:19:32 UTC) #5
adamk
rs=me lgtm
7 years, 2 months ago (2013-10-24 17:52:39 UTC) #6
philipj_slow
https://codereview.chromium.org/38313002/diff/20001/LayoutTests/media/video-intrinsic-width-height.html File LayoutTests/media/video-intrinsic-width-height.html (right): https://codereview.chromium.org/38313002/diff/20001/LayoutTests/media/video-intrinsic-width-height.html#newcode30 LayoutTests/media/video-intrinsic-width-height.html:30: { On 2013/10/24 17:19:32, acolwell wrote: > nit: {} ...
7 years, 2 months ago (2013-10-24 19:26:06 UTC) #7
philipj_slow
https://codereview.chromium.org/38313002/diff/20001/LayoutTests/media/video-intrinsic-width-height.html File LayoutTests/media/video-intrinsic-width-height.html (right): https://codereview.chromium.org/38313002/diff/20001/LayoutTests/media/video-intrinsic-width-height.html#newcode30 LayoutTests/media/video-intrinsic-width-height.html:30: { On 2013/10/24 17:19:32, acolwell wrote: > nit: {} ...
7 years, 2 months ago (2013-10-24 19:26:06 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/philipj@opera.com/38313002/80001
7 years, 2 months ago (2013-10-24 19:27:26 UTC) #9
commit-bot: I haz the power
Change committed as 160491
7 years, 2 months ago (2013-10-24 20:28:24 UTC) #10
eric.carlson
This comment in the test is incorrect: These tests assume that the default intrinsic width ...
7 years ago (2013-12-17 17:30:52 UTC) #11
philipj_slow
6 years, 11 months ago (2014-01-13 10:58:34 UTC) #12
Message was sent while issue was closed. 
On 2013/12/17 17:30:52, eric.carlson wrote:
> This comment in the test is incorrect:
> 
>     These tests assume that the default intrinsic width is 300x150, so that
the
> default
>     intrinsic ratio is 2:1. This is no longer per spec, but is what is
> implemented.
> 
> The spec still says the intrinsic width and height of a <video> element is
> 300x150 if the poster and media file dimensions are unavailable:
>
http://www.w3.org/TR/2013/WD-html51-20130528/embedded-content-0.html#concept-...

It looks like the spec changed since May, see
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...

It says "The default object size is a width of 300 CSS pixels and a height of
150 CSS pixels." and falls back to "otherwise the intrinsic width is missing"
for the intrinsic width.

Powered by Google App Engine
This is Rietveld 408576698