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

Issue 983073002: Make font-size-adjust animatable. (Closed)

Created:
5 years, 9 months ago by changseok
Modified:
5 years, 7 months ago
CC:
dstockwell, darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-css, blink-reviews-style_chromium.org, dglazkov+blink, ed+blinkwatch_opera.com, Mike Lawther (Google), rjwright, rwlbuis, shans, Steve Block
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Make font-size-adjust animatable. This change brings animation support to the font-size-adjust css property. BUG=451346 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192116

Patch Set 1 #

Total comments: 9

Patch Set 2 : Removed unnecessary code and comments. #

Patch Set 3 : Updated the comment in FontBuilder.cpp #

Patch Set 4 : Do not allow font-size-adjust to animate for none or 0 #

Total comments: 13

Patch Set 5 : Just rebased. #

Patch Set 6 : Addressed comments #

Total comments: 2

Patch Set 7 : Rebased #

Patch Set 8 : Changed to use fallBackToLegacy in StringKeyframe.cpp #

Total comments: 2

Patch Set 9 : Rebased. #

Patch Set 10 : Rebase expectations for bug2886-2.html #

Unified diffs Side-by-side diffs Delta from patch set Stats (+203 lines, -65 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/animations/interpolation/font-size-adjust-interpolation.html View 1 2 3 4 5 1 chunk +72 lines, -0 lines 0 comments Download
A LayoutTests/animations/interpolation/font-size-adjust-interpolation-expected.txt View 1 2 3 4 5 1 chunk +46 lines, -0 lines 0 comments Download
M LayoutTests/platform/mac/tables/mozilla/bugs/bug2886-2-expected.png View 1 2 3 4 5 6 7 8 9 Binary file 0 comments Download
M LayoutTests/platform/mac/tables/mozilla/bugs/bug2886-2-expected.txt View 1 2 3 4 5 6 7 8 9 1 chunk +64 lines, -64 lines 0 comments Download
M Source/core/animation/StringKeyframe.cpp View 1 2 3 4 5 6 7 2 chunks +6 lines, -0 lines 0 comments Download
M Source/core/animation/css/CSSAnimatableValueFactory.cpp View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/animation/css/CSSPropertyEquality.cpp View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/CSSProperties.in View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/resolver/AnimatedStyleBuilder.cpp View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/css/resolver/FontBuilder.cpp View 1 2 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 40 (10 generated)
changseok
Please have a look.
5 years, 9 months ago (2015-03-06 09:25:07 UTC) #2
rune
https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CSSAnimatableValueFactory.cpp File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right): https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CSSAnimatableValueFactory.cpp#newcode383 Source/core/animation/css/CSSAnimatableValueFactory.cpp:383: return createFromDouble(style.fontSizeAdjust()); Shouldn't you use the font's aspect value ...
5 years, 9 months ago (2015-03-06 10:08:06 UTC) #3
rune
In the CL description skip "an" in "brings an animation support".
5 years, 9 months ago (2015-03-06 10:09:11 UTC) #4
changseok
https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CSSAnimatableValueFactory.cpp File Source/core/animation/css/CSSAnimatableValueFactory.cpp (right): https://codereview.chromium.org/983073002/diff/1/Source/core/animation/css/CSSAnimatableValueFactory.cpp#newcode383 Source/core/animation/css/CSSAnimatableValueFactory.cpp:383: return createFromDouble(style.fontSizeAdjust()); On 2015/03/06 10:08:05, rune wrote: > Shouldn't ...
5 years, 9 months ago (2015-03-08 06:16:56 UTC) #5
Eric Willigers
Please consider adding a case in StringKeyframe.cpp so font-size-adjust can be animated using interpolable values. ...
5 years, 9 months ago (2015-03-08 07:10:15 UTC) #7
changseok
On 2015/03/08 07:10:15, Eric Willigers wrote: > Please consider adding a case in StringKeyframe.cpp so ...
5 years, 9 months ago (2015-03-08 08:18:45 UTC) #8
Timothy Loh
This (and the patch adding font-size-adjust support) seem a bit weird with respect to 'none'. ...
5 years, 9 months ago (2015-03-08 23:44:48 UTC) #9
rune
On 2015/03/08 23:44:48, Timothy Loh wrote: > This (and the patch adding font-size-adjust support) seem ...
5 years, 9 months ago (2015-03-09 00:02:07 UTC) #10
Timothy Loh
On 2015/03/09 00:02:07, rune (OOO) wrote: > On 2015/03/08 23:44:48, Timothy Loh wrote: > > ...
5 years, 9 months ago (2015-03-09 00:21:30 UTC) #11
Eric Willigers
> I saw the StringKeyframe class and tried it for a while. But I could ...
5 years, 9 months ago (2015-03-09 02:49:07 UTC) #12
changseok
On 2015/03/09 00:21:30, Timothy Loh wrote: > You can't really make it animate smoothly unless ...
5 years, 9 months ago (2015-03-10 17:51:20 UTC) #13
changseok
Any comments? =)
5 years, 9 months ago (2015-03-11 15:43:28 UTC) #14
changseok
Eric@, rune@ Do you have any concern? I'd like to make this go further.
5 years, 9 months ago (2015-03-13 16:08:35 UTC) #15
rune
On 2015/03/13 16:08:35, changseok wrote: > Eric@, rune@ > Do you have any concern? I'd ...
5 years, 9 months ago (2015-03-16 12:46:44 UTC) #16
changseok
On 2015/03/16 12:46:44, rune wrote: > I'll defer to the owners of core/animation rune, thanks ...
5 years, 9 months ago (2015-03-16 16:12:09 UTC) #18
dstockwell
Seems like there's still confusion between 0 and none. https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/interpolation/font-size-adjust-interpolation.html File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/interpolation/font-size-adjust-interpolation.html#newcode36 LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:36: ...
5 years, 9 months ago (2015-03-16 21:49:31 UTC) #19
changseok
https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/interpolation/font-size-adjust-interpolation.html File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/interpolation/font-size-adjust-interpolation.html#newcode36 LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:36: {at: -2, is: 'none'}, On 2015/03/16 21:49:31, dstockwell wrote: ...
5 years, 9 months ago (2015-03-17 07:12:58 UTC) #20
dstockwell
https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/interpolation/font-size-adjust-interpolation.html File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/interpolation/font-size-adjust-interpolation.html#newcode36 LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:36: {at: -2, is: 'none'}, On 2015/03/17 at 07:12:58, changseok ...
5 years, 9 months ago (2015-03-17 07:42:10 UTC) #21
changseok
Thanks for the review. It was quite helpful. I updated a patch to address comments. ...
5 years, 9 months ago (2015-03-17 11:35:06 UTC) #22
dstockwell
https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/interpolation/font-size-adjust-interpolation.html File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/60001/LayoutTests/animations/interpolation/font-size-adjust-interpolation.html#newcode36 LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:36: {at: -2, is: 'none'}, On 2015/03/17 at 11:35:06, changseok ...
5 years, 9 months ago (2015-03-17 23:36:46 UTC) #23
changseok
> OK, but please file the bug and start the discussion with the CSS Working ...
5 years, 9 months ago (2015-03-18 09:05:03 UTC) #24
dstockwell
lgtm On 2015/03/18 at 09:05:03, shivamidow wrote: > > OK, but please file the bug ...
5 years, 9 months ago (2015-03-18 09:29:41 UTC) #25
changseok
Thanks all. =) https://codereview.chromium.org/983073002/diff/140001/LayoutTests/animations/interpolation/font-size-adjust-interpolation.html File LayoutTests/animations/interpolation/font-size-adjust-interpolation.html (right): https://codereview.chromium.org/983073002/diff/140001/LayoutTests/animations/interpolation/font-size-adjust-interpolation.html#newcode65 LayoutTests/animations/interpolation/font-size-adjust-interpolation.html:65: {at: -0.3, is: 'none'}, On 2015/03/18 ...
5 years, 9 months ago (2015-03-18 09:58:37 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/983073002/140001
5 years, 9 months ago (2015-03-18 10:20:39 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/55530)
5 years, 9 months ago (2015-03-18 12:57:18 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/983073002/160001
5 years, 9 months ago (2015-03-18 15:15:24 UTC) #33
changseok
Well I got it why bots are unhappy with this cl. > failures: > tables/mozilla/bugs/bug2886-2.html ...
5 years, 9 months ago (2015-03-18 17:02:40 UTC) #35
changseok
On 2015/03/18 17:02:40, changseok wrote: > Well I got it why bots are unhappy with ...
5 years, 9 months ago (2015-03-18 17:12:30 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/983073002/160002
5 years, 9 months ago (2015-03-18 17:19:46 UTC) #39
commit-bot: I haz the power
5 years, 9 months ago (2015-03-18 20:13:22 UTC) #40
Message was sent while issue was closed.
Committed patchset #10 (id:160002) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192116

Powered by Google App Engine
This is Rietveld 408576698