Chromium Code Reviews
|
|
|
Created:
4 years, 11 months ago by alancutter (OOO until 2018) Modified:
4 years, 10 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-css, blink-reviews-style_chromium.org, dglazkov+blink, Eric Willigers, rjwright, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix crash when animating -webkit-{min,max}-content keyword
When animating -webkit-min-content and -webkit-max-content we snapshot
AnimatableValues off a ComputedStyle that have these values applied.
These are stored via a CSSPrimitiveValue conversion that converts them to
unprefixed min-content/max-content values. When these get applied during
animation we crash as these values have not been unprefixed yet.
This change adds support for applying unprefixed min-content/max-content
in StyleBuilder::applyProperty. They are still unsupported by the
CSSPropertyParser so this change does not ship the unprefixed values to CSS.
BUG=472883
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196933
Patch Set 1 #
Messages
Total messages: 18 (5 generated)
alancutter@chromium.org changed reviewers: + timloh@chromium.org
shans@chromium.org changed reviewers: + shans@chromium.org
Crashing is bad, but we're also committed to not supporting prefixed property animation through the Web Animations API. Does this violate that, or is there another check that prevents prefixed properties from getting this far?
On 2015/06/04 at 07:45:21, shans wrote: > Crashing is bad, but we're also committed to not supporting prefixed property animation through the Web Animations API. > > Does this violate that, or is there another check that prevents prefixed properties from getting this far? These are prefixed keywords rather than properties. We currently support prefixed values in Web Animations, should this not be the case?
On 2015/06/04 07:52:25, alancutter wrote: > On 2015/06/04 at 07:45:21, shans wrote: > > Crashing is bad, but we're also committed to not supporting prefixed property > animation through the Web Animations API. > > > > Does this violate that, or is there another check that prevents prefixed > properties from getting this far? > > These are prefixed keywords rather than properties. We currently support > prefixed values in Web Animations, should this not be the case? Good point. And .. I'm not sure, actually. Gut preference is that we shouldn't.
ok lgtm
The CQ bit was checked by alancutter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162593006/1
The CQ bit was unchecked by shans@chromium.org
On 2015/06/04 at 07:56:16, shans wrote: > On 2015/06/04 07:52:25, alancutter wrote: > > On 2015/06/04 at 07:45:21, shans wrote: > > > Crashing is bad, but we're also committed to not supporting prefixed property > > animation through the Web Animations API. > > > > > > Does this violate that, or is there another check that prevents prefixed > > properties from getting this far? > > > > These are prefixed keywords rather than properties. We currently support > > prefixed values in Web Animations, should this not be the case? > > Good point. And .. I'm not sure, actually. Gut preference is that we shouldn't. This is something we should resolve for Web Animations in a separate patch. For CSS Animations which supports prefixed keywords we should probably avoid crashing debug in this scenario.
On 2015/06/04 at 13:17:36, alancutter wrote: > On 2015/06/04 at 07:56:16, shans wrote: > > On 2015/06/04 07:52:25, alancutter wrote: > > > On 2015/06/04 at 07:45:21, shans wrote: > > > > Crashing is bad, but we're also committed to not supporting prefixed property > > > animation through the Web Animations API. > > > > > > > > Does this violate that, or is there another check that prevents prefixed > > > properties from getting this far? > > > > > > These are prefixed keywords rather than properties. We currently support > > > prefixed values in Web Animations, should this not be the case? > > > > Good point. And .. I'm not sure, actually. Gut preference is that we shouldn't. > > This is something we should resolve for Web Animations in a separate patch. > For CSS Animations which supports prefixed keywords we should probably avoid crashing debug in this scenario. To be clear this patch does not add animation support for -webkit-min/max-content, they are already supported on ToT in Web/CSS animations.
On 2015/06/04 13:25:29, alancutter wrote: > On 2015/06/04 at 13:17:36, alancutter wrote: > > On 2015/06/04 at 07:56:16, shans wrote: > > > On 2015/06/04 07:52:25, alancutter wrote: > > > > On 2015/06/04 at 07:45:21, shans wrote: > > > > > Crashing is bad, but we're also committed to not supporting prefixed > property > > > > animation through the Web Animations API. > > > > > > > > > > Does this violate that, or is there another check that prevents prefixed > > > > properties from getting this far? > > > > > > > > These are prefixed keywords rather than properties. We currently support > > > > prefixed values in Web Animations, should this not be the case? > > > > > > Good point. And .. I'm not sure, actually. Gut preference is that we > shouldn't. > > > > This is something we should resolve for Web Animations in a separate patch. > > For CSS Animations which supports prefixed keywords we should probably avoid > crashing debug in this scenario. If the resolution is global, sure (but you should amend this patch with a TODO). If it's per-property then I think you should do it here. On the other hand, maybe we'll talk about it and decide that we have no choice but to support -webkit prefixed values. In any of these cases, unless you're fixing some critical security bug in stable, delaying for 12 hours to discuss before committing is not going to hurt anything. > To be clear this patch does not add animation support for > -webkit-min/max-content, they are already supported on ToT in Web/CSS > animations. Uh, given that attempting to animate -webkit-min-content or -webkit-max-content currently crashes, clearly this patch does add animation support for them.
On 2015/06/04 at 20:49:48, shans wrote: > On 2015/06/04 13:25:29, alancutter wrote: > > On 2015/06/04 at 13:17:36, alancutter wrote: > > > On 2015/06/04 at 07:56:16, shans wrote: > > > > On 2015/06/04 07:52:25, alancutter wrote: > > > > > On 2015/06/04 at 07:45:21, shans wrote: > > > > > > Crashing is bad, but we're also committed to not supporting prefixed > > property > > > > > animation through the Web Animations API. > > > > > > > > > > > > Does this violate that, or is there another check that prevents prefixed > > > > > properties from getting this far? > > > > > > > > > > These are prefixed keywords rather than properties. We currently support > > > > > prefixed values in Web Animations, should this not be the case? > > > > > > > > Good point. And .. I'm not sure, actually. Gut preference is that we > > shouldn't. > > > > > > This is something we should resolve for Web Animations in a separate patch. > > > For CSS Animations which supports prefixed keywords we should probably avoid > > crashing debug in this scenario. > > If the resolution is global, sure (but you should amend this patch with a TODO). If it's per-property then I think you should do it here. On the other hand, maybe we'll talk about it and decide that we have no choice but to support -webkit prefixed values. In any of these cases, unless you're fixing some critical security bug in stable, delaying for 12 hours to discuss before committing is not going to hurt anything. > > > To be clear this patch does not add animation support for > > -webkit-min/max-content, they are already supported on ToT in Web/CSS > > animations. > > Uh, given that attempting to animate -webkit-min-content or -webkit-max-content currently crashes, clearly this patch does add animation support for them. It only crashes when we fall back to AnimatableValues (if one side requires style resolve).
Discussed this offline with shans and dstockwell. Came to the decision that Web Animations should 50% flip prefixed values while leaving CSS Animation behaviour as is. Given CSS Animations can hit this assert we decided to proceed with this crash fix.
The CQ bit was checked by alancutter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162593006/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=196933 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
