Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1488773003:
Add js_interface_generator for generating extensions interfaces (Closed)
DescriptionAdd js_interface_generator for generating extensions interfaces
BUG=563744
Committed: https://crrev.com/f0e8cce5133fa2fe35eff3e107acd455c86f95fe
Cr-Commit-Position: refs/heads/master@{#364423}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Move common functions to js_util.py #Patch Set 3 : similarity=25 #
Total comments: 7
Patch Set 4 : Move COPYRIGHT / INFO -> js_util.py #Patch Set 5 : . #
Total comments: 22
Patch Set 6 : Feedback #
Total comments: 6
Patch Set 7 : Embed entire expected output text except year and exe name #Patch Set 8 : Rebase + shorten line #
Total comments: 4
Dependent Patchsets: Messages
Total messages: 43 (15 generated)
stevenjb@chromium.org changed reviewers: + michaelpg@chromium.org, rdevlin.cronin@chromium.org
https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... File tools/json_schema_compiler/js_interface_generator.py (right): https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... tools/json_schema_compiler/js_interface_generator.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. Do we really need a new generator, rather than just passing in a flag to the extern generator? What needs to change between the interface and the extern, really? https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... File tools/json_schema_compiler/js_interface_generator_test.py (right): https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... tools/json_schema_compiler/js_interface_generator_test.py:82: * Does something exciting! And what's more, this is a multiline function Will these closure docs conflict with the externs?
https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... File tools/json_schema_compiler/js_interface_generator.py (right): https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... tools/json_schema_compiler/js_interface_generator.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2015/12/02 17:58:28, Devlin wrote: > Do we really need a new generator, rather than just passing in a flag to the > extern generator? What needs to change between the interface and the extern, > really? See below :) I started on that path, but there are at least as many differences as similarities. This seemed simpler. There are some shared functions that we could put in a common file. My python fu is not great, but I will look int that. https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... File tools/json_schema_compiler/js_interface_generator_test.py (right): https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... tools/json_schema_compiler/js_interface_generator_test.py:82: * Does something exciting! And what's more, this is a multiline function On 2015/12/02 17:58:28, Devlin wrote: > Will these closure docs conflict with the externs? Any @param/@return needs to match, that's pretty much the point. I don't think the rest matters?
There is definitely more than enough copied code to warrant extracting out a helper class. Also, can you re-upload with a lower similarity so we can see the actual differences? https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... File tools/json_schema_compiler/js_interface_generator.py (right): https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... tools/json_schema_compiler/js_interface_generator.py:37: GetHeader = staticmethod(GetHeader) i'm pretty sure python 2.3 came out before i was born, do we really have to support it? https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... tools/json_schema_compiler/js_interface_generator.py:51: (sys.argv[0]), self._namespace.name)).Append() 4-space indent
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Separated out the common code and decreased the similarity for better diffs. PTAL https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... File tools/json_schema_compiler/js_interface_generator.py (right): https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... tools/json_schema_compiler/js_interface_generator.py:37: GetHeader = staticmethod(GetHeader) On 2015/12/02 19:39:45, michaelpg wrote: > i'm pretty sure python 2.3 came out before i was born, do we really have to > support it? apparantly. https://codereview.chromium.org/1488773003/diff/1/tools/json_schema_compiler/... tools/json_schema_compiler/js_interface_generator.py:51: (sys.argv[0]), self._namespace.name)).Append() On 2015/12/02 19:39:45, michaelpg wrote: > 4-space indent Done.
https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... File tools/json_schema_compiler/js_interface_generator.py (right): https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:19: LICENSE = ("""// Copyright %s The Chromium Authors. All rights reserved. Might almost be worth defining this as a constant in the util file? https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:65: What about enums? https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:92: .Append('%s: assertNotReached,' % (function.name))) Do we want asssertNotReached over just function() {}?
https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... File tools/json_schema_compiler/js_interface_generator.py (right): https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:19: LICENSE = ("""// Copyright %s The Chromium Authors. All rights reserved. On 2015/12/04 17:44:23, Devlin wrote: > Might almost be worth defining this as a constant in the util file? Done. https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:65: On 2015/12/04 17:44:23, Devlin wrote: > What about enums? We don't need them in the interface, they are already defined in the externs file, same for types. https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:92: .Append('%s: assertNotReached,' % (function.name))) On 2015/12/04 17:44:23, Devlin wrote: > Do we want asssertNotReached over just function() {}? asssertNotReached is what michael used in the existing (hand created) settings_private_interface.js: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... It seems like a good idea to me; a Fake should implement any function it expects to have called, even if many of the implementations are trivial.
https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... File tools/json_schema_compiler/js_interface_generator.py (right): https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:65: On 2015/12/04 18:24:07, stevenjb wrote: > On 2015/12/04 17:44:23, Devlin wrote: > > What about enums? > > We don't need them in the interface, they are already defined in the externs > file, same for types. Well, the enums are exposed as JS objects on the API, so code could say something like chrome.myApi.MyEnum.FOO. AFAIK, the externs define them, but externs are only used for closure compilation, not for any code actually running. So if your page relies on an enum value (I know some of the extension pages do), won't it break?
On 2015/12/04 18:28:59, Devlin wrote: > https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... > File tools/json_schema_compiler/js_interface_generator.py (right): > > https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... > tools/json_schema_compiler/js_interface_generator.py:65: > On 2015/12/04 18:24:07, stevenjb wrote: > > On 2015/12/04 17:44:23, Devlin wrote: > > > What about enums? > > > > We don't need them in the interface, they are already defined in the externs > > file, same for types. > > Well, the enums are exposed as JS objects on the API, so code could say > something like chrome.myApi.MyEnum.FOO. AFAIK, the externs define them, but > externs are only used for closure compilation, not for any code actually > running. So if your page relies on an enum value (I know some of the extension > pages do), won't it break? I don't think I am following. This is also entirely for closure compilation. It is intended to be complementary to (and dependent on) the externs file. i.e. this is just an interface for Closure, not a fake implementation. e.g. This is a fake implementation: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w... This is some code that uses the interface for @type allowing chrome.settingsPrivate to be replaced with FakeSettingsPrivate: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
On 2015/12/04 18:48:25, stevenjb wrote: > On 2015/12/04 18:28:59, Devlin wrote: > > > https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... > > File tools/json_schema_compiler/js_interface_generator.py (right): > > > > > https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... > > tools/json_schema_compiler/js_interface_generator.py:65: > > On 2015/12/04 18:24:07, stevenjb wrote: > > > On 2015/12/04 17:44:23, Devlin wrote: > > > > What about enums? > > > > > > We don't need them in the interface, they are already defined in the externs > > > file, same for types. > > > > Well, the enums are exposed as JS objects on the API, so code could say > > something like chrome.myApi.MyEnum.FOO. AFAIK, the externs define them, but > > externs are only used for closure compilation, not for any code actually > > running. So if your page relies on an enum value (I know some of the > extension > > pages do), won't it break? > > I don't think I am following. This is also entirely for closure compilation. It > is intended to be complementary to (and dependent on) the externs file. i.e. > this is just an interface for Closure, not a fake implementation. > > e.g. > > This is a fake implementation: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w... > > This is some code that uses the interface for @type allowing > chrome.settingsPrivate to be replaced with FakeSettingsPrivate: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... Oh, I totally missed that part. I thought this was for generating a default stub implementation that could be easily wired in so that files didn't need to depend on a (real) chrome. Surprising how similar the two are.
Right. Well, it's related to that, and a good copy/paste starting point, but it turns out in order to make a Fake work with Closure we need an interface first. On Fri, Dec 4, 2015 at 12:35 PM, <rdevlin.cronin@chromium.org> wrote: > On 2015/12/04 18:48:25, stevenjb wrote: > >> On 2015/12/04 18:28:59, Devlin wrote: >> > >> > > > https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... > >> > File tools/json_schema_compiler/js_interface_generator.py (right): >> > >> > >> > > > https://codereview.chromium.org/1488773003/diff/100001/tools/json_schema_comp... > >> > tools/json_schema_compiler/js_interface_generator.py:65: >> > On 2015/12/04 18:24:07, stevenjb wrote: >> > > On 2015/12/04 17:44:23, Devlin wrote: >> > > > What about enums? >> > > >> > > We don't need them in the interface, they are already defined in the >> > externs > >> > > file, same for types. >> > >> > Well, the enums are exposed as JS objects on the API, so code could say >> > something like chrome.myApi.MyEnum.FOO. AFAIK, the externs define >> them, but >> > externs are only used for closure compilation, not for any code actually >> > running. So if your page relies on an enum value (I know some of the >> extension >> > pages do), won't it break? >> > > I don't think I am following. This is also entirely for closure >> compilation. >> > It > >> is intended to be complementary to (and dependent on) the externs file. >> i.e. >> this is just an interface for Closure, not a fake implementation. >> > > e.g. >> > > This is a fake implementation: >> > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w... > > This is some code that uses the interface for @type allowing >> chrome.settingsPrivate to be replaced with FakeSettingsPrivate: >> > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > Oh, I totally missed that part. I thought this was for generating a > default > stub implementation that could be easily wired in so that files didn't > need to > depend on a (real) chrome. Surprising how similar the two are. > > https://codereview.chromium.org/1488773003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/code.py:34: self._line_prefixes) 4-space indent https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/code.py:177: """Trims any trailing newlines nit: period https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/code.py:179: while len(self._code) > 0: opt nit: "while self._code:" (cf lines 41, 50, 63, 137...) https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/compiler.py:150: output_code += [filename, '', code, ''] is this where the silly filename at the beginning of extern output comes from? can we just... not? https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator.py:86: .Append(self._js_util.GenerateSeeLink(self._namespace.name,'type', space https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator.py:134: c.Append(self._js_util.GenerateSeeLink(self._namespace.name,'type', space https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/js_interface_generator.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:6: Note: This is a work in progress, and generated externs may require tweaking. externs -> interfaces https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:29: namespace)) can you add the assertNotReached like here? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... otherwise if someone tries to execute this file for some reason, they'll get an unhelpful failure if it has any events since ChromeEvent is just an extern
PTAL https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/code.py:34: self._line_prefixes) On 2015/12/04 21:02:19, michaelpg wrote: > 4-space indent Done. https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/code.py:177: """Trims any trailing newlines On 2015/12/04 21:02:19, michaelpg wrote: > nit: period Done. https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/code.py:179: while len(self._code) > 0: On 2015/12/04 21:02:19, michaelpg wrote: > opt nit: "while self._code:" (cf lines 41, 50, 63, 137...) Done. https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/compiler.py:150: output_code += [filename, '', code, ''] On 2015/12/04 21:02:19, michaelpg wrote: > is this where the silly filename at the beginning of extern output comes from? > can we just... not? It is. And this is correct: If we are writing a single file to std:out, we want to skip the "sill" filename at the beginning. However if we generate, e.g., a .cc and a .h to stdout then the filename separators are helpful. Added a comment. https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator.py:86: .Append(self._js_util.GenerateSeeLink(self._namespace.name,'type', On 2015/12/04 21:02:19, michaelpg wrote: > space "space" ?? https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator.py:134: c.Append(self._js_util.GenerateSeeLink(self._namespace.name,'type', On 2015/12/04 21:02:19, michaelpg wrote: > space ? https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/js_interface_generator.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:6: Note: This is a work in progress, and generated externs may require tweaking. On 2015/12/04 21:02:19, michaelpg wrote: > externs -> interfaces Done. https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:29: namespace)) On 2015/12/04 21:02:19, michaelpg wrote: > can you add the assertNotReached like here? > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > otherwise if someone tries to execute this file for some reason, they'll get an > unhelpful failure if it has any events since ChromeEvent is just an extern Based on Devlin's confusion, yeah, I'll do that. It seemed a bit awkward to me, but I guess it makes sense.
Ok, I think this LGTM assuming the generated interfaces are working. Caveat: my python-fu is weak (but improving) https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/compiler.py:150: output_code += [filename, '', code, ''] On 2015/12/07 18:31:24, stevenjb wrote: > On 2015/12/04 21:02:19, michaelpg wrote: > > is this where the silly filename at the beginning of extern output comes from? > > can we just... not? > > It is. And this is correct: If we are writing a single file to std:out, we want > to skip the "sill" filename at the beginning. ah, I didn't realize that's what |generators| represented. Yay, thanks! > However if we generate, e.g., a > .cc and a .h to stdout then the filename separators are helpful. Does any *nix utility do this? it's wildly unexpected behavior and goes against the "philosophy" of text-transforming tools IMO. Not a reason to change it now, though. > Added a comment. https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator.py:86: .Append(self._js_util.GenerateSeeLink(self._namespace.name,'type', On 2015/12/07 18:31:24, stevenjb wrote: > On 2015/12/04 21:02:19, michaelpg wrote: > > space > > "space" ?? sorry, i guess a certain someone is rubbing off on me... insert a space after the first comma, before 'type' https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator.py:134: c.Append(self._js_util.GenerateSeeLink(self._namespace.name,'type', On 2015/12/07 18:31:24, stevenjb wrote: > On 2015/12/04 21:02:19, michaelpg wrote: > > space > > ? ditto
lgtm https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator.py:38: GetHeader = staticmethod(GetHeader) yuck. See other comment. https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... File tools/json_schema_compiler/js_externs_generator_test.py (right): https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator_test.py:75: expected_output = JsExternsGenerator.GetHeader(sys.argv[0], 'fakeApi') + """ I slightly prefer having this hard-coded, even if it's simplistic. Just in the spirit of testing literal expected output, vs relying on internal functions. Also, if this allows us to make GetHeader non-static (this is the only other use of it I saw?), that's an added win. https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... File tools/json_schema_compiler/js_interface_generator.py (right): https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:90: .Append('%s: assertNotReached,' % (function.name))) nit: be consistent in indentation (here and l50 differ). I would say l50 is probably right-er.
PTAL https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/compiler.py:150: output_code += [filename, '', code, ''] On 2015/12/07 18:46:53, michaelpg wrote: > On 2015/12/07 18:31:24, stevenjb wrote: > > On 2015/12/04 21:02:19, michaelpg wrote: > > > is this where the silly filename at the beginning of extern output comes > from? > > > can we just... not? > > > > It is. And this is correct: If we are writing a single file to std:out, we > want > > to skip the "sill" filename at the beginning. > > ah, I didn't realize that's what |generators| represented. Yay, thanks! > > > However if we generate, e.g., a > > .cc and a .h to stdout then the filename separators are helpful. > > Does any *nix utility do this? it's wildly unexpected behavior and goes against > the "philosophy" of text-transforming tools IMO. Not a reason to change it now, > though. > > > Added a comment. > So, I *think* that the intention is to specify an output directory and the .cc/.h files will be placed there (see if destdir: above). I seem to recall problems with specifying an output directory for the JS externs (or maybe decided it was too much hassle?). Either way, this is at least an improvement to the way we've been generating externs (and now interfaces) without affecting existing behavior for other generators. https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator.py:86: .Append(self._js_util.GenerateSeeLink(self._namespace.name,'type', On 2015/12/07 18:46:53, michaelpg wrote: > On 2015/12/07 18:31:24, stevenjb wrote: > > On 2015/12/04 21:02:19, michaelpg wrote: > > > space > > > > "space" ?? > > sorry, i guess a certain someone is rubbing off on me... > > insert a space after the first comma, before 'type' Ah. Yes indeed :) Done. https://codereview.chromium.org/1488773003/diff/140001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator.py:134: c.Append(self._js_util.GenerateSeeLink(self._namespace.name,'type', On 2015/12/07 18:46:53, michaelpg wrote: > On 2015/12/07 18:31:24, stevenjb wrote: > > On 2015/12/04 21:02:19, michaelpg wrote: > > > space > > > > ? > > ditto Done. https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator.py:38: GetHeader = staticmethod(GetHeader) On 2015/12/07 18:55:27, Devlin wrote: > yuck. See other comment. Acknowledged. https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... File tools/json_schema_compiler/js_externs_generator_test.py (right): https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator_test.py:75: expected_output = JsExternsGenerator.GetHeader(sys.argv[0], 'fakeApi') + """ On 2015/12/07 18:55:27, Devlin wrote: > I slightly prefer having this hard-coded, even if it's simplistic. Just in the > spirit of testing literal expected output, vs relying on internal functions. > > Also, if this allows us to make GetHeader non-static (this is the only other use > of it I saw?), that's an added win. Sigh. The previous way isn't any better IMHO because we a) define INFO above and b) pass multiple arguments to it. I will go ahead and explicitly embed the entire expected header (passing just year and tool name), although I feel it falls pretty firmly into the category of testing something that doesn't really need testing, i.e. adding maintenance cost (already kinda high) without much benefit. https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... File tools/json_schema_compiler/js_interface_generator.py (right): https://codereview.chromium.org/1488773003/diff/160001/tools/json_schema_comp... tools/json_schema_compiler/js_interface_generator.py:90: .Append('%s: assertNotReached,' % (function.name))) On 2015/12/07 18:55:27, Devlin wrote: > nit: be consistent in indentation (here and l50 differ). I would say l50 is > probably right-er. Line 50 might be "right-er" (but I think maybe not with the () wrapper?), but this is more consistent with other usage in this and js_externs_generator. Changed this.
slgtm
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1488773003/#ps180001 (title: "Embed entire expected output text except year and exe name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1488773003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1488773003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1488773003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1488773003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1488773003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1488773003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1488773003/#ps200001 (title: "Rebase + shorten line")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1488773003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1488773003/200001
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add js_interface_generator for generating extensions interfaces BUG=563744 ========== to ========== Add js_interface_generator for generating extensions interfaces BUG=563744 Committed: https://crrev.com/f0e8cce5133fa2fe35eff3e107acd455c86f95fe Cr-Commit-Position: refs/heads/master@{#364423} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f0e8cce5133fa2fe35eff3e107acd455c86f95fe Cr-Commit-Position: refs/heads/master@{#364423}
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
sweet! https://codereview.chromium.org/1488773003/diff/200001/tools/json_schema_comp... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/1488773003/diff/200001/tools/json_schema_comp... tools/json_schema_compiler/code.py:177: """Trims any trailing newlines. i'm confused by this code, but have you seen rstrip()? https://codereview.chromium.org/1488773003/diff/200001/tools/json_schema_comp... File tools/json_schema_compiler/js_externs_generator_test.py (right): https://codereview.chromium.org/1488773003/diff/200001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator_test.py:282: (datetime.now().year, sys.argv[0])) not sure i understand this indent
Message was sent while issue was closed.
https://codereview.chromium.org/1488773003/diff/200001/tools/json_schema_comp... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/1488773003/diff/200001/tools/json_schema_comp... tools/json_schema_compiler/code.py:177: """Trims any trailing newlines. On 2015/12/10 22:17:33, Dan Beam wrote: > i'm confused by this code, but have you seen rstrip()? self._code is an array of Line objects. This removes empty Line objects at the end. The comment could be better. https://codereview.chromium.org/1488773003/diff/200001/tools/json_schema_comp... File tools/json_schema_compiler/js_externs_generator_test.py (right): https://codereview.chromium.org/1488773003/diff/200001/tools/json_schema_comp... tools/json_schema_compiler/js_externs_generator_test.py:282: (datetime.now().year, sys.argv[0])) On 2015/12/10 22:17:33, Dan Beam wrote: > not sure i understand this indent Hmm, me neither. I blame my editor. If (when) we need to update this I will fix it, or in a separate followup. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
