Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2870643003:
Add support for animations in vector icons. (Closed)
|
Created:
3 years, 7 months ago by Evan Stade Modified:
3 years, 7 months ago CC:
chromium-reviews, sadrul Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for animations in vector icons.
Adds the ability to transition from one path to another, interpolatable
path, as well as transitioning some paint flags. Support for
transitioning other paint flags can be added as necessary. Other useful
transition operations might include applying a transform, which will
require adding a transform element but should otherwise not present an
issue.
BUG=718549
Review-Url: https://codereview.chromium.org/2870643003
Cr-Commit-Position: refs/heads/master@{#471314}
Committed: https://chromium.googlesource.com/chromium/src/+/cc104c522c7cd341182415921253445e411b7e37
Patch Set 1 #
Total comments: 14
Patch Set 2 : docs #
Total comments: 9
Patch Set 3 : docs #Patch Set 4 : typo fixed #Patch Set 5 : another typo #Patch Set 6 : rebase #Patch Set 7 : . #Patch Set 8 : what happened #
Total comments: 9
Patch Set 9 : sadrul review #Patch Set 10 : update docs #Patch Set 11 : deps #Patch Set 12 : trace event #Patch Set 13 : deps, casting #
Messages
Total messages: 46 (28 generated)
estade@chromium.org changed reviewers: + tdanderson@chromium.org
depends on https://codereview.chromium.org/2861203002/
https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:14: #include "base/strings/string_number_conversions.h" general question: are the performance characteristics of your overall approach satisfactory to owner(s) of ui/views and/or chrome/browser/ui/views? I am happy with the approach you are taking here in ui/gfx but cannot speak to or anticipate objections you may receive from sadrul, etc, once you start landing and using moticons. Might be worth a chat if you haven't done so already. https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:394: Tween::CalculateValue(static_cast<Tween::Type>(arg(2)), state); I recognize that allowing non-numeric arguments would be a bit of a pain. But I'm a bit concerned that this relies on the Tween::Type enum a) never having anything removed and b) only ever having things added to the end. Do you have a longer-term plan for handling non-numeric args like this one? If you do end up going this route, consider adding a test to tween_unittest.cc that will fail if anyone removes from / adds to the middle of the enum. https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:601: base::TimeDelta GetDurationOfAnimation(const VectorIcon& icon) { I don't see any place in this CL where you are using this function. I suggest omitting here and re-introducing in the CL where it is first used. https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.h File ui/gfx/paint_vector_icon.h (right): https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.h#... ui/gfx/paint_vector_icon.h:21: // used as the fill. The size will come from the .icon file (the 1x version, if nit: update docs for |elapsed_time|. https://codereview.chromium.org/2870643003/diff/1/ui/gfx/vector_icon_types.h File ui/gfx/vector_icon_types.h (right): https://codereview.chromium.org/2870643003/diff/1/ui/gfx/vector_icon_types.h#... ui/gfx/vector_icon_types.h:61: // Defines a timed transition for other elements. Consider providing a mini-explainer here, since unlike the commands on lines 35-50, these do not map 1:1 to something I can easily find in the SVG spec. I would suggest highlighting: * the fact that the verbs and argument counts must be identical in the _FROM and _TO blocks * specify the arguments for _END * clarify that the first two arguments for _END are relative to the same t=0 starting time, i.e., they are not relative to the ending time of the previous transition.
https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:14: #include "base/strings/string_number_conversions.h" On 2017/05/08 21:22:54, tdanderson wrote: > general question: are the performance characteristics of your overall approach > satisfactory to owner(s) of ui/views and/or chrome/browser/ui/views? I am happy > with the approach you are taking here in ui/gfx but cannot speak to or > anticipate objections you may receive from sadrul, etc, once you start landing > and using moticons. Might be worth a chat if you haven't done so already. I don't see why they wouldn't be acceptable. We make copious use of gfx::Animations that do re-layout and/or paint at every step of the animation. What additional concern does this overall approach raise? https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:394: Tween::CalculateValue(static_cast<Tween::Type>(arg(2)), state); On 2017/05/08 21:22:54, tdanderson wrote: > I recognize that allowing non-numeric arguments would be a bit of a pain. But > I'm a bit concerned that this relies on the Tween::Type enum a) never having > anything removed and b) only ever having things added to the end. Do you have a > longer-term plan for handling non-numeric args like this one? > > If you do end up going this route, consider adding a test to tween_unittest.cc > that will fail if anyone removes from / adds to the middle of the enum. I don't think that's necessary. I haven't actually tried to write an icon with a tween type yet (in all my testing I just hard-coded the tween) but I expect to write the tween value directly into the icon definition, i.e. TRANSITION_END, 1000, 250, gfx::Tween::EASE_OUT, and if that doesn't compile I'm sure we can just add another constructor for PathElement. https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:601: base::TimeDelta GetDurationOfAnimation(const VectorIcon& icon) { On 2017/05/08 21:22:54, tdanderson wrote: > I don't see any place in this CL where you are using this function. I suggest > omitting here and re-introducing in the CL where it is first used. You could make the same argument for the transition commands. The code around there is completely dead until we actually have icons using them. I have one very large CL which adds this functionality and makes use of it, and I shared it so you could see the overall direction this was going if you were curious, but it's unwieldy to review. https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.h File ui/gfx/paint_vector_icon.h (right): https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.h#... ui/gfx/paint_vector_icon.h:21: // used as the fill. The size will come from the .icon file (the 1x version, if On 2017/05/08 21:22:54, tdanderson wrote: > nit: update docs for |elapsed_time|. Done. https://codereview.chromium.org/2870643003/diff/1/ui/gfx/vector_icon_types.h File ui/gfx/vector_icon_types.h (right): https://codereview.chromium.org/2870643003/diff/1/ui/gfx/vector_icon_types.h#... ui/gfx/vector_icon_types.h:61: // Defines a timed transition for other elements. On 2017/05/08 21:22:54, tdanderson wrote: > Consider providing a mini-explainer here, since unlike the commands on lines > 35-50, these do not map 1:1 to something I can easily find in the SVG spec. I > would suggest highlighting: > > * the fact that the verbs and argument counts must be identical in the _FROM and > _TO blocks > * specify the arguments for _END > * clarify that the first two arguments for _END are relative to the same t=0 > starting time, i.e., they are not relative to the ending time of the previous > transition. The doc I shared kinda explains this, and I plan to copy over parts of that into the developer page at https://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui (which should itself maybe be ported to a README.md). But I didn't want to do that before the code was landed and in use and we had some degree of certainty it was not going to change further.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Please see my comments below. Adding sadrul@ as FYI. https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:14: #include "base/strings/string_number_conversions.h" On 2017/05/08 22:27:41, Evan Stade wrote: > On 2017/05/08 21:22:54, tdanderson wrote: > > general question: are the performance characteristics of your overall approach > > satisfactory to owner(s) of ui/views and/or chrome/browser/ui/views? I am > happy > > with the approach you are taking here in ui/gfx but cannot speak to or > > anticipate objections you may receive from sadrul, etc, once you start landing > > and using moticons. Might be worth a chat if you haven't done so already. > > I don't see why they wouldn't be acceptable. We make copious use of > gfx::Animations that do re-layout and/or paint at every step of the animation. > What additional concern does this overall approach raise? I remember Sadrul noting some potential concerns in passing, though I do not recall the specifics. +sadrul@. (And note to myself: in the future, it will be helpful to supply additional details/motivation in code review comments such as this one, rather than just 'I think someone said something about this.') https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:394: Tween::CalculateValue(static_cast<Tween::Type>(arg(2)), state); On 2017/05/08 22:27:41, Evan Stade wrote: > On 2017/05/08 21:22:54, tdanderson wrote: > > I recognize that allowing non-numeric arguments would be a bit of a pain. But > > I'm a bit concerned that this relies on the Tween::Type enum a) never having > > anything removed and b) only ever having things added to the end. Do you have > a > > longer-term plan for handling non-numeric args like this one? > > > > If you do end up going this route, consider adding a test to tween_unittest.cc > > that will fail if anyone removes from / adds to the middle of the enum. > > I don't think that's necessary. I haven't actually tried to write an icon with a > tween type yet (in all my testing I just hard-coded the tween) but I expect to > write the tween value directly into the icon definition, i.e. > > TRANSITION_END, 1000, 250, gfx::Tween::EASE_OUT, > > and if that doesn't compile I'm sure we can just add another constructor for > PathElement. Acknowledged. https://codereview.chromium.org/2870643003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:601: base::TimeDelta GetDurationOfAnimation(const VectorIcon& icon) { On 2017/05/08 22:27:41, Evan Stade wrote: > On 2017/05/08 21:22:54, tdanderson wrote: > > I don't see any place in this CL where you are using this function. I suggest > > omitting here and re-introducing in the CL where it is first used. > > You could make the same argument for the transition commands. The code around > there is completely dead until we actually have icons using them. I have one > very large CL which adds this functionality and makes use of it, and I shared it > so you could see the overall direction this was going if you were curious, but > it's unwieldy to review. Acknowledged. https://codereview.chromium.org/2870643003/diff/1/ui/gfx/vector_icon_types.h File ui/gfx/vector_icon_types.h (right): https://codereview.chromium.org/2870643003/diff/1/ui/gfx/vector_icon_types.h#... ui/gfx/vector_icon_types.h:61: // Defines a timed transition for other elements. On 2017/05/08 22:27:42, Evan Stade wrote: > On 2017/05/08 21:22:54, tdanderson wrote: > > Consider providing a mini-explainer here, since unlike the commands on lines > > 35-50, these do not map 1:1 to something I can easily find in the SVG spec. I > > would suggest highlighting: > > > > * the fact that the verbs and argument counts must be identical in the _FROM > and > > _TO blocks > > * specify the arguments for _END > > * clarify that the first two arguments for _END are relative to the same t=0 > > starting time, i.e., they are not relative to the ending time of the previous > > transition. > > The doc I shared kinda explains this, and I plan to copy over parts of that into > the developer page at > https://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui > (which should itself maybe be ported to a README.md). But I didn't want to do > that before the code was landed and in use and we had some degree of certainty > it was not going to change further. This sg, so long as the plan is to have the above points explained in the developer page. https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.cc:358: start_new_path(); I think you want to push a copy of flags_array.back() onto |flags_array| here, then perform an extra flags_array.pop_back() after line 412. Consider: <A> TRANSITION_FROM <B> TRANSITION_TO <C> TRANSITION_END <D> As currently implemented, any changes to PaintFlags properties within <A> can be overwritten within <B>, and these changes will persist once <D> is reached. It seems we'd instead want to have <D> use only the PaintFlags properties specified within <A>. https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.cc:597: while (parser.Advance()) { You will need to change this condition since Advance() no longer returns a bool.
In case you do have input Sadrul, here's the explainer doc: https://docs.google.com/a/google.com/document/d/1qnPSE-Gq3rW8ty2aliurwB7hsEjW... And here's a larger CL that puts this code to use (shared for demonstration rather than review): https://codereview.chromium.org/2860163002/ https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.cc:358: start_new_path(); On 2017/05/09 20:31:40, tdanderson wrote: > I think you want to push a copy of flags_array.back() onto |flags_array| here, > then perform an extra flags_array.pop_back() after line 412. Consider: > > <A> > TRANSITION_FROM > <B> > TRANSITION_TO > <C> > TRANSITION_END > <D> > > As currently implemented, any changes to PaintFlags properties within <A> can be > overwritten within <B>, and these changes will persist once <D> is reached. It > seems we'd instead want to have <D> use only the PaintFlags properties specified > within <A>. The current implementation is correct. For pathing commands, the things in <B> and <C> have to be similar enough to be interpolatable (i.e. only differ in their arguments), but there's no such restriction for the non-pathing commands like color, AA, etc. <D> is not something that comes after the end of the transition. Both <A> and <D> in your example are things that hold true for any |elapsed_time|. (It doesn't make sense to specify the same property in both <A> and <D>, but that is nothing new; it already didn't make sense to use PATH_COLOR_ARGB twice in a row.) <B> is something that's true before |delay| and <C> is something that's true only after |delay|. Therefore we can just write <B> on top of the active flags and then blend <C> onto that depending on |elapsed_time|.
LGTM with the loop in GetDurationOfAnimation() fixed. https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.cc:358: start_new_path(); On 2017/05/09 21:25:04, Evan Stade wrote: > On 2017/05/09 20:31:40, tdanderson wrote: > > I think you want to push a copy of flags_array.back() onto |flags_array| here, > > then perform an extra flags_array.pop_back() after line 412. Consider: > > > > <A> > > TRANSITION_FROM > > <B> > > TRANSITION_TO > > <C> > > TRANSITION_END > > <D> > > > > As currently implemented, any changes to PaintFlags properties within <A> can > be > > overwritten within <B>, and these changes will persist once <D> is reached. It > > seems we'd instead want to have <D> use only the PaintFlags properties > specified > > within <A>. > > The current implementation is correct. For pathing commands, the things in <B> > and <C> have to be similar enough to be interpolatable (i.e. only differ in > their arguments), but there's no such restriction for the non-pathing commands > like color, AA, etc. > > <D> is not something that comes after the end of the transition. Both <A> and > <D> in your example are things that hold true for any |elapsed_time|. (It > doesn't make sense to specify the same property in both <A> and <D>, but that is > nothing new; it already didn't make sense to use PATH_COLOR_ARGB twice in a > row.) <B> is something that's true before |delay| and <C> is something that's > true only after |delay|. Therefore we can just write <B> on top of the active > flags and then blend <C> onto that depending on |elapsed_time|. This makes sense, thank you for the clear explanation. Mental model corrected accordingly.
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.cc:606: return last_motion; Can this be cached in VectorIcon? e.g. Optional<base::TimeDelta> animation_duration_? Having to parse for each call to this doesn't sound great. https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/vector_icon_types.h File ui/gfx/vector_icon_types.h (right): https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/vector_icon_type... ui/gfx/vector_icon_types.h:64: TRANSITION_END, Can you please document the parameters here?
https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.cc:597: while (parser.Advance()) { On 2017/05/09 20:31:40, tdanderson (OOO til 5-23) wrote: > You will need to change this condition since Advance() no longer returns a bool. Done. https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.cc:606: return last_motion; On 2017/05/09 22:12:03, sadrul wrote: > Can this be cached in VectorIcon? e.g. Optional<base::TimeDelta> > animation_duration_? Having to parse for each call to this doesn't sound great. It will be cached by the call site. I suppose I could create a static map in this file similar to what VectorIconCache does, but for animated icons we're going to be reading through the icon many times anyway and one more time doesn't seem like it will hurt (given that reading should be an extremely fast operation; we're just iterating over an array at most a few hundred elements long). https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/vector_icon_types.h File ui/gfx/vector_icon_types.h (right): https://codereview.chromium.org/2870643003/diff/20001/ui/gfx/vector_icon_type... ui/gfx/vector_icon_types.h:64: TRANSITION_END, On 2017/05/09 22:12:03, sadrul wrote: > Can you please document the parameters here? once the dust settles, I plan to update the docs on the chromium dev site for vector icons, and possible create a README.md. But I can add a short note here about the params as well.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Sadrul, ptal.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... ui/gfx/paint_vector_icon.cc:165: const base::TimeDelta* elapsed_time = nullptr) { Can this just be a const-ref? const base::TimeDelta& elapsed_time = base::TimeDelta()) { https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... ui/gfx/paint_vector_icon.cc:396: path1.interpolate(path2, weight, &interpolated_path); Do you have a sense of how costly this interpolation step is? Can this step have a TRACE_EVENT? https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... File ui/gfx/paint_vector_icon.h (right): https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... ui/gfx/paint_vector_icon.h:28: const base::TimeDelta* elapsed_time = nullptr); Make these const-refs (TimeDelta() being the default value). https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/vector_icon_typ... File ui/gfx/vector_icon_types.h (right): https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/vector_icon_typ... ui/gfx/vector_icon_types.h:64: // Parameters are delay (ms), duration (ms), and tween type. Please also include how the tween type is defined (e.g. string? some int value corresponding to some enum?)
https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... ui/gfx/paint_vector_icon.cc:165: const base::TimeDelta* elapsed_time = nullptr) { On 2017/05/11 04:22:55, sadrul wrote: > Can this just be a const-ref? > > const base::TimeDelta& elapsed_time = base::TimeDelta()) { Done. https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... ui/gfx/paint_vector_icon.cc:396: path1.interpolate(path2, weight, &interpolated_path); On 2017/05/11 04:22:55, sadrul wrote: > Do you have a sense of how costly this interpolation step is? Can this step have > a TRACE_EVENT? it should be cheap. For each argument it's just taking a weighted average of a pair of floats. Do you think that warrants TRACE_EVENT? https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... File ui/gfx/paint_vector_icon.h (right): https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... ui/gfx/paint_vector_icon.h:28: const base::TimeDelta* elapsed_time = nullptr); On 2017/05/11 04:22:55, sadrul wrote: > Make these const-refs (TimeDelta() being the default value). Done. https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/vector_icon_typ... File ui/gfx/vector_icon_types.h (right): https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/vector_icon_typ... ui/gfx/vector_icon_types.h:64: // Parameters are delay (ms), duration (ms), and tween type. On 2017/05/11 04:22:55, sadrul wrote: > Please also include how the tween type is defined (e.g. string? some int value > corresponding to some enum?) Done, but it seems that will be apparent once animated icons exist in the codebase (git grep TRANSITION_END will immediately drum up some examples to work from, e.g. TRANSITION_END, 100, 250, gfx::Tween::FAST_OUT_SLOW_IN,
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Re the build failures in the trybots: looks like there are several targets that include vector_icon_types.h (e.g. //chrome/app/vector_icons, //ash/resources/vector_icons etc.). Is it possible to create a separate target for vector-icons (e.g. //ui/gfx:vector_icons, or maybe even //ui/gfx/vector_icons), and dep on that target from all those various targets instead? https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... ui/gfx/paint_vector_icon.cc:396: path1.interpolate(path2, weight, &interpolated_path); On 2017/05/11 16:16:26, Evan Stade wrote: > On 2017/05/11 04:22:55, sadrul wrote: > > Do you have a sense of how costly this interpolation step is? Can this step > have > > a TRACE_EVENT? > > it should be cheap. For each argument it's just taking a weighted average of a > pair of floats. Do you think that warrants TRACE_EVENT? Let's add TRACE_EVENT now, and a TODO referencing some crbug to investigate removing this once we have some animated vector icons, if the cost turns out to be negligible.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/05/11 17:36:38, sadrul wrote: > Re the build failures in the trybots: looks like there are several targets that > include vector_icon_types.h (e.g. //chrome/app/vector_icons, > //ash/resources/vector_icons etc.). Is it possible to create a separate target > for vector-icons (e.g. //ui/gfx:vector_icons, or maybe even > //ui/gfx/vector_icons), and dep on that target from all those various targets > instead? Not sure I understand the benefit of having a new target. A couple DEPS were missing and I've added them; trybots seem to like it (there are some msvc complaints around casting that should be easy to resolve). We could instead depend on //ui/gfx (animation is a public_dep of that one). I can't recall if there was some reason we didn't do that in the first place. > > https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... > File ui/gfx/paint_vector_icon.cc (right): > > https://codereview.chromium.org/2870643003/diff/140001/ui/gfx/paint_vector_ic... > ui/gfx/paint_vector_icon.cc:396: path1.interpolate(path2, weight, > &interpolated_path); > On 2017/05/11 16:16:26, Evan Stade wrote: > > On 2017/05/11 04:22:55, sadrul wrote: > > > Do you have a sense of how costly this interpolation step is? Can this step > > have > > > a TRACE_EVENT? > > > > it should be cheap. For each argument it's just taking a weighted average of a > > pair of floats. Do you think that warrants TRACE_EVENT? > > Let's add TRACE_EVENT now, and a TODO referencing some crbug to investigate > removing this once we have some animated vector icons, if the cost turns out to > be negligible. done
lgtm On 2017/05/11 21:05:05, Evan Stade wrote: > On 2017/05/11 17:36:38, sadrul wrote: > > Re the build failures in the trybots: looks like there are several targets > that > > include vector_icon_types.h (e.g. //chrome/app/vector_icons, > > //ash/resources/vector_icons etc.). Is it possible to create a separate target > > for vector-icons (e.g. //ui/gfx:vector_icons, or maybe even > > //ui/gfx/vector_icons), and dep on that target from all those various targets > > instead? > > Not sure I understand the benefit of having a new target. A couple DEPS were > missing and I've added them; trybots seem to like it (there are some msvc > complaints around casting that should be easy to resolve). > > We could instead depend on //ui/gfx (animation is a public_dep of that one). I > can't recall if there was some reason we didn't do that in the first place. Sure, that would work too. But including the same file in multiple targets will result into this kind of issues, and I think the fix (either have a separate vector_icons targret, or dep'ing on gfx) is simple. So we should do that.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2870643003/#ps240001 (title: "deps, casting")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1494601581315510,
"parent_rev": "6d29b2b574b2c85043ef6953a3edb9f04c86ed58", "commit_rev":
"cc104c522c7cd341182415921253445e411b7e37"}
Message was sent while issue was closed.
Description was changed from ========== Add support for animations in vector icons. Adds the ability to transition from one path to another, interpolatable path, as well as transitioning some paint flags. Support for transitioning other paint flags can be added as necessary. Other useful transition operations might include applying a transform, which will require adding a transform element but should otherwise not present an issue. BUG=718549 ========== to ========== Add support for animations in vector icons. Adds the ability to transition from one path to another, interpolatable path, as well as transitioning some paint flags. Support for transitioning other paint flags can be added as necessary. Other useful transition operations might include applying a transform, which will require adding a transform element but should otherwise not present an issue. BUG=718549 Review-Url: https://codereview.chromium.org/2870643003 Cr-Commit-Position: refs/heads/master@{#471314} Committed: https://chromium.googlesource.com/chromium/src/+/cc104c522c7cd341182415921253... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/cc104c522c7cd341182415921253... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
