Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2592143002:
First cut of JS closure compiler use for VR HTML UI. (Closed)
|
Created:
4 years ago by cjgrant Modified:
3 years, 11 months ago CC:
chromium-reviews, vitalyp+closure_chromium.org, feature-vr-reviews_chromium.org, arv+watch_chromium.org, jlklein+watch-closure_chromium.org, dbeam+watch-closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFirst cut of JS closure compiler use for VR HTML UI.
Also, run clang-format to fix up styling.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/0cc9f99274349ffcf91788c58dfc0b3297bcd07a
Cr-Commit-Position: refs/heads/master@{#441437}
Patch Set 1 #
Total comments: 1
Patch Set 2 : API function type checking now works; need to ensure this new approach is okay. #Patch Set 3 : Remove test file. #
Total comments: 9
Patch Set 4 : Cleanup and more coverage. #Patch Set 5 : Relearn the alphabet. #Patch Set 6 : More cleanup... #
Total comments: 10
Patch Set 7 : Run clang-format so that dbeam@ doesn't have to fix our files; remove the third-party change to exp… #
Messages
Total messages: 27 (10 generated)
Description was changed from ========== Experimental: First cut of JS closure compiler use for VR HTML UI. BUG= ========== to ========== Experimental: First cut of JS closure compiler use for VR HTML UI. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
cjgrant@chromium.org changed reviewers: + amp@chromium.org
The compiler can be run via: third_party/closure_compiler/run_compiler vr_shell_ui
https://codereview.chromium.org/2592143002/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2592143002/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:175: update.setTranslation(0, -0.6, 'cat'); This is what I was playing with: - If I make a call like this from within vr_shell_ui_api.js, it complains about type. - If I break the name of this function, and try calling xSetTranslation(), it complains (ie. it knows what the update class offers). - But, this call works fine.
On 2016/12/21 17:10:51, cjgrant wrote: > https://codereview.chromium.org/2592143002/diff/1/chrome/browser/resources/vr... > File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): > > https://codereview.chromium.org/2592143002/diff/1/chrome/browser/resources/vr... > chrome/browser/resources/vr_shell/vr_shell_ui.js:175: update.setTranslation(0, > -0.6, 'cat'); > This is what I was playing with: > - If I make a call like this from within vr_shell_ui_api.js, it complains about > type. > - If I break the name of this function, and try calling xSetTranslation(), it > complains (ie. it knows what the update class offers). > - But, this call works fine. Thanks for the suggestion Adam. The latest patch set allows type checking. :)
cjgrant@chromium.org changed reviewers: + bshe@chromium.org
This is a great improvement for our javascript! There are more small things that could be addressed (in this change or future) and Chromium also limits what is possible, but how close to we want to be to the full js style guide https://google.github.io/styleguide/jsguide.html? https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:24: * @param {Array} commands Doesn't close support specifying the type of the objects in the array as well? Something like {!Array<TYPE>}. Are these commands just strings? https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:116: this.properties = {}; It looks like properties could be a well defined object. Maybe use a @typedef here to specify what it's properites are (like 'contentQuad', 'parentId', etc)? Or if things compile fine now I suppose that could be left to a future change. https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:122: this.varvar = 1; I can't remember if this was specifically restricted in Chromium javascript, but aren't private members supposed to end in '_'? Also, this varvar, and the comments seem to be copied from elsewhere... https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:126: * Operates on an instance of MyClass and returns something. Bad copy paste? https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:172: //this.setTranslation(1, 2, 'cat'); Remove comment.
https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:24: * @param {Array} commands On 2016/12/21 20:58:58, amp wrote: > Doesn't close support specifying the type of the objects in the array as well? > Something like {!Array<TYPE>}. > > Are these commands just strings? yes, you can templatize types in closure, so Array<T>, Object<T>, Promise<T>, etc.
Thanks guys! https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:24: * @param {Array} commands On 2016/12/22 00:18:21, Dan Beam wrote: > On 2016/12/21 20:58:58, amp wrote: > > Doesn't close support specifying the type of the objects in the array as well? > > Something like {!Array<TYPE>}. > > > > Are these commands just strings? > > yes, you can templatize types in closure, so Array<T>, Object<T>, Promise<T>, > etc. Cool. Yep, there's a lot more we can do - this review was basically to show a proof of concept. Dan, do you have an opinion on the removal of the "var api = (function() {" approach? This appeared to confuse the compiler, and we lost type checking across files. https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:116: this.properties = {}; On 2016/12/21 20:58:58, amp wrote: > It looks like properties could be a well defined object. Maybe use a @typedef > here to specify what it's properites are (like 'contentQuad', 'parentId', etc)? > > Or if things compile fine now I suppose that could be left to a future change. As of now, native parses a dictionary, and updates only properties that are in the dictionary. Most of the time, we're updating only one of several properties (like changing the visibility flag). We could make this cleaner by defining an enum of keys, but since this module's job is hiding details of the native API, I don't think an enum buys us much. https://codereview.chromium.org/2592143002/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:122: this.varvar = 1; On 2016/12/21 20:58:58, amp wrote: > I can't remember if this was specifically restricted in Chromium javascript, but > aren't private members supposed to end in '_'? > > Also, this varvar, and the comments seem to be copied from elsewhere... > Yeah, this is test code.
Folks, here's more of the first cut. There is definitely a LOT more than can be done, starting with me reading the full Javascript style guide. This compiles though, and is a step in the right direction. I think further improvements should be follow-on changes. Next things to address: - The @private tags on members aren't actually enforced, possibly because we're using ES6 classes/constructors - Private members need trailing underscores_ - vr_shell_ui.js should also be scrubbed.
LGTM as a starting point. We can have further changes to the UI update the style as we go. https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:82: * @param {api.Action} action It looks a little odd to have the type include the object wrapper name. I would think it would just be {Action}. Is there a mechanism for exporting and pulling in modules/classes from different javascript for Chromium? I couldn't find any examples looking through chromium code search. Something like goog.provide and goog.require (there may be updated ways that I am not as familiar with) is what I was thinking, but apparently Chromium UI code doesn't use those. https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:110: * @struct What does tagging a class as @struct do?
On 2016/12/22 21:10:43, amp wrote: > LGTM as a starting point. We can have further changes to the UI update the > style as we go. > > https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... > File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): > > https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui_api.js:82: * @param {api.Action} > action > It looks a little odd to have the type include the object wrapper name. I > would think it would just be {Action}. > > Is there a mechanism for exporting and pulling in modules/classes from different > javascript for Chromium? I couldn't find any examples looking through chromium > code search. > > Something like goog.provide and goog.require (there may be updated ways that I > am not as familiar with) is what I was thinking, but apparently Chromium UI code > doesn't use those. > > https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... > chrome/browser/resources/vr_shell/vr_shell_ui_api.js:110: * @struct > What does tagging a class as @struct do? rubber stamp lgtm
cjgrant@chromium.org changed reviewers: + dbeam@chromium.org
dbeam@chromium.org: Please review changes in the third-party closure compiler inclusion. This change is an incremental step towards improving our VR JS code health, but I'm not sure if there are additional implications of having our modules included in that list.
https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:82: * @param {api.Action} action On 2016/12/22 21:10:43, amp wrote: > It looks a little odd to have the type include the object wrapper name. I > would think it would just be {Action}. > > Is there a mechanism for exporting and pulling in modules/classes from different > javascript for Chromium? I couldn't find any examples looking through chromium > code search. > > Something like goog.provide and goog.require (there may be updated ways that I > am not as familiar with) is what I was thinking, but apparently Chromium UI code > doesn't use those. Without the api prefix, the compiler says: ## /work/code/clankium/src/chrome/browser/resources/vr_shell/vr_shell_ui_api.js:82: ERROR - Bad type annotation. Unknown type Action ## * @param {Action} action Given that the object wrapper is acting like a namespace, I personally like it there. https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:110: * @struct On 2016/12/22 21:10:43, amp wrote: > What does tagging a class as @struct do? From the JSDoc annotation guide: "@struct is used to create objects with a fixed number of properties. When a constructor (Foo in the example) is annotated with @struct, you can only use the dot notation to access the properties of Foo objects, not the bracket notation. Also, you cannot add a property to a Foo instance after it's constructed. The annotation can also be used directly on object literals." If I attempt to set a different property on such an object, the compiler complains: ## /work/code/clankium/src/chrome/browser/resources/vr_shell/vr_shell_ui.js:159: ERROR - Cannot add a property to a struct instance after it is constructed. (If you already declared the property, make sure to give it a type.) ## update.cat = 4; ## ^^^ Seems like we'd want this checking applied by default if we could.
Description was changed from ========== Experimental: First cut of JS closure compiler use for VR HTML UI. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== First cut of JS closure compiler use for VR HTML UI. Also, run clang-format to fix up styling. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by cjgrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org, amp@chromium.org Link to the patchset: https://codereview.chromium.org/2592143002/#ps120001 (title: "Run clang-format so that dbeam@ doesn't have to fix our files; remove the third-party change to exp…")
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": 120001, "attempt_start_ts": 1483554261335030,
"parent_rev": "816fd618e6e5c211aa015f68417737e5737be858", "commit_rev":
"689de0633e3cdb19768688e7afb6f1caa3f16357"}
Message was sent while issue was closed.
Description was changed from ========== First cut of JS closure compiler use for VR HTML UI. Also, run clang-format to fix up styling. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== First cut of JS closure compiler use for VR HTML UI. Also, run clang-format to fix up styling. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2592143002 Committed: https://chromium.googlesource.com/chromium/src/+/689de0633e3cdb19768688e7afb6... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/689de0633e3cdb19768688e7afb6...
Message was sent while issue was closed.
Description was changed from ========== First cut of JS closure compiler use for VR HTML UI. Also, run clang-format to fix up styling. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2592143002 Committed: https://chromium.googlesource.com/chromium/src/+/689de0633e3cdb19768688e7afb6... ========== to ========== First cut of JS closure compiler use for VR HTML UI. Also, run clang-format to fix up styling. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/0cc9f99274349ffcf91788c58dfc0b3297bcd07a Cr-Commit-Position: refs/heads/master@{#441437} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0cc9f99274349ffcf91788c58dfc0b3297bcd07a Cr-Commit-Position: refs/heads/master@{#441437}
Message was sent while issue was closed.
https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:5: var api = new Object(); var api = {}; OR cr.exportPath('api'); OR cr.define('api', function() { // ... magic happens for @enum as long as you return at the bottom // because of our fancy compiler pass ... /** @enum {number} */ var Command = {...}; return {Command: Command}; }); https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:17: 'REMOVE_ANIMATION': 4 indent off protip: tools/clang-format-js https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui_scene.js (right): https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui_scene.js:5: var ui = new Object(); don't invoke Object, just use {}
Message was sent while issue was closed.
https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right): https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:5: var api = new Object(); On 2017/01/04 20:29:02, Dan Beam wrote: > var api = {}; > > OR > > cr.exportPath('api'); > > OR > > cr.define('api', function() { > // ... magic happens for @enum as long as you return at the bottom > // because of our fancy compiler pass ... > /** @enum {number} */ > var Command = {...}; > return {Command: Command}; > }); We previously used your Option 3, but ditched it as I have having trouble getting the compiler to follow it. Option 1 looks sufficient. https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui_api.js:17: 'REMOVE_ANIMATION': 4 On 2017/01/04 20:29:01, Dan Beam wrote: > indent off > > protip: tools/clang-format-js See the next patch set - I've been watching your clang-format goodness unfolding, and applied it before submitting. :) https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/vr_shell/vr_shell_ui_scene.js (right): https://codereview.chromium.org/2592143002/diff/100001/chrome/browser/resourc... chrome/browser/resources/vr_shell/vr_shell_ui_scene.js:5: var ui = new Object(); On 2017/01/04 20:29:02, Dan Beam wrote: > don't invoke Object, just use {} Thanks; I'll fix that in a follow-on change. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
