Creating a directory for the new language detector called CLD3 and

Creating a directory for the new language detector called CLD3 and adding OWNERS, LICENSE, and README.chromium files.

BUG=

Committed: https://crrev.com/63af3b8deab2a1beaf35fa8c385208989145acb8
Cr-Commit-Position: refs/heads/master@{#406137}

[Anton Bakalov, 2016-06-30 02:14:02]

Description was changed from

==========
Creating a directory for the new language detector called CLD3 and
adding OWNERS, LICENSE, and README.chromium files.

BUG=
==========

to

==========
Creating a directory for the new language detector called CLD3 and adding
OWNERS, LICENSE, and README.chromium files.

BUG=
==========

[Anton Bakalov, 2016-06-30 02:14:02]

abakalov@chromium.org changed reviewers:
+ andrewhayden@chromium.org, thakis@chromium.org

[Anton Bakalov, 2016-06-30 02:22:46]

Hi Nico,

This CL sets up the directory for a language detector that will replace the
existing model (//third_party/cld_2). 

Thanks,
Anton

[Andrew Hayden (chromium.org), 2016-06-30 07:25:13]

LGTM!

https://codereview.chromium.org/2110083003/diff/1/third_party/cld_3/LICENSE
File third_party/cld_3/LICENSE (right):

https://codereview.chromium.org/2110083003/diff/1/third_party/cld_3/LICENSE#n...
third_party/cld_3/LICENSE:1: Copyright 2016 The CLD3 Authors.  All rights
reserved.
I don't think you need this up here but I see you have this upstream as well, so
it's fine to copy here.

[Nico, 2016-06-30 15:18:28]

If this code "designed to run in the Chrome browser", and even more importantly
"it relies on code in Chromium", then it should live in the chromium repo (with
the folder it's in mirrored to a separate git repo if you want to provide it for
others). Else, it's not possible to atomically change the code in chromium that
cld3 uses.

[djweiss, 2016-06-30 15:39:53]

On 2016/06/30 15:18:28, Nico wrote:
> If this code "designed to run in the Chrome browser", and even more
importantly
> "it relies on code in Chromium", then it should live in the chromium repo
(with
> the folder it's in mirrored to a separate git repo if you want to provide it
for
> others). Else, it's not possible to atomically change the code in chromium
that
> cld3 uses.

AFAIK, it only depends on StringPiece...Anton, are there any others? We could
remove this dependency in the github repo if that would resolve your concerns.
Our plan is to eventually make CLD3 a stand-alone tool, but our initial priority
is to first solve any issues that would get in the way of replacing CLD2 in
Chromium. We'd rather find out if there's structural issues that need to be
addressed beforehand. Does that sound reasonable?

[Nico, 2016-06-30 16:03:32]

We have a bunch of libraries we use in chrome that are supposed to also be
standalone. We found that life is easier if as many libraries as possible live
in the chrome repo and then are mirrored out for others (examples: grit
https://chromium.googlesource.com/chromium/src/tools/grit/ gn
https://chromium.googlesource.com/chromium/src/tools/gn/ etc). Would that be an
option for you?

[Anton Bakalov, 2016-06-30 17:55:22]

On 2016/06/30 16:03:32, Nico wrote:
> We have a bunch of libraries we use in chrome that are supposed to also be
> standalone. We found that life is easier if as many libraries as possible live
> in the chrome repo and then are mirrored out for others (examples: grit
> https://chromium.googlesource.com/chromium/src/tools/grit/ gn
> https://chromium.googlesource.com/chromium/src/tools/gn/ etc). Would that be
an
> option for you?

I am fine with having the code live in the Chrome repo. What would be the proper
location? Andrew, do you see any disadvantages to this approach? 

To answer the earlier question, the new model depends on Chromium's //base
(i.e., string_piece, logging, macros) but we could remove these dependencies if
necessary.

[Nico, 2016-06-30 18:01:16]

On 2016/06/30 17:55:22, Anton Bakalov wrote:
> On 2016/06/30 16:03:32, Nico wrote:
> > We have a bunch of libraries we use in chrome that are supposed to also be
> > standalone. We found that life is easier if as many libraries as possible
live
> > in the chrome repo and then are mirrored out for others (examples: grit
> > https://chromium.googlesource.com/chromium/src/tools/grit/ gn
> > https://chromium.googlesource.com/chromium/src/tools/gn/ etc). Would that be
> an
> > option for you?
> 
> I am fine with having the code live in the Chrome repo. What would be the
proper
> location? Andrew, do you see any disadvantages to this approach? 

I think the most natural place would probably be components/cld3, but you could
use third_party/cld_3 as well (cld 1 also lived in the src repo, in
third_party).

> To answer the earlier question, the new model depends on Chromium's //base
> (i.e., string_piece, logging, macros) but we could remove these dependencies
if
> necessary.

If you're part of the chromium repo, this is fine from our PoV. If you want to
have a git mirror of your folder for others to use, others might appreciate if
you didn't depend on the rest of src, but you could address this later on.

[Anton Bakalov, 2016-06-30 18:15:35]

On 2016/06/30 18:01:16, Nico wrote:
> On 2016/06/30 17:55:22, Anton Bakalov wrote:
> > On 2016/06/30 16:03:32, Nico wrote:
> > > We have a bunch of libraries we use in chrome that are supposed to also be
> > > standalone. We found that life is easier if as many libraries as possible
> live
> > > in the chrome repo and then are mirrored out for others (examples: grit
> > > https://chromium.googlesource.com/chromium/src/tools/grit/ gn
> > > https://chromium.googlesource.com/chromium/src/tools/gn/ etc). Would that
be
> > an
> > > option for you?
> > 
> > I am fine with having the code live in the Chrome repo. What would be the
> proper
> > location? Andrew, do you see any disadvantages to this approach? 
> 
> I think the most natural place would probably be components/cld3, but you
could
> use third_party/cld_3 as well (cld 1 also lived in the src repo, in
> third_party).
> 
> > To answer the earlier question, the new model depends on Chromium's //base
> > (i.e., string_piece, logging, macros) but we could remove these dependencies
> if
> > necessary.
> 
> If you're part of the chromium repo, this is fine from our PoV. If you want to
> have a git mirror of your folder for others to use, others might appreciate if
> you didn't depend on the rest of src, but you could address this later on.

Yes, as David pointed out, our top priority is getting the model into Chrome
ASAP. Once that is done, we'll spend some extra time cleaning up dependencies
and making the model completely stand-alone.

[Andrew Hayden (chromium.org), 2016-07-01 10:48:35]

On 2016/06/30 18:15:35, Anton Bakalov wrote:
> On 2016/06/30 18:01:16, Nico wrote:
> > On 2016/06/30 17:55:22, Anton Bakalov wrote:
> > > On 2016/06/30 16:03:32, Nico wrote:
> > > > We have a bunch of libraries we use in chrome that are supposed to also
be
> > > > standalone. We found that life is easier if as many libraries as
possible
> > live
> > > > in the chrome repo and then are mirrored out for others (examples: grit
> > > > https://chromium.googlesource.com/chromium/src/tools/grit/ gn
> > > > https://chromium.googlesource.com/chromium/src/tools/gn/ etc). Would
that
> be
> > > an
> > > > option for you?
> > > 
> > > I am fine with having the code live in the Chrome repo. What would be the
> > proper
> > > location? Andrew, do you see any disadvantages to this approach? 
> > 
> > I think the most natural place would probably be components/cld3, but you
> could
> > use third_party/cld_3 as well (cld 1 also lived in the src repo, in
> > third_party).
> > 
> > > To answer the earlier question, the new model depends on Chromium's //base
> > > (i.e., string_piece, logging, macros) but we could remove these
dependencies
> > if
> > > necessary.
> > 
> > If you're part of the chromium repo, this is fine from our PoV. If you want
to
> > have a git mirror of your folder for others to use, others might appreciate
if
> > you didn't depend on the rest of src, but you could address this later on.
> 
> Yes, as David pointed out, our top priority is getting the model into Chrome
> ASAP. Once that is done, we'll spend some extra time cleaning up dependencies
> and making the model completely stand-alone.

So to recap:

1. The code is already in github, the mirror has been created, and scripts have
been built assuming this all lives under third_party/cld_3.
2. Nico has a valid point about the inappropriateness of constraining Chromium
changes because of third_party code.

We should either fix the dependencies and check in to third_party, or do as Nico
suggests. I hate to check this in to components/ if we're going to immediately
move it out as soon as some lingering things are fixed. OTOH we have a schedule
to meet so if it's going to be a month of work to fix the dependencies, I don't
think we have a lot of choices.

Maybe this discussion should be done offline and a decision agreed upon by all.
I am fine with either approach, but would prefer the cleaner third_party option
if feasible.

[Nico, 2016-07-01 15:02:54]

On 2016/07/01 10:48:35, Andrew Hayden (chromium.org) wrote:
> On 2016/06/30 18:15:35, Anton Bakalov wrote:
> > On 2016/06/30 18:01:16, Nico wrote:
> > > On 2016/06/30 17:55:22, Anton Bakalov wrote:
> > > > On 2016/06/30 16:03:32, Nico wrote:
> > > > > We have a bunch of libraries we use in chrome that are supposed to
also
> be
> > > > > standalone. We found that life is easier if as many libraries as
> possible
> > > live
> > > > > in the chrome repo and then are mirrored out for others (examples:
grit
> > > > > https://chromium.googlesource.com/chromium/src/tools/grit/ gn
> > > > > https://chromium.googlesource.com/chromium/src/tools/gn/ etc). Would
> that
> > be
> > > > an
> > > > > option for you?
> > > > 
> > > > I am fine with having the code live in the Chrome repo. What would be
the
> > > proper
> > > > location? Andrew, do you see any disadvantages to this approach? 
> > > 
> > > I think the most natural place would probably be components/cld3, but you
> > could
> > > use third_party/cld_3 as well (cld 1 also lived in the src repo, in
> > > third_party).
> > > 
> > > > To answer the earlier question, the new model depends on Chromium's
//base
> > > > (i.e., string_piece, logging, macros) but we could remove these
> dependencies
> > > if
> > > > necessary.
> > > 
> > > If you're part of the chromium repo, this is fine from our PoV. If you
want
> to
> > > have a git mirror of your folder for others to use, others might
appreciate
> if
> > > you didn't depend on the rest of src, but you could address this later on.
> > 
> > Yes, as David pointed out, our top priority is getting the model into Chrome
> > ASAP. Once that is done, we'll spend some extra time cleaning up
dependencies
> > and making the model completely stand-alone.
> 
> So to recap:
> 
> 1. The code is already in github, the mirror has been created, and scripts
have
> been built assuming this all lives under third_party/cld_3.
> 2. Nico has a valid point about the inappropriateness of constraining Chromium
> changes because of third_party code.
> 
> We should either fix the dependencies and check in to third_party, or do as
Nico
> suggests. I hate to check this in to components/ if we're going to immediately
> move it out as soon as some lingering things are fixed.

Why'd you do that? You could just keep it there.

> OTOH we have a schedule
> to meet so if it's going to be a month of work to fix the dependencies, I
don't
> think we have a lot of choices.
> 
> Maybe this discussion should be done offline and a decision agreed upon by
all.
> I am fine with either approach, but would prefer the cleaner third_party
option
> if feasible.

[Nico, 2016-07-07 01:55:08]

Lgtm

I'm assuming you have the 3 reviews mentioned at
https://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review

[Nico, 2016-07-07 13:32:55]

On 2016/07/07 01:55:08, Nico wrote:
> Lgtm
> 
> I'm assuming you have the 3 reviews mentioned at
>
https://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review

Yes, you need security and license review. The latter should be very quick,
given that the third-party dep is apache-licensed and written by google folks
(you :-) ). This Cl here doesn't actually add anything, so it doesn't block
this, but it does block landing a Cl that actually adds the new library to deps
and adds it to the build.

[Nico, 2016-07-07 17:26:05]

On 2016/07/07 13:32:55, Nico wrote:
> On 2016/07/07 01:55:08, Nico wrote:
> > Lgtm
> > 
> > I'm assuming you have the 3 reviews mentioned at
> >
>
https://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review
> 
> Yes, you need security and license review. The latter should be very quick,
> given that the third-party dep is apache-licensed and written by google folks
> (you :-) ). This Cl here doesn't actually add anything, so it doesn't block
> this, but it does block landing a Cl that actually adds the new library to
deps
> and adds it to the build.

There are two things happening here:

1. Adding a new third-party library. This requires eng, security, legal reviews.

2. Shipping a new feature. This needs the launch bug thing and also eng review.


1 should be pretty light-weight and fast. It's fairly uncommon that a new
feature is just a thing wrapper around a third-party library, which is why we
have both of these things (though they seem somewhat overlapping in this
particular case :-) ).

So for 1, just shoot a quick mail and say "I'm adding a dep to a third-party
cld3 lib, for that cld3 feature <feature bug link>; I trust that's ok?" and that
should be approved in a few hours at most I'd guess. And you can email security@
something like "i'm adding a third-party lib; all the code will be behind a
compile-time switch until the feature's enabled; we're getting a real security
review for launch <feature bug link>. Sound good?" or something along those
lines.

[Nico, 2016-07-07 17:26:46]

(i'm also happy to chat over vc, though by now it's probably no longer
necessary. might've been a good idea early on though, not sure :-) )

[Anton Bakalov, 2016-07-07 18:26:09]

On 2016/07/07 17:26:46, Nico wrote:
> (i'm also happy to chat over vc, though by now it's probably no longer
> necessary. might've been a good idea early on though, not sure :-) )

Thanks for the super helpful information, Nico!
Everything is clear now :-)

[Anton Bakalov, 2016-07-18 22:15:01]

The CQ bit was checked by abakalov@chromium.org

[commit-bot: I haz the power, 2016-07-18 22:16:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 23:37:30]

Description was changed from

==========
Creating a directory for the new language detector called CLD3 and adding
OWNERS, LICENSE, and README.chromium files.

BUG=
==========

to

==========
Creating a directory for the new language detector called CLD3 and adding
OWNERS, LICENSE, and README.chromium files.

BUG=
==========

[commit-bot: I haz the power, 2016-07-18 23:37:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-18 23:37:37]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 23:40:51]

Description was changed from

==========
Creating a directory for the new language detector called CLD3 and adding
OWNERS, LICENSE, and README.chromium files.

BUG=
==========

to

==========
Creating a directory for the new language detector called CLD3 and adding
OWNERS, LICENSE, and README.chromium files.

BUG=

Committed: https://crrev.com/63af3b8deab2a1beaf35fa8c385208989145acb8
Cr-Commit-Position: refs/heads/master@{#406137}
==========

[commit-bot: I haz the power, 2016-07-18 23:40:52]

Patchset 1 (id:??) landed as
https://crrev.com/63af3b8deab2a1beaf35fa8c385208989145acb8
Cr-Commit-Position: refs/heads/master@{#406137}

[Anton Bakalov, 2016-07-18 23:41:26]

Thanks again for the reviews!

https://codereview.chromium.org/2110083003/diff/1/third_party/cld_3/LICENSE
File third_party/cld_3/LICENSE (right):

https://codereview.chromium.org/2110083003/diff/1/third_party/cld_3/LICENSE#n...
third_party/cld_3/LICENSE:1: Copyright 2016 The CLD3 Authors.  All rights
reserved.
On 2016/06/30 07:25:13, Andrew Hayden (chromium.org) wrote:
> I don't think you need this up here but I see you have this upstream as well,
so
> it's fine to copy here.

Acknowledged.