[Extern Generation] Add a presubmit script to check externs not being updated

[Extern Generation] Add a presubmit script to check externs not being updated

Add a presubmit script that checks for when extension api files are touched, but
the corresponding extern file is not. Right now, this is very simple - it only
checks that the extern is modified in some way (doesn't validate that it's the
*right* way), and is only a warning (because sometimes api file changes don't
cause extern changes). As an improvement, we would validate that the extern
files contain the proper content - let's do that later.

This will also be rolled out piecemeal, since many APIs don't currently have a
dedicated extern file. This change only imposes the check on bluetooth (because
it was handy) - if all goes well, we'll roll this out to all api files.

Also establish a dedicated extern folder, since it's silly for chrome-generated
externs to live in third_party/.

BUG=469920
Committed: https://crrev.com/9ab806c3e9213b95a7a4ab1342c9b4d5ff49830f
Cr-Commit-Position: refs/heads/master@{#378018}

[Devlin, 2016-02-22 23:26:14]

[email protected] changed reviewers:
+ [email protected]

[Devlin, 2016-02-22 23:26:15]

v1, starting small.

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/PRESU...
File extensions/common/api/PRESUBMIT.py (right):

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/PRESU...
extensions/common/api/PRESUBMIT.py:14: 'extensions/common/api/bluetooth.idl':
'ui/externs/bluetooth.js',
Just adding one for now.
Step 2: add all the ones that already have dedicated extern files.
Step 3-n: add the rest.

https://codereview.chromium.org/1718243003/diff/1/ui/externs/bluetooth.js
File ui/externs/bluetooth.js (right):

https://codereview.chromium.org/1718243003/diff/1/ui/externs/bluetooth.js#new...
ui/externs/bluetooth.js:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
I think you mentioned that ui/externs wouldn't be your first choice - where
would you prefer this go?

[Dan Beam, 2016-02-23 03:15:24]

we mainly compile UI written with JS right now.  but I don't think externs are
specific to UI.  they're generic enough to wherever JS runs.

I don't know a better place for externs than they currently are.  i think i
originally proposed src/externs but somebody (darin@?) didn't like that.

the rub is that the file manager lives outside of chrome, so it's in
ui/file_manager

maybe ui/webui, but remoting/ also uses externs (for now)

i think chrome/extensions/externs for extension APIs might make sense?

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/PRESU...
File extensions/common/api/PRESUBMIT.py (right):

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/PRESU...
extensions/common/api/PRESUBMIT.py:14: 'extensions/common/api/bluetooth.idl':
'ui/externs/bluetooth.js',
nit: use os.path.join() maybe?

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
File extensions/common/api/externs_checker.py (right):

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker.py:5: class _WarningEntry(object):
nit: is this better than a dict or a tuple?  can this class be private inside of
ExternsChecker?

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker.py:9: 
\n\n between file-level globals

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker.py:11: COMMAND_STUB = str('src/ $ python
tools/json_schema_compiler/compiler.py %s '
nit: prefix with _ (also, STUB seems a bit odd... _COMMAND_TEMPLATE, just
_COMMAND?)

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker.py:23: if (len(entries) == 1):
if len(entries) == 1:

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker.py:37: if len(bad_files) > 0:
nit: if bad_files:

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
File extensions/common/api/externs_checker_test.py (right):

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker_test.py:10: class MockFile(object):
use src/PRESUBMIT_test_mocks.py

[Dan Beam, 2016-02-23 03:16:24]

On 2016/02/23 03:15:24, Dan Beam wrote:
> we mainly compile UI written with JS right now.  but I don't think externs are
> specific to UI.  they're generic enough to wherever JS runs.
> 
> I don't know a better place for externs than they currently are.  i think i
> originally proposed src/externs but somebody (darin@?) didn't like that.
> 
> the rub is that the file manager lives outside of chrome, so it's in
> ui/file_manager
> 
> maybe ui/webui, but remoting/ also uses externs (for now)
> 
> i think chrome/extensions/externs for extension APIs might make sense?

extensions/externs*?

like i said: I don't have a good spot for these files but i suppose ui/ at least
can be shared by many places...

[Devlin, 2016-02-23 23:12:21]

Patchset #2 (id:20001) has been deleted

[Devlin, 2016-02-23 23:12:39]

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/PRESU...
File extensions/common/api/PRESUBMIT.py (right):

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/PRESU...
extensions/common/api/PRESUBMIT.py:14: 'extensions/common/api/bluetooth.idl':
'ui/externs/bluetooth.js',
On 2016/02/23 03:15:23, Dan Beam wrote:
> nit: use os.path.join() maybe?

Good call, done.

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
File extensions/common/api/externs_checker.py (right):

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker.py:5: class _WarningEntry(object):
On 2016/02/23 03:15:24, Dan Beam wrote:
> nit: is this better than a dict or a tuple?  can this class be private inside
of
> ExternsChecker?

Tuple yes (I originally had it like that and decided it was too easy to mix up
source and extern), dict, no.  Using dict now.

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker.py:9: 
On 2016/02/23 03:15:24, Dan Beam wrote:
> \n\n between file-level globals

Moot

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker.py:11: COMMAND_STUB = str('src/ $ python
tools/json_schema_compiler/compiler.py %s '
On 2016/02/23 03:15:24, Dan Beam wrote:
> nit: prefix with _ (also, STUB seems a bit odd... _COMMAND_TEMPLATE, just
> _COMMAND?)

I like _COMMAND_TEMPLATE, done.

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker.py:23: if (len(entries) == 1):
On 2016/02/23 03:15:24, Dan Beam wrote:
> if len(entries) == 1:

Done.

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker.py:37: if len(bad_files) > 0:
On 2016/02/23 03:15:24, Dan Beam wrote:
> nit: if bad_files:

Ah, right, done.

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
File extensions/common/api/externs_checker_test.py (right):

https://codereview.chromium.org/1718243003/diff/1/extensions/common/api/exter...
extensions/common/api/externs_checker_test.py:10: class MockFile(object):
On 2016/02/23 03:15:24, Dan Beam wrote:
> use src/PRESUBMIT_test_mocks.py

Hey, look at that!

Done.

[Dan Beam, 2016-02-23 23:26:18]

still not sure about ui/...

maybe ui/base?  js can be everywhere...

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
File extensions/common/api/PRESUBMIT.py (right):

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:25: input_api.PresubmitLocalPath(), '..',
'..', '..', 'ui', 'externs')
externs_root = join(api_root, '..', '..', '..', 'ui', 'externs')

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:29: #TODO(devlin): Add more!
nit: # TODO(rdevlin.cronin):

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
File extensions/common/api/externs_checker.py (right):

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
extensions/common/api/externs_checker.py:7: ' %s --root=. --generator=externs >
%s')
why can't we combine the strings in _GetWarningText and _GetCommand?

_UPDATE_MESSAGE = """
To update the externs, run:
src/ $ python tools/json_schema_compiler/compiler.py \
    %s --root. --generator=externs > %s
"""

or something

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
extensions/common/api/externs_checker.py:10: self._api_pairs = api_pairs
why are you initializing members in a different order than receiving them as
arguments?  alphabetizing members?  a little confusing

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
extensions/common/api/externs_checker.py:23: self._GetCommand('<source_file>',
'<output_file>'))
# or source, dest = ... if you want

replacements = ('<source_file>', '<output_file>') if len(entries) > 1 else
    (entries[0]['source'], entries[0]['extern'])
return self._UPDATE_MESSAGE % replacements

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
File extensions/common/api/externs_checker_test.py (right):

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
extensions/common/api/externs_checker_test.py:12:
sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(
nit: why do you have to use dirname() instead of '..'?

[Devlin, 2016-02-24 00:05:06]

On 2016/02/23 23:26:18, Dan Beam wrote:
> still not sure about ui/...
> 
> maybe ui/base?  js can be everywhere...

ui/base sgtm.  I'm assuming we still want an externs subfolder (ui/base/externs)
but lemme know if not.

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
File extensions/common/api/PRESUBMIT.py (right):

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:25: input_api.PresubmitLocalPath(), '..',
'..', '..', 'ui', 'externs')
On 2016/02/23 23:26:17, Dan Beam wrote:
> externs_root = join(api_root, '..', '..', '..', 'ui', 'externs')

Whoops, done.

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:29: #TODO(devlin): Add more!
On 2016/02/23 23:26:17, Dan Beam wrote:
> nit: # TODO(rdevlin.cronin):

I know it's unusual, but I use devlin just about everywhere.  In codesearch,
there's 64 TODO(devlin) and 18 TODO(rdevlin.cronin).  I'd like to stick with
this.

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
File extensions/common/api/externs_checker.py (right):

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
extensions/common/api/externs_checker.py:7: ' %s --root=. --generator=externs >
%s')
On 2016/02/23 23:26:17, Dan Beam wrote:
> why can't we combine the strings in _GetWarningText and _GetCommand?
> 
> _UPDATE_MESSAGE = """
> To update the externs, run:
> src/ $ python tools/json_schema_compiler/compiler.py \
>     %s --root. --generator=externs > %s
> """
> 
> or something

Done.

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
extensions/common/api/externs_checker.py:10: self._api_pairs = api_pairs
On 2016/02/23 23:26:17, Dan Beam wrote:
> why are you initializing members in a different order than receiving them as
> arguments?  alphabetizing members?  a little confusing

Didn't even notice.  Done.

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
extensions/common/api/externs_checker.py:23: self._GetCommand('<source_file>',
'<output_file>'))
On 2016/02/23 23:26:17, Dan Beam wrote:
> # or source, dest = ... if you want
> 
> replacements = ('<source_file>', '<output_file>') if len(entries) > 1 else
>     (entries[0]['source'], entries[0]['extern'])
> return self._UPDATE_MESSAGE % replacements

