Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1033223007:
[Extension API Extern Generation] Support inline object definitions (Closed)
|
Created:
5 years, 9 months ago by Devlin Modified:
5 years, 8 months ago Reviewers:
Tyler Breisacher (Chromium) CC:
chromium-reviews, Dan Beam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Extension API Extern Generation] Support inline object definitions
The Extension APIs can define their objects "inlined", i.e., at the place of
their use (for instance, as a function parameter). The generated externs need
to reflect this, rather than simply call the parameter an 'Object'.
BUG=469920
Committed: https://crrev.com/9ead75ee2fb3655639689925da6a68141a2556e6
Cr-Commit-Position: refs/heads/master@{#322804}
Patch Set 1 : #
Total comments: 15
Patch Set 2 : Dan's #
Total comments: 13
Patch Set 3 : Tyler's #Patch Set 4 : Presubmit fix #
Messages
Total messages: 24 (11 generated)
Patchset #1 (id:1) has been deleted
rdevlin.cronin@chromium.org changed reviewers: + dbeam@chromium.org
Dan, please take a look (Tyler, FYI). This change looks kinda crazy (because inlined objects are evil!), but it's pretty much focused on: - Allowing same-line concatenation/appending in code.py - Using Code objects more than strings in js_externs_generator.py. Sorry it's so noisy, though. https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator_test.py (right): https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator_test.py:213: * @param {{ Yay! For reference, before, this would have read: /** * @param {Object} * @param {function(Object):void} * @return Object */
dbeam@chromium.org changed reviewers: + tbreisacher@chromium.org - dbeam@chromium.org
i think tbreisacher@ is a better reviewer for this https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:21: white_space=False): nit: white_space -> preserve_whitespace, strip, rstrip, strip_right, right_strip, s/strip/trim/g} and possibly invert logic also, technically i think whitespace is 1 word but really don't care enough to fight about that one (at all) https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:28: or start a new line. same_line -> allow_wrapping, dont_wrap, no_wrap imo (and possibly invert logic) https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:29: white_space: whether or not trailing whitespace should be allowed. nit: indent off https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:32: prefix = indent_level * ' ' if indent_level else ''.join( think this could be: (indent_level * ' ').join( https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:38: if same_line and len(self._code) > 0: nit: can this just be if same_line and self._code: https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:60: if len(obj._code) is 0: nit: if not obj._code: https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator_test.py (right): https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator_test.py:230: """ % datetime.now().year if you're actually testing that the copyright is updated, shouldn't you be putting an old year in the fake_json file? e.g. 2014?
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:21: white_space=False): On 2015/03/27 19:05:25, Dan Beam wrote: > nit: white_space -> preserve_whitespace, strip, rstrip, strip_right, > right_strip, s/strip/trim/g} and possibly invert logic > > also, technically i think whitespace is 1 word but really don't care enough to > fight about that one (at all) Done. https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:28: or start a new line. On 2015/03/27 19:05:25, Dan Beam wrote: > same_line -> allow_wrapping, dont_wrap, no_wrap imo (and possibly invert logic) wrapping doesn't work as well, I think, because it implies (to me) that it's for breaking lines, versus whether to append *everything* to a new line, or the old one. How about inverting the logic and calling it "new_line"? https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:29: white_space: whether or not trailing whitespace should be allowed. On 2015/03/27 19:05:25, Dan Beam wrote: > nit: indent off Done. https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:32: prefix = indent_level * ' ' if indent_level else ''.join( On 2015/03/27 19:05:25, Dan Beam wrote: > think this could be: > > (indent_level * ' ').join( nope, different logic. |indent_level| basically forces a _total_ indent, not an additional indent. So it should be: if indent_level: use indent_level else use prefixes https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:38: if same_line and len(self._code) > 0: On 2015/03/27 19:05:25, Dan Beam wrote: > nit: can this just be > > if same_line and self._code: Done. https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/code.py:60: if len(obj._code) is 0: On 2015/03/27 19:05:25, Dan Beam wrote: > nit: > > if not obj._code: Done. https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator_test.py (right): https://codereview.chromium.org/1033223007/diff/20001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator_test.py:230: """ % datetime.now().year On 2015/03/27 19:05:25, Dan Beam wrote: > if you're actually testing that the copyright is updated, shouldn't you be > putting an old year in the fake_json file? e.g. 2014? That's probably wise. Done. https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/cc_generator.py (left): https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/cc_generator.py:510: .Eblock('}')) This Eblock (and another in .h generator) didn't have a corresponding Sblock. When blocks just adjusted indent_level, this wasn't terribly noticeable because a) there wouldn't be a runtime error and b) -2 * ' ' = '' in python (so formatting doesn't freak out). That said, it's still a bug, because it means the next Sblock will go to zero indentation (instead of 2).
Just a general comment: For formatting issues, it might be easier to generate badly-formatted code and then pipe the output through clang-format (which, while designed for C++, actually works pretty well on JS code, believe it or not) to get the nicer formatting. I don't think it formats comments though so for JSDoc it probably won't work too well. https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/code.py:28: to the last line of the code. I'm wary of the number of parameters we're adding here. c.Append('foo', new_line=True) is just shorthand for c.Append().Append('foo'); right? I'm inclined to think the latter form is better since it keeps the API simpler (and is actually shorter) https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/code.py:86: def Sblock(self, line=None, line_prefix=None, new_line=True): Ugh. Let's not do it in this CL but let's rename these to StartBlock and EndBlock at some point.
https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/code.py:28: to the last line of the code. On 2015/03/27 22:52:18, Tyler Breisacher (Chromium) wrote: > I'm wary of the number of parameters we're adding here. > > c.Append('foo', new_line=True) > > is just shorthand for > > c.Append().Append('foo'); > > right? I'm inclined to think the latter form is better since it keeps the API > simpler (and is actually shorter) Well, no - new_line=True is the default. And if it wasn't, c.Append() would be a no-op (it would append an empty string onto the last line). Also, I think it is more common to want c.Append(line1) c.Append(line2) c.Append(line3) than c.Append(line1-pt1) c.Append(line1-pt2) c.Append(line1-pt3) but the latter is very useful for the JS docs, when we have recursive generation of, e.g., object types (and generating them as strings doesn't really make as much sense). Of course, we could just separate these into two methods, Append() and Continue() or some such thing. https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/code.py:86: def Sblock(self, line=None, line_prefix=None, new_line=True): On 2015/03/27 22:52:18, Tyler Breisacher (Chromium) wrote: > Ugh. Let's not do it in this CL but let's rename these to StartBlock and > EndBlock at some point. SGTM. :)
lgtm https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/code.py:28: to the last line of the code. On 2015/03/27 23:04:09, Devlin wrote: > On 2015/03/27 22:52:18, Tyler Breisacher (Chromium) wrote: > > I'm wary of the number of parameters we're adding here. > > > > c.Append('foo', new_line=True) > > > > is just shorthand for > > > > c.Append().Append('foo'); > > > > right? I'm inclined to think the latter form is better since it keeps the API > > simpler (and is actually shorter) > > Well, no - new_line=True is the default. And if it wasn't, c.Append() would be > a no-op (it would append an empty string onto the last line). > > Also, I think it is more common to want > c.Append(line1) > c.Append(line2) > c.Append(line3) > than > c.Append(line1-pt1) > c.Append(line1-pt2) > c.Append(line1-pt3) > but the latter is very useful for the JS docs, when we have recursive generation > of, e.g., object types (and generating them as strings doesn't really make as > much sense). > > Of course, we could just separate these into two methods, Append() and > Continue() or some such thing. Oh right. Okay well, I guess this is good then. I just get worried when I see lots of parameters being added to a method. https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/code.py:96: line_prefix if line_prefix else ' ' * self._indent_size) I'm certainly not a Pythonista but I think you can write this as line_prefix or ' ' * self._indent_size https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:196: for i in range(len(function.params)): for i, param in enumerate(function.params): and then s/function.params[i]/param/ on the next line https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator_test.py (right): https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator_test.py:248: self.maxDiff = None # Lets us see the full diff when inequal. If you're going to do this for every test (and I think you probably should) then just put it in a setUp method.
https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/code.py (right): https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/code.py:28: to the last line of the code. > Oh right. Okay well, I guess this is good then. I just get worried when I see > lots of parameters being added to a method. I appreciate the skepticism. :) I tried to figure out the simplest way, and, sadly, adding the extra parameters was actually preferable to the alternatives. https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/code.py:96: line_prefix if line_prefix else ' ' * self._indent_size) On 2015/03/27 23:22:32, Tyler Breisacher (Chromium) wrote: > I'm certainly not a Pythonista but I think you can write this as > > line_prefix or ' ' * self._indent_size Ah, right. Keep forgetting that. :) Done. https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator.py (right): https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator.py:196: for i in range(len(function.params)): On 2015/03/27 23:22:32, Tyler Breisacher (Chromium) wrote: > for i, param in enumerate(function.params): > > and then s/function.params[i]/param/ on the next line Oh, nifty. Done (also on line 230) https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... File tools/json_schema_compiler/js_externs_generator_test.py (right): https://codereview.chromium.org/1033223007/diff/60001/tools/json_schema_compi... tools/json_schema_compiler/js_externs_generator_test.py:248: self.maxDiff = None # Lets us see the full diff when inequal. On 2015/03/27 23:22:32, Tyler Breisacher (Chromium) wrote: > If you're going to do this for every test (and I think you probably should) then > just put it in a setUp method. Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbreisacher@chromium.org Link to the patchset: https://codereview.chromium.org/1033223007/#ps80001 (title: "Tyler's")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1033223007/80001
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 rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbreisacher@chromium.org Link to the patchset: https://codereview.chromium.org/1033223007/#ps100001 (title: "Presubmit fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1033223007/100001
The CQ bit was unchecked by rdevlin.cronin@chromium.org
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1033223007/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9ead75ee2fb3655639689925da6a68141a2556e6 Cr-Commit-Position: refs/heads/master@{#322804} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
