Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696293002:
Adds in-out easing type. (Closed)
DescriptionAdds in-out easing type.
BUG=691610
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2696293002
Cr-Commit-Position: refs/heads/master@{#452664}
Committed: https://chromium.googlesource.com/chromium/src/+/1afab6869b24fbaaae6831564ae9ca68bf3e9883
Patch Set 1 #
Total comments: 12
Patch Set 2 : MakeUnique, default value, comments #
Total comments: 1
Patch Set 3 : Replaced ease_out_ with ease_in_ #Patch Set 4 : Rebased to ToT #Patch Set 5 : Rebased to ToT #
Messages
Total messages: 27 (12 generated)
Description was changed from ========== Adds in-out easing type. BUG=691610 ========== to ========== Adds in-out easing type. BUG=691610 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tiborg@chromium.org changed reviewers: + cjgrant@chromium.org, mthiesse@chromium.org
https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/easing.h (right): https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/easing.h:19: EASEINOUT nit: Should have a comma as well. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/easing.h:82: // Behaves like EaseIn for inputs smaller 0.5 and like EaseOut otherwise. s/smaller/smaller than/. But I found the comment a bit confusing, in that it sounded like a different curve for small movements. Maybe just stay "Starts with EaseIn and finishes with EaseOut.", if you think that's better. https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/ui_scene.cc (right): https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/ui_scene.cc:180: result.reset(new easing::EaseInOut(pow)); Consider moving this function (or file!) to use: result = base::MakeUnique<easing::EaseInOut>(pow) See base::MakeUnique docs. https://codereview.chromium.org/2696293002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2696293002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:99: anim.setEasing(new api.InOutEasing(2)); We should probably have a module level constant EASING or EASING_POWER in place of '2'.
https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/easing.cc (right): https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/easing.cc:41: return ease_out_.CalculateValueImpl((state - 0.5) * 2) / 2 + 0.5; Could reuse ease_in_, i.e. return 1.0 - (ease_in_.CalculateValueImpl((1.0 - state) * 2) / 2); (Though double check I got the math right :P)
https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/easing.cc (right): https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/easing.cc:41: return ease_out_.CalculateValueImpl((state - 0.5) * 2) / 2 + 0.5; On 2017/02/16 15:36:43, mthiesse wrote: > Could reuse ease_in_, i.e. return 1.0 - (ease_in_.CalculateValueImpl((1.0 - > state) * 2) / 2); > > (Though double check I got the math right :P) Good call. Math works. Should we do the same for EaseOut, i.e. reuse EaseIn in it? https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/easing.h (right): https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/easing.h:19: EASEINOUT On 2017/02/16 15:10:25, cjgrant wrote: > nit: Should have a comma as well. > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Done. https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/easing.h:82: // Behaves like EaseIn for inputs smaller 0.5 and like EaseOut otherwise. On 2017/02/16 15:10:25, cjgrant wrote: > s/smaller/smaller than/. But I found the comment a bit confusing, in that it > sounded like a different curve for small movements. Maybe just stay "Starts > with EaseIn and finishes with EaseOut.", if you think that's better. Done. https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/ui_scene.cc (right): https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/ui_scene.cc:180: result.reset(new easing::EaseInOut(pow)); On 2017/02/16 15:10:25, cjgrant wrote: > Consider moving this function (or file!) to use: > > result = base::MakeUnique<easing::EaseInOut>(pow) > > See base::MakeUnique docs. Done. https://codereview.chromium.org/2696293002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2696293002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:99: anim.setEasing(new api.InOutEasing(2)); On 2017/02/16 15:10:25, cjgrant wrote: > We should probably have a module level constant EASING or EASING_POWER in place > of '2'. Set default values for the easing. So, we don't need the number here anymore.
https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/easing.cc (right): https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/easing.cc:41: return ease_out_.CalculateValueImpl((state - 0.5) * 2) / 2 + 0.5; On 2017/02/16 18:39:33, tiborg wrote: > On 2017/02/16 15:36:43, mthiesse wrote: > > Could reuse ease_in_, i.e. return 1.0 - (ease_in_.CalculateValueImpl((1.0 - > > state) * 2) / 2); > > > > (Though double check I got the math right :P) > > Good call. Math works. Should we do the same for EaseOut, i.e. reuse EaseIn in > it? I wouldn't bother, I don't think that would simplify anything. If you'd like to though, go for it.
lgtm https://codereview.chromium.org/2696293002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2696293002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:99: anim.setEasing(new api.InOutEasing(2)); On 2017/02/16 18:39:33, tiborg wrote: > On 2017/02/16 15:10:25, cjgrant wrote: > > We should probably have a module level constant EASING or EASING_POWER in > place > > of '2'. > > Set default values for the easing. So, we don't need the number here anymore. Acknowledged. https://codereview.chromium.org/2696293002/diff/20001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): https://codereview.chromium.org/2696293002/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:399: constructor(pow = DEFAULT_EASING_POW) { nit: On POW vs POWER: https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Probably not worth changing this here; just an FYI for code you add later.
On 2017/02/16 19:15:07, mthiesse wrote: > https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... > File chrome/browser/android/vr_shell/easing.cc (right): > > https://codereview.chromium.org/2696293002/diff/1/chrome/browser/android/vr_s... > chrome/browser/android/vr_shell/easing.cc:41: return > ease_out_.CalculateValueImpl((state - 0.5) * 2) / 2 + 0.5; > On 2017/02/16 18:39:33, tiborg wrote: > > On 2017/02/16 15:36:43, mthiesse wrote: > > > Could reuse ease_in_, i.e. return 1.0 - (ease_in_.CalculateValueImpl((1.0 - > > > state) * 2) / 2); > > > > > > (Though double check I got the math right :P) > > > > Good call. Math works. Should we do the same for EaseOut, i.e. reuse EaseIn in > > it? > > I wouldn't bother, I don't think that would simplify anything. If you'd like to > though, go for it. Ok, replaced ease_out_ with ease_in_
On 2017/02/16 20:54:07, cjgrant wrote: > lgtm > > https://codereview.chromium.org/2696293002/diff/1/chrome/browser/resources/vr... > File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): > > https://codereview.chromium.org/2696293002/diff/1/chrome/browser/resources/vr... > chrome/browser/resources/vr_shell/vr_shell_ui.js:99: anim.setEasing(new > api.InOutEasing(2)); > On 2017/02/16 18:39:33, tiborg wrote: > > On 2017/02/16 15:10:25, cjgrant wrote: > > > We should probably have a module level constant EASING or EASING_POWER in > > place > > > of '2'. > > > > Set default values for the easing. So, we don't need the number here anymore. > > Acknowledged. > > https://codereview.chromium.org/2696293002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): > > https://codereview.chromium.org/2696293002/diff/20001/chrome/browser/resource... > chrome/browser/resources/vr_shell/vr_shell_ui_api.js:399: constructor(pow = > DEFAULT_EASING_POW) { > nit: On POW vs POWER: > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > Probably not worth changing this here; just an FYI for code you add later. Ok, thanks. Acknowledged for next time.
lgtm
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cjgrant@chromium.org Link to the patchset: https://codereview.chromium.org/2696293002/#ps40001 (title: "Replaced ease_out_ with ease_in_")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, cjgrant@chromium.org Link to the patchset: https://codereview.chromium.org/2696293002/#ps60001 (title: "Rebased to ToT")
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_asan_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_...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_...)
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, cjgrant@chromium.org Link to the patchset: https://codereview.chromium.org/2696293002/#ps80001 (title: "Rebased to ToT")
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": 80001, "attempt_start_ts": 1487886702959460,
"parent_rev": "2e27deaf15dd4cd8462c122479c7c15f69faa80d", "commit_rev":
"1afab6869b24fbaaae6831564ae9ca68bf3e9883"}
Message was sent while issue was closed.
Description was changed from ========== Adds in-out easing type. BUG=691610 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Adds in-out easing type. BUG=691610 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2696293002 Cr-Commit-Position: refs/heads/master@{#452664} Committed: https://chromium.googlesource.com/chromium/src/+/1afab6869b24fbaaae6831564ae9... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1afab6869b24fbaaae6831564ae9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