Done.

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
File extensions/common/api/externs_checker_test.py (right):

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/e...
extensions/common/api/externs_checker_test.py:12:
sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(
On 2016/02/23 23:26:17, Dan Beam wrote:
> nit: why do you have to use dirname() instead of '..'?

Copy-pasted.  Changed to use .. and join.

[Dan Beam, 2016-02-24 00:15:32]

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
File extensions/common/api/PRESUBMIT.py (right):

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:29: #TODO(devlin): Add more!
On 2016/02/24 00:05:05, Devlin (Slow until 2-26) wrote:
> On 2016/02/23 23:26:17, Dan Beam wrote:
> > nit: # TODO(rdevlin.cronin):
> 
> I know it's unusual, but I use devlin just about everywhere.  In codesearch,
> there's 64 TODO(devlin) and 18 TODO(rdevlin.cronin).  I'd like to stick with
> this.

you should change TODO(devlin) to TODO(rdevlin.cronin), IMO
http://www.chromium.org/chromium-os/python-style-guidelines#TOC-TODOs
https://google.github.io/styleguide/pyguide.html#TODO_Comments

[Devlin, 2016-02-24 00:24:23]

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
File extensions/common/api/PRESUBMIT.py (right):

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:29: #TODO(devlin): Add more!
On 2016/02/24 00:15:31, Dan Beam wrote:
> On 2016/02/24 00:05:05, Devlin (Slow until 2-26) wrote:
> > On 2016/02/23 23:26:17, Dan Beam wrote:
> > > nit: # TODO(rdevlin.cronin):
> > 
> > I know it's unusual, but I use devlin just about everywhere.  In codesearch,
> > there's 64 TODO(devlin) and 18 TODO(rdevlin.cronin).  I'd like to stick with
> > this.
> 
> you should change TODO(devlin) to TODO(rdevlin.cronin), IMO
> http://www.chromium.org/chromium-os/python-style-guidelines#TOC-TODOs
> https://google.github.io/styleguide/pyguide.html#TODO_Comments

Changed here.

[Devlin, 2016-02-24 00:27:30]

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
File extensions/common/api/PRESUBMIT.py (right):

https://codereview.chromium.org/1718243003/diff/40001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:29: #TODO(devlin): Add more!
On 2016/02/24 00:24:23, Devlin (Slow until 2-26) wrote:
> On 2016/02/24 00:15:31, Dan Beam wrote:
> > On 2016/02/24 00:05:05, Devlin (Slow until 2-26) wrote:
> > > On 2016/02/23 23:26:17, Dan Beam wrote:
> > > > nit: # TODO(rdevlin.cronin):
> > > 
> > > I know it's unusual, but I use devlin just about everywhere.  In
codesearch,
> > > there's 64 TODO(devlin) and 18 TODO(rdevlin.cronin).  I'd like to stick
with
> > > this.
> > 
> > you should change TODO(devlin) to TODO(rdevlin.cronin), IMO
> > http://www.chromium.org/chromium-os/python-style-guidelines#TOC-TODOs
> > https://google.github.io/styleguide/pyguide.html#TODO_Comments
> 
> Changed here.

For reference, the reasons I prefer devlin are:
- searchable internally and externally (devlin is my codereview setting, and
will also show up on internal chat - rdevlin.cronin will not, as my google
username is different).
- shorter.
- more recognizable.
- avoids people calling me rdevlin. :)

