Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2576053002:
Give automation extension api owners ownership of externs (Closed)
|
Created:
4 years ago by David Tseng Modified:
4 years ago CC:
chromium-reviews, vitalyp+closure_chromium.org, jlklein+watch-closure_chromium.org, dbeam+watch-closure_chromium.org, aboxhall, dmazzoni Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGive automation extension api owners ownership of externs
BUG=
Committed: https://crrev.com/484e132178179990a45f6c74c218d12bee61f094
Cr-Commit-Position: refs/heads/master@{#438680}
Patch Set 1 #Patch Set 2 : Give automation extension api owners ownership of externs #Messages
Total messages: 19 (9 generated)
Description was changed from ========== Give automation extension api owners ownership of externs BUG= ========== to ========== Give automation extension api owners ownership of externs BUG= ==========
dtseng@chromium.org changed reviewers: + redevlin.cronin@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
you misspelled Devlin's email, but lgtm from me
The CQ bit was checked by dtseng@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": 1481758547162360,
"parent_rev": "85665ab590c0f4ef1f6abc7b0c358a04f310e31d", "commit_rev":
"130125f81433eb86f8066e0f05c3510aa3287d5a"}
Message was sent while issue was closed.
Description was changed from ========== Give automation extension api owners ownership of externs BUG= ========== to ========== Give automation extension api owners ownership of externs BUG= Review-Url: https://codereview.chromium.org/2576053002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Give automation extension api owners ownership of externs BUG= Review-Url: https://codereview.chromium.org/2576053002 ========== to ========== Give automation extension api owners ownership of externs BUG= Committed: https://crrev.com/484e132178179990a45f6c74c218d12bee61f094 Cr-Commit-Position: refs/heads/master@{#438680} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/484e132178179990a45f6c74c218d12bee61f094 Cr-Commit-Position: refs/heads/master@{#438680}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2579603002/ by dbeam@chromium.org. The reason for reverting is: Manually editing generated files is not the right way to change things in this folder. https://codereview.chromium.org/2563013003/ https://codereview.chromium.org/2544203004/ Please either fix the tools, report bugs, or remove generation steps (or move these externs to somewhere else), then resubmit this CL..
Message was sent while issue was closed.
not lgtm in the meantime
Message was sent while issue was closed.
On 2016/12/15 00:36:54, Dan Beam wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/2579603002/ by mailto:dbeam@chromium.org. > > The reason for reverting is: Manually editing generated files is not the right > way to change things in this folder. > > https://codereview.chromium.org/2563013003/ > https://codereview.chromium.org/2544203004/ > > Please either fix the tools, report bugs, or remove generation steps (or move > these externs to somewhere else), then resubmit this CL.. Ok. Would be great to figure out what you guys want to do with these... These externs really don't even belong in third party but are proper extension api resources (e.g. that license warning you're probably ignoring when uploading). Anyway, pointers to a Docs, bug, etc with plans would be useful to unblock future work/cls. Thanks.
Message was sent while issue was closed.
Description was changed from ========== Give automation extension api owners ownership of externs BUG= Committed: https://crrev.com/484e132178179990a45f6c74c218d12bee61f094 Cr-Commit-Position: refs/heads/master@{#438680} ========== to ========== Give automation extension api owners ownership of externs BUG= Committed: https://crrev.com/484e132178179990a45f6c74c218d12bee61f094 Cr-Commit-Position: refs/heads/master@{#438680} ==========
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - redevlin.cronin@chromium.org
Message was sent while issue was closed.
fixing the reviewers list so that Devlin gets this On 2016/12/15 03:02:14, David Tseng wrote: > On 2016/12/15 00:36:54, Dan Beam wrote: > > A revert of this CL (patchset #2 id:20001) has been created in > > https://codereview.chromium.org/2579603002/ by mailto:dbeam@chromium.org. > > > > The reason for reverting is: Manually editing generated files is not the right > > way to change things in this folder. > > > > https://codereview.chromium.org/2563013003/ > > https://codereview.chromium.org/2544203004/ > > > > Please either fix the tools, report bugs, or remove generation steps (or move > > these externs to somewhere else), then resubmit this CL.. > > Ok. > > Would be great to figure out what you guys want to do with these... > > These externs really don't even belong in third party but are proper extension > api resources (e.g. that license warning you're probably ignoring when > uploading). Anyway, pointers to a Docs, bug, etc with plans would be useful to > unblock future work/cls. Thanks. Closure compiler is a third-party tool, as far as Chromium is concerned. We use this tool to typecheck our code. Externs don't make sense outside of the context of closure compilation, as far as I know (do you know anywhere else these are used except for closure?). This is why closure compiler externs are considered "third-party". We used to keep a big, hand-edited list of externs for lots of stuff. But it didn't make a ton of sense to us. Just one more thing to update, and in an error-prone, time-consuming way. So tbreisacher@ and rdevlin.cronin@ wrote a generator that took .json or .idl files and spit out sweet .js extern files. And generally it works really well and improved on things. We haven't yet set up a presubmit requiring re-generating an .idl's equivalent .js externs file, but that'd be helpful. I don't think it's reasonable to expect folks to (know to) update a .js file on the other side of the code base. BUT what I do expect is files that say "this is generated" can be re-generated at any time. Many others believe this is reasonable as well, and folks have lost manual edits in the past because of this (see: https://codereview.chromium.org/1034873002). I've been meaning to write a script that regenerates externs automatically (i.e. takes all the IDLs we care about and re-runs tools/json_schema_compiler/compiler.py and plops the results into third_party/closure_compiler/externs/). I just haven't done it yet. I'm sorry that there's not more docs on this process. This is pretty much all the formal "doc" on it (other than code and bugs): https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat... https://docs.google.com/document/d/1Ee9ggmp6U-lM-w9WmxN5cSLkK9B5YAq14939Woo-J... I'm redoing a lot of the doc for the webui platform and this could be included in the process. Like I mentioned, there are a few alternatives: 1) improve the generator 2) remove the instructions to auto-generate and continue to manually curate 3) move these externs out of this file
Message was sent while issue was closed.
On 2016/12/15 04:19:51, Dan Beam wrote: > fixing the reviewers list so that Devlin gets this > > On 2016/12/15 03:02:14, David Tseng wrote: > > On 2016/12/15 00:36:54, Dan Beam wrote: > > > A revert of this CL (patchset #2 id:20001) has been created in > > > https://codereview.chromium.org/2579603002/ by mailto:dbeam@chromium.org. > > > > > > The reason for reverting is: Manually editing generated files is not the > right > > > way to change things in this folder. > > > > > > https://codereview.chromium.org/2563013003/ > > > https://codereview.chromium.org/2544203004/ > > > > > > Please either fix the tools, report bugs, or remove generation steps (or > move > > > these externs to somewhere else), then resubmit this CL.. > > > > Ok. > > > > Would be great to figure out what you guys want to do with these... > > > > These externs really don't even belong in third party but are proper extension > > api resources (e.g. that license warning you're probably ignoring when > > uploading). Anyway, pointers to a Docs, bug, etc with plans would be useful to > > unblock future work/cls. Thanks. > > Closure compiler is a third-party tool, as far as Chromium is concerned. We use > this tool to typecheck our code. Externs don't make sense outside of the > context of closure compilation, as far as I know (do you know anywhere else > these are used except for closure?). This is why closure compiler externs are > considered "third-party". > > We used to keep a big, hand-edited list of externs for lots of stuff. But it > didn't make a ton of sense to us. Just one more thing to update, and in an > error-prone, time-consuming way. > > So tbreisacher@ and rdevlin.cronin@ wrote a generator that took .json or .idl > files and spit out sweet .js extern files. And generally it works really well > and improved on things. We haven't yet set up a presubmit requiring > re-generating an .idl's equivalent .js externs file, but that'd be helpful. I > don't think it's reasonable to expect folks to (know to) update a .js file on > the other side of the code base. > > BUT what I do expect is files that say "this is generated" can be re-generated > at any time. Many others believe this is reasonable as well, and folks have > lost manual edits in the past because of this (see: > https://codereview.chromium.org/1034873002). > > I've been meaning to write a script that regenerates externs automatically (i.e. > takes all the IDLs we care about and re-runs > tools/json_schema_compiler/compiler.py and plops the results into > third_party/closure_compiler/externs/). I just haven't done it yet. > > I'm sorry that there's not more docs on this process. This is pretty much all > the formal "doc" on it (other than code and bugs): > https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilat... > https://docs.google.com/document/d/1Ee9ggmp6U-lM-w9WmxN5cSLkK9B5YAq14939Woo-J... > > I'm redoing a lot of the doc for the webui platform and this could be included > in the process. > > Like I mentioned, there are a few alternatives: > > 1) improve the generator > 2) remove the instructions to auto-generate and continue to manually curate > 3) move these externs out of this file Fun fact I learned today: a presubmit does exist for this, it's just incomplete: https://cs.chromium.org/chromium/src/extensions/common/api/PRESUBMIT.py?q=api... Additional fun fact: Devlin is interested in reducing the checked in, generated code, see: https://codereview.chromium.org/2583573002/ |
