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

Issue 689613002: Disallow non-flexbox child renderers for RenderMedia (Closed)

Created:
6 years, 1 month ago by philipj_slow
Modified:
6 years, 1 month ago
CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1
Project:
blink
Visibility:
Public.

Description

Disallow non-flexbox child renderers for RenderMedia Inline children of replaced content is not handled and triggered the ASSERT(curr->isRenderInline()) UNKNOWN in RenderInline::splitInlines(). The render tree at the point of failure was: RenderView 0xd0c23404010 #document 0x2be215c04b48 RenderBlock 0xd0c2341c010 HTML 0x2be215c101a8 RenderBody 0xd0c2341c110 BODY 0x2be215c102b8 RenderBlock (anonymous) 0xd0c2341c310 RenderVideo 0xd0c23420010 VIDEO 0x2be215c48010 * RenderInline (relative positioned) 0xd0c23424010 DIV 0x2be215c68010 RenderBlock (anonymous) 0xd0c2341c210 RenderBlock (anonymous) 0xd0c2341c410 Making ::-webkit-media-controls* internal would have fixed this: https://codereview.chromium.org/662243003/ https://groups.google.com/a/chromium.org/d/msg/blink-dev/YCIaYPa_DhI/RyDlcHmD1_wJ Unfortunately, that is not without risk. This fix allows ::-webkit-media-controls { display: none; } to keep working until the usage can be measured. BUG=415407, 426759 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184652

Patch Set 1 #

Total comments: 4

Patch Set 2 : nits #

Total comments: 10

Patch Set 3 : test everything #

Total comments: 4

Patch Set 4 : z-index #

Unified diffs Side-by-side diffs Delta from patch set Stats (+80 lines, -0 lines) Patch
A LayoutTests/media/webkit-media-controls-display.html View 1 2 1 chunk +24 lines, -0 lines 0 comments Download
A LayoutTests/media/webkit-media-controls-display-expected.html View 1 2 1 chunk +24 lines, -0 lines 0 comments Download
A LayoutTests/media/webkit-media-controls-webkit-appearance.html View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/media/webkit-media-controls-webkit-appearance-expected.html View 1 2 3 1 chunk +13 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderMedia.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/RenderMedia.cpp View 1 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (7 generated)
philipj_slow
PTAL
6 years, 1 month ago (2014-10-29 10:04:32 UTC) #3
mstensho (USE GERRIT)
https://codereview.chromium.org/689613002/diff/1/LayoutTests/media/controls-css-overload.html File LayoutTests/media/controls-css-overload.html (right): https://codereview.chromium.org/689613002/diff/1/LayoutTests/media/controls-css-overload.html#newcode2 LayoutTests/media/controls-css-overload.html:2: <title>overloading controls CSS doesn't crash</title> Title not necessary. Also, ...
6 years, 1 month ago (2014-10-29 10:12:21 UTC) #4
rune
lgtm with nits. perhaps jchaffraix should have a say. https://codereview.chromium.org/689613002/diff/1/LayoutTests/media/controls-css-overload-expected.html File LayoutTests/media/controls-css-overload-expected.html (right): https://codereview.chromium.org/689613002/diff/1/LayoutTests/media/controls-css-overload-expected.html#newcode2 LayoutTests/media/controls-css-overload-expected.html:2: ...
6 years, 1 month ago (2014-10-29 10:14:19 UTC) #6
philipj_slow
nits
6 years, 1 month ago (2014-10-29 10:31:21 UTC) #7
philipj_slow
Julien, can you PTAL?
6 years, 1 month ago (2014-10-29 10:32:04 UTC) #10
Julien - ping for review
https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/RenderMedia.cpp File Source/core/rendering/RenderMedia.cpp (right): https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/RenderMedia.cpp#newcode86 Source/core/rendering/RenderMedia.cpp:86: // check can be removed if ::-webkit-media-controls is made ...
6 years, 1 month ago (2014-10-29 18:42:15 UTC) #12
philipj_slow
https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/RenderMedia.cpp File Source/core/rendering/RenderMedia.cpp (right): https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/RenderMedia.cpp#newcode86 Source/core/rendering/RenderMedia.cpp:86: // check can be removed if ::-webkit-media-controls is made ...
6 years, 1 month ago (2014-10-29 18:54:05 UTC) #13
philipj_slow
https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/RenderMedia.cpp File Source/core/rendering/RenderMedia.cpp (right): https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/RenderMedia.cpp#newcode86 Source/core/rendering/RenderMedia.cpp:86: // check can be removed if ::-webkit-media-controls is made ...
6 years, 1 month ago (2014-10-29 21:24:24 UTC) #14
Julien - ping for review
The direction is good, let's just tighten (and test!) the couple of cases I pointed ...
6 years, 1 month ago (2014-10-29 21:27:13 UTC) #15
philipj_slow
https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/RenderMedia.cpp File Source/core/rendering/RenderMedia.cpp (right): https://codereview.chromium.org/689613002/diff/20001/Source/core/rendering/RenderMedia.cpp#newcode87 Source/core/rendering/RenderMedia.cpp:87: return child->isFlexibleBox(); On 2014/10/29 21:27:12, Julien Chaffraix - PST ...
6 years, 1 month ago (2014-10-29 21:40:37 UTC) #16
philipj_slow
test everything
6 years, 1 month ago (2014-10-30 11:41:27 UTC) #17
philipj_slow
Rune, can you take a look at the new tests? I hope I covered everything ...
6 years, 1 month ago (2014-10-30 11:41:58 UTC) #18
rune
https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit-media-controls-display-expected.html File LayoutTests/media/webkit-media-controls-display-expected.html (right): https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit-media-controls-display-expected.html#newcode14 LayoutTests/media/webkit-media-controls-display-expected.html:14: var video = document.createElement("video"); I'd probably just write out ...
6 years, 1 month ago (2014-10-30 12:15:19 UTC) #19
philipj_slow
z-index
6 years, 1 month ago (2014-10-30 12:28:14 UTC) #20
philipj_slow
https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit-media-controls-display-expected.html File LayoutTests/media/webkit-media-controls-display-expected.html (right): https://codereview.chromium.org/689613002/diff/40001/LayoutTests/media/webkit-media-controls-display-expected.html#newcode14 LayoutTests/media/webkit-media-controls-display-expected.html:14: var video = document.createElement("video"); On 2014/10/30 12:15:19, rune wrote: ...
6 years, 1 month ago (2014-10-30 12:28:58 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/689613002/60001
6 years, 1 month ago (2014-10-30 12:30:01 UTC) #23
commit-bot: I haz the power
6 years, 1 month ago (2014-10-30 14:38:07 UTC) #24
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 184652

Powered by Google App Engine
This is Rietveld 408576698