Note also that "devlin" works with rdevlin.cronin for things like "git log
--author=devlin".

[Dan Beam, 2016-02-24 00:50:43]

eh, i still don't really think bluetooth extension API externs belong in
ui/base.

they should be either vertically (i.e. feature) oriented, as in: in some
bluetooth-y or extension-y directory, or horizontally -- some dir with "closure"
in it (because that's really all they're used for).  that's really the only
thing I found ALL externs to have in common: that they're used by the closure
compiler for type checking.  so I slapped them in third_party/closure_compiler.

maybe we should make a different directory for closure stuff if you disagree
with the /third_party/ part?

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/P...
File extensions/common/api/PRESUBMIT.py (right):

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:12: 
\n\n

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:28: #TODO(rdevlin.cronin): Add more!
note: the space after #, as in # TODO(
                                ^

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:32: 
\n\n

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/e...
File extensions/common/api/externs_checker_test.py (right):

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/e...
extensions/common/api/externs_checker_test.py:16: 
\n\n

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/e...
extensions/common/api/externs_checker_test.py:59: 
\n\n

[Devlin, 2016-02-24 04:45:24]

Talking offline about extern location

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/P...
File extensions/common/api/PRESUBMIT.py (right):

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:12: 
On 2016/02/24 00:50:42, Dan Beam wrote:
> \n\n

