Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1140593003:
Remove ExtraAnnotationNames override in Runner.java (Closed)
|
Created:
5 years, 7 months ago by Jeremy Klein Modified:
5 years, 7 months ago CC:
chromium-reviews, vitalyp+closure_chromium.org, dbeam+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove ExtraAnnotationNames override in Runner.java
This clobbers --extra_annotation_names in compile.py.
BUG=
Committed: https://crrev.com/88d45dfa38c49c6ac7108001ecd134f2873a843b
Cr-Commit-Position: refs/heads/master@{#329534}
Patch Set 1 #
Total comments: 8
Patch Set 2 : revert compile.py #Patch Set 3 : Rebuild runner.jar #
Messages
Total messages: 14 (4 generated)
jlklein@chromium.org changed reviewers: + dbeam@chromium.org
tbreisacher@chromium.org changed reviewers: + tbreisacher@chromium.org
https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... File third_party/closure_compiler/compile.py (right): https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... third_party/closure_compiler/compile.py:58: "suppressReceiverCheck", This seems to come from the devtools code. I really don't think you need it. https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=s... https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... File third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java (right): https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:140: private List<CompilerInstanceDescriptor> getDescriptors() { I get the sense there is a lot of other code here that you don't need. Like, what is this method doing? Do you have any lines your args file that have " +" in them? Good chance to simplify!
lgtm https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... File third_party/closure_compiler/compile.py (right): https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... third_party/closure_compiler/compile.py:51: _EXTRA_ANNOTATIONS = [ nit: _EXTRA_JSDOC_ANNOTATIONS, remove comment https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... File third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java (right): https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:140: private List<CompilerInstanceDescriptor> getDescriptors() { On 2015/05/12 21:39:51, Tyler Breisacher (Chromium) wrote: > I get the sense there is a lot of other code here that you don't need. Like, > what is this method doing? Do you have any lines your args file that have " +" > in them? > > Good chance to simplify! the args file is dynamically generated so this is possible
https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... File third_party/closure_compiler/compile.py (right): https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... third_party/closure_compiler/compile.py:51: _EXTRA_ANNOTATIONS = [ On 2015/05/12 23:09:20, Dan Beam wrote: > nit: _EXTRA_JSDOC_ANNOTATIONS, remove comment Reverting this file's changes. https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... third_party/closure_compiler/compile.py:58: "suppressReceiverCheck", On 2015/05/12 21:39:51, Tyler Breisacher (Chromium) wrote: > This seems to come from the devtools code. I really don't think you need it. > > https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=s... Done. https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... File third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java (right): https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:140: private List<CompilerInstanceDescriptor> getDescriptors() { On 2015/05/12 23:09:20, Dan Beam wrote: > On 2015/05/12 21:39:51, Tyler Breisacher (Chromium) wrote: > > I get the sense there is a lot of other code here that you don't need. Like, > > what is this method doing? Do you have any lines your args file that have " +" > > in them? > > > > Good chance to simplify! > > the args file is dynamically generated so this is possible Hmm... So is there anything else I can kill here?
https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... File third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java (right): https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:140: private List<CompilerInstanceDescriptor> getDescriptors() { On 2015/05/12 23:39:24, Jeremy Klein wrote: > On 2015/05/12 23:09:20, Dan Beam wrote: > > On 2015/05/12 21:39:51, Tyler Breisacher (Chromium) wrote: > > > I get the sense there is a lot of other code here that you don't need. Like, > > > what is this method doing? Do you have any lines your args file that have " > +" > > > in them? > > > > > > Good chance to simplify! > > > > the args file is dynamically generated so this is possible > > Hmm... So is there anything else I can kill here? what if dev tools wants to use our infra eventually? that is a mild goal (to have only 1 stack). remoting/ used to roll their own, as did Files.app I believe. less stacks == less hacks (well, maybe...)
On 2015/05/12 23:51:15, Dan Beam wrote: > https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... > File > third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java > (right): > > https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... > third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:140: > private List<CompilerInstanceDescriptor> getDescriptors() { > On 2015/05/12 23:39:24, Jeremy Klein wrote: > > On 2015/05/12 23:09:20, Dan Beam wrote: > > > On 2015/05/12 21:39:51, Tyler Breisacher (Chromium) wrote: > > > > I get the sense there is a lot of other code here that you don't need. > Like, > > > > what is this method doing? Do you have any lines your args file that have > " > > +" > > > > in them? > > > > > > > > Good chance to simplify! > > > > > > the args file is dynamically generated so this is possible > > > > Hmm... So is there anything else I can kill here? > > what if dev tools wants to use our infra eventually? that is a mild goal (to > have only 1 stack). remoting/ used to roll their own, as did Files.app I > believe. less stacks == less hacks (well, maybe...) Making decisions based on what-ifs is questionable. But, if they want to reuse this infra, they should add their annotation to the list in compile.py, where all the other non-standard annotations are.
On 2015/05/12 23:54:22, Tyler Breisacher (Chromium) wrote: > On 2015/05/12 23:51:15, Dan Beam wrote: > > > https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... > > File > > > third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java > > (right): > > > > > https://codereview.chromium.org/1140593003/diff/1/third_party/closure_compile... > > > third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:140: > > private List<CompilerInstanceDescriptor> getDescriptors() { > > On 2015/05/12 23:39:24, Jeremy Klein wrote: > > > On 2015/05/12 23:09:20, Dan Beam wrote: > > > > On 2015/05/12 21:39:51, Tyler Breisacher (Chromium) wrote: > > > > > I get the sense there is a lot of other code here that you don't need. > > Like, > > > > > what is this method doing? Do you have any lines your args file that > have > > " > > > +" > > > > > in them? > > > > > > > > > > Good chance to simplify! > > > > > > > > the args file is dynamically generated so this is possible > > > > > > Hmm... So is there anything else I can kill here? > > > > what if dev tools wants to use our infra eventually? that is a mild goal (to > > have only 1 stack). remoting/ used to roll their own, as did Files.app I > > believe. less stacks == less hacks (well, maybe...) > > Making decisions based on what-ifs is questionable. But, if they want to reuse > this infra, they should add their annotation to the list in compile.py, where > all the other non-standard annotations are. fair, we can add it then. still lgtm
The CQ bit was checked by jlklein@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1140593003/#ps40001 (title: "Rebuild runner.jar")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1140593003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/88d45dfa38c49c6ac7108001ecd134f2873a843b Cr-Commit-Position: refs/heads/master@{#329534} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
