Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696273002:
Can more conveniently set easing from JS. (Closed)
DescriptionCan more conveniently set easing from JS.
- Adds setEasing to Aninmation class.
- Adds classes to instantiate easings with the appropriate parameters.
BUG=691604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2696273002
Cr-Commit-Position: refs/heads/master@{#452082}
Committed: https://chromium.googlesource.com/chromium/src/+/c74bd60acf193d5ef22e4caeb9c8b63bb6d2d46d
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added defaults, added @abstract, removed animation #
Dependent Patchsets: Messages
Total messages: 18 (9 generated)
Description was changed from ========== Can more conveniently set easing from JS. - Added setEasing to Aninmation class. - Added classes to instantiate easings with the appropriate parameters. BUG=691604 ========== to ========== Can more conveniently set easing from JS. - Added setEasing to Aninmation class. - Added classes to instantiate easings with the appropriate parameters. BUG=691604 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tiborg@chromium.org changed reviewers: + cjgrant@chromium.org, mthiesse@chromium.org
lgtm after a couple tweaks. For the commit message - it's convention to have a blank line between the first line of the commit message and any other content. I'm used to this being enforced by the tools, but maybe it's not here? (see all other git commit messages) https://codereview.chromium.org/2696273002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2696273002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:102: anim.setEasing(new api.InEasing(2)); Was this meant to be included? https://codereview.chromium.org/2696273002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): https://codereview.chromium.org/2696273002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:345: * @abstract Nice! I missed that @abstract exists. Could you apply it to NativeCommandHandler as well?
https://codereview.chromium.org/2696273002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): https://codereview.chromium.org/2696273002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:360: constructor(p1x, p1y, p2x, p2y) { We should provide reasonable defaults for all easing parameters. Josh originally pushed back on even having them as part of the API and wanted us to just provide good values natively. At least if there are defaults here he can tweak them.
Description was changed from ========== Can more conveniently set easing from JS. - Added setEasing to Aninmation class. - Added classes to instantiate easings with the appropriate parameters. BUG=691604 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Can more conveniently set easing from JS. - Adds setEasing to Aninmation class. - Adds classes to instantiate easings with the appropriate parameters. BUG=691604 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2696273002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2696273002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:102: anim.setEasing(new api.InEasing(2)); On 2017/02/16 14:51:47, cjgrant wrote: > Was this meant to be included? Oops, no. Removed. https://codereview.chromium.org/2696273002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): https://codereview.chromium.org/2696273002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:345: * @abstract On 2017/02/16 14:51:47, cjgrant wrote: > Nice! I missed that @abstract exists. Could you apply it to > NativeCommandHandler as well? Done. Also added to fill class. https://codereview.chromium.org/2696273002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:360: constructor(p1x, p1y, p2x, p2y) { On 2017/02/16 15:12:06, mthiesse wrote: > We should provide reasonable defaults for all easing parameters. Josh originally > pushed back on even having them as part of the API and wanted us to just provide > good values natively. At least if there are defaults here he can tweak them. Makes sense. Done.
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/2696273002/#ps20001 (title: "Added defaults, added @abstract, removed animation")
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by tiborg@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": 20001, "attempt_start_ts": 1487776835330230,
"parent_rev": "e1fcfcddcd90de5017009feefa7e18716c450f4e", "commit_rev":
"c74bd60acf193d5ef22e4caeb9c8b63bb6d2d46d"}
Message was sent while issue was closed.
Description was changed from ========== Can more conveniently set easing from JS. - Adds setEasing to Aninmation class. - Adds classes to instantiate easings with the appropriate parameters. BUG=691604 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Can more conveniently set easing from JS. - Adds setEasing to Aninmation class. - Adds classes to instantiate easings with the appropriate parameters. BUG=691604 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2696273002 Cr-Commit-Position: refs/heads/master@{#452082} Committed: https://chromium.googlesource.com/chromium/src/+/c74bd60acf193d5ef22e4caeb9c8... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c74bd60acf193d5ef22e4caeb9c8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