Done.

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:28: #TODO(rdevlin.cronin): Add more!
On 2016/02/24 00:50:42, Dan Beam wrote:
> note: the space after #, as in # TODO(
>                                 ^

Done.

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/P...
extensions/common/api/PRESUBMIT.py:32: 
On 2016/02/24 00:50:42, Dan Beam wrote:
> \n\n

Done.

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/e...
File extensions/common/api/externs_checker_test.py (right):

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/e...
extensions/common/api/externs_checker_test.py:16: 
On 2016/02/24 00:50:42, Dan Beam wrote:
> \n\n

Done.

https://codereview.chromium.org/1718243003/diff/80001/extensions/common/api/e...
extensions/common/api/externs_checker_test.py:59: 
On 2016/02/24 00:50:42, Dan Beam wrote:
> \n\n

Done.

[Dan Beam, 2016-02-24 19:06:49]

lgtm

https://codereview.chromium.org/1718243003/diff/100001/extensions/common/api/...
File extensions/common/api/externs_checker.py (right):

https://codereview.chromium.org/1718243003/diff/100001/extensions/common/api/...
extensions/common/api/externs_checker.py:15: def _GetWarningText(self, entries):
I don't really see the point of having this to only call once (nor pushing the
knowledge of how to act into here, sharing key names)

would probably be better to pass only the active replacements in from outer
scope or just do all of this there

[Dan Beam, 2016-02-24 19:07:28]

(but you're gonna need to find a ui/base/ owner that agreed with your location
placement)

[Devlin, 2016-02-25 01:08:03]

[email protected] changed reviewers:
+ [email protected]

[Devlin, 2016-02-25 01:08:04]

Scott, are you okay with the externs location?

[Devlin, 2016-02-25 01:14:11]

https://codereview.chromium.org/1718243003/diff/100001/extensions/common/api/...
File extensions/common/api/externs_checker.py (right):

https://codereview.chromium.org/1718243003/diff/100001/extensions/common/api/...
extensions/common/api/externs_checker.py:15: def _GetWarningText(self, entries):
On 2016/02/24 19:06:49, Dan Beam wrote:
> I don't really see the point of having this to only call once (nor pushing the
> knowledge of how to act into here, sharing key names)
> 
> would probably be better to pass only the active replacements in from outer
> scope or just do all of this there

Done.

[Dan Beam, 2016-02-25 01:42:53]

https://codereview.chromium.org/1718243003/diff/120001/extensions/common/api/...
File extensions/common/api/externs_checker.py (right):

https://codereview.chromium.org/1718243003/diff/120001/extensions/common/api/...
extensions/common/api/externs_checker.py:24: replacements = (('<source_file>',
'<output_file>') if len(bad_files) > 1 else
80 col wrap

[sky, 2016-02-25 02:32:10]

On 2016/02/25 01:08:04, Devlin (Slow until 2-26) wrote:
> Scott, are you okay with the externs location?

Why do you want this in ui rather than where the code is used? Isn't this api
very extensions related? Shouldn't it live there?

[Devlin, 2016-02-25 03:58:14]

On 2016/02/25 02:32:10, sky wrote:
> On 2016/02/25 01:08:04, Devlin (Slow until 2-26) wrote:
> > Scott, are you okay with the externs location?
> 
> Why do you want this in ui rather than where the code is used? Isn't this api
> very extensions related? Shouldn't it live there?

This api is, yes - but not all are (and I think it makes more sense for all
externs to live together).  It's also used by chrome resources, remoting/, and
ui/webui.  It seems like adding a dependency from remoting/ and ui/webui/ on
extensions/ isn't necessarily desirable (even though the fact that they use
these externs implies there already is some dependency).  All those places can
pull from ui/ without adding to the dependency list.

I'm not dead-set on this location, but I get the feeling that adding it to
extensions/ might not be quite right either.

[sky, 2016-02-25 17:15:44]

What about extensions/presubmit? Yes, it means the other sites are
pulling from the extensions dir, but it's only the presubmit
directory.

  -Scott

On Wed, Feb 24, 2016 at 7:58 PM,  <[email protected]> wrote:
> On 2016/02/25 02:32:10, sky wrote:
>> On 2016/02/25 01:08:04, Devlin (Slow until 2-26) wrote:
>> > Scott, are you okay with the externs location?
>>
>> Why do you want this in ui rather than where the code is used? Isn't this
>> api
>> very extensions related? Shouldn't it live there?
>
> This api is, yes - but not all are (and I think it makes more sense for all
> externs to live together). It's also used by chrome resources, remoting/,
> and
> ui/webui. It seems like adding a dependency from remoting/ and ui/webui/ on
> extensions/ isn't necessarily desirable (even though the fact that they use
> these externs implies there already is some dependency). All those places
> can
> pull from ui/ without adding to the dependency list.
>
> I'm not dead-set on this location, but I get the feeling that adding it to
> extensions/ might not be quite right either.
>
> https://codereview.chromium.org/1718243003/

-- 
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 [email protected].

[Devlin, 2016-02-25 17:39:36]

Closure externs aren't just for presumbit, so I'm not sure that having them
in a presumbit dir makes sense. We could potentially have
extensions/externs, but it would mean that other externs (e.g. those for
chrome.send, polymer, other custom utils) wouldn't belong and we couldn't
have all externs together. I have a slight preference for there to be a
single extern dir, but can be overridden. :) Dan, do you have a preference
between one extern dir vs extern dirs near the files they correspond with?

On Thu, Feb 25, 2016, 9:15 AM Scott Violet <[email protected]> wrote:

> What about extensions/presubmit? Yes, it means the other sites are
> pulling from the extensions dir, but it's only the presubmit
> directory.
>
>   -Scott
>
> On Wed, Feb 24, 2016 at 7:58 PM,  <[email protected]> wrote:
> > On 2016/02/25 02:32:10, sky wrote:
> >> On 2016/02/25 01:08:04, Devlin (Slow until 2-26) wrote:
> >> > Scott, are you okay with the externs location?
> >>
> >> Why do you want this in ui rather than where the code is used? Isn't
> this
> >> api
> >> very extensions related? Shouldn't it live there?
> >
> > This api is, yes - but not all are (and I think it makes more sense for
> all
> > externs to live together). It's also used by chrome resources, remoting/,
> > and
> > ui/webui. It seems like adding a dependency from remoting/ and ui/webui/
> on
> > extensions/ isn't necessarily desirable (even though the fact that they
> use
> > these externs implies there already is some dependency). All those places
> > can
> > pull from ui/ without adding to the dependency list.
> >
> > I'm not dead-set on this location, but I get the feeling that adding it
> to
> > extensions/ might not be quite right either.
> >
> > https://codereview.chromium.org/1718243003/
>

-- 
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 [email protected].

[sky, 2016-02-25 18:50:15]

Ah, ok. I don't think this makes sense in ui, and I see how it doesn't
make sense in extensions either. I'm not sure what the right place is.

  -Scott

On Thu, Feb 25, 2016 at 9:39 AM, Devlin Cronin
<[email protected]> wrote:
> Closure externs aren't just for presumbit, so I'm not sure that having them
> in a presumbit dir makes sense. We could potentially have
> extensions/externs, but it would mean that other externs (e.g. those for
> chrome.send, polymer, other custom utils) wouldn't belong and we couldn't
> have all externs together. I have a slight preference for there to be a
> single extern dir, but can be overridden. :) Dan, do you have a preference
> between one extern dir vs extern dirs near the files they correspond with?
>
>
> On Thu, Feb 25, 2016, 9:15 AM Scott Violet <[email protected]> wrote:
>>
>> What about extensions/presubmit? Yes, it means the other sites are
>> pulling from the extensions dir, but it's only the presubmit
>> directory.
>>
>>   -Scott
>>
>> On Wed, Feb 24, 2016 at 7:58 PM,  <[email protected]> wrote:
>> > On 2016/02/25 02:32:10, sky wrote:
>> >> On 2016/02/25 01:08:04, Devlin (Slow until 2-26) wrote:
>> >> > Scott, are you okay with the externs location?
>> >>
>> >> Why do you want this in ui rather than where the code is used? Isn't
>> >> this
>> >> api
>> >> very extensions related? Shouldn't it live there?
>> >
>> > This api is, yes - but not all are (and I think it makes more sense for
>> > all
>> > externs to live together). It's also used by chrome resources,
>> > remoting/,
>> > and
>> > ui/webui. It seems like adding a dependency from remoting/ and ui/webui/
>> > on
>> > extensions/ isn't necessarily desirable (even though the fact that they
>> > use
>> > these externs implies there already is some dependency). All those
>> > places
>> > can
>> > pull from ui/ without adding to the dependency list.
>> >
>> > I'm not dead-set on this location, but I get the feeling that adding it
>> > to
>> > extensions/ might not be quite right either.
>> >
>> > https://codereview.chromium.org/1718243003/

-- 
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 [email protected].

[Devlin, 2016-02-26 01:29:06]

[email protected] changed reviewers:
- [email protected]

[Devlin, 2016-02-26 01:29:07]

Since we couldn't decide on a better place to put these, I'm leaving them where
they are for now.  Scott's off the hook for this review (moved to cc).

[Devlin, 2016-02-26 01:29:31]

[email protected] changed reviewers:
+ [email protected]

[Devlin, 2016-02-26 01:29:32]

+John for PRESUBMIT_test_mocks.py

[jam, 2016-02-26 21:33:01]

lgtm

[Devlin, 2016-02-26 21:40:09]

The CQ bit was checked by [email protected]

[Devlin, 2016-02-26 21:40:10]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected]
Link to the patchset: https://codereview.chromium.org/1718243003/#ps140001
(title: "Move file back")

[commit-bot: I haz the power, 2016-02-26 21:42:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1718243003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1718243003/140001

[commit-bot: I haz the power, 2016-02-26 23:17:22]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-02-26 23:18:48]

Description was changed from

==========
[Extern Generation] Add a presubmit script to check externs not being updated

Add a presubmit script that checks for when extension api files are touched, but
the corresponding extern file is not. Right now, this is very simple - it only
checks that the extern is modified in some way (doesn't validate that it's the
*right* way), and is only a warning (because sometimes api file changes don't
cause extern changes). As an improvement, we would validate that the extern
files contain the proper content - let's do that later.

This will also be rolled out piecemeal, since many APIs don't currently have a
dedicated extern file. This change only imposes the check on bluetooth (because
it was handy) - if all goes well, we'll roll this out to all api files.

Also establish a dedicated extern folder, since it's silly for chrome-generated
externs to live in third_party/.

BUG=469920
==========

to

==========
[Extern Generation] Add a presubmit script to check externs not being updated

Add a presubmit script that checks for when extension api files are touched, but
the corresponding extern file is not. Right now, this is very simple - it only
checks that the extern is modified in some way (doesn't validate that it's the
*right* way), and is only a warning (because sometimes api file changes don't
cause extern changes). As an improvement, we would validate that the extern
files contain the proper content - let's do that later.

This will also be rolled out piecemeal, since many APIs don't currently have a
dedicated extern file. This change only imposes the check on bluetooth (because
it was handy) - if all goes well, we'll roll this out to all api files.

Also establish a dedicated extern folder, since it's silly for chrome-generated
externs to live in third_party/.

BUG=469920

Committed: https://crrev.com/9ab806c3e9213b95a7a4ab1342c9b4d5ff49830f
Cr-Commit-Position: refs/heads/master@{#378018}
==========

[commit-bot: I haz the power, 2016-02-26 23:18:49]

Patchset 7 (id:??) landed as
https://crrev.com/9ab806c3e9213b95a7a4ab1342c9b4d5ff49830f
Cr-Commit-Position: refs/heads/master@{#378018}