Move Chrome OS pairing interface to chrome/browser/chromeos

Move Chrome OS pairing interface to chrome/browser/chromeos

BUG=393413
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284549

[Zachary Kuznia, 2014-07-12 03:45:36]

Please take a look.

[chromium-reviews, 2014-07-12 06:58:45]

Why it can't live in chromeos?
12 июля 2014 г. 7:45 пользователь <zork@chromium.org> написал:

> Reviewers: dzhioev,
>
> Message:
> Please take a look.
>
> Description:
> Move Chrome OS pairing interface to chrome/browser/chromeos
>
> BUG=393413
>
> Please review this at https://codereview.chromium.org/387163002/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+34, -979 lines):
>   M chrome/browser/chromeos/login/screens/controller_pairing_screen.h
>   M chrome/browser/chromeos/login/screens/controller_pairing_screen.cc
>   M chrome/browser/chromeos/login/screens/host_pairing_screen.h
>   M chrome/browser/chromeos/login/screens/host_pairing_screen.cc
>   A + chrome/browser/chromeos/pairing/OWNERS
>   A + chrome/browser/chromeos/pairing/controller_pairing_controller.h
>   A + chrome/browser/chromeos/pairing/controller_pairing_controller.cc
>   A + chrome/browser/chromeos/pairing/fake_controller_pairing_controller.h
>   A + chrome/browser/chromeos/pairing/fake_controller_
> pairing_controller.cc
>   A + chrome/browser/chromeos/pairing/fake_host_pairing_controller.h
>   A + chrome/browser/chromeos/pairing/fake_host_pairing_controller.cc
>   A + chrome/browser/chromeos/pairing/host_pairing_controller.h
>   A + chrome/browser/chromeos/pairing/host_pairing_controller.cc
>   M chrome/chrome_browser_chromeos.gypi
>   M chromeos/chromeos.gyp
>   D chromeos/pairing/OWNERS
>   D chromeos/pairing/controller_pairing_controller.h
>   D chromeos/pairing/controller_pairing_controller.cc
>   D chromeos/pairing/fake_controller_pairing_controller.h
>   D chromeos/pairing/fake_controller_pairing_controller.cc
>   D chromeos/pairing/fake_host_pairing_controller.h
>   D chromeos/pairing/fake_host_pairing_controller.cc
>   D chromeos/pairing/host_pairing_controller.h
>   D chromeos/pairing/host_pairing_controller.cc
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dzhioev (left Google), 2014-07-14 12:14:24]

+1
On 2014/07/12 06:58:45, chromium-reviews wrote:
> Why it can't live in chromeos?
> 12 июля 2014 г. 7:45 пользователь <mailto:zork@chromium.org> написал:
> 
> > Reviewers: dzhioev,
> >
> > Message:
> > Please take a look.
> >
> > Description:
> > Move Chrome OS pairing interface to chrome/browser/chromeos
> >
> > BUG=393413
> >
> > Please review this at https://codereview.chromium.org/387163002/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >
> > Affected files (+34, -979 lines):
> >   M chrome/browser/chromeos/login/screens/controller_pairing_screen.h
> >   M chrome/browser/chromeos/login/screens/controller_pairing_screen.cc
> >   M chrome/browser/chromeos/login/screens/host_pairing_screen.h
> >   M chrome/browser/chromeos/login/screens/host_pairing_screen.cc
> >   A + chrome/browser/chromeos/pairing/OWNERS
> >   A + chrome/browser/chromeos/pairing/controller_pairing_controller.h
> >   A + chrome/browser/chromeos/pairing/controller_pairing_controller.cc
> >   A + chrome/browser/chromeos/pairing/fake_controller_pairing_controller.h
> >   A + chrome/browser/chromeos/pairing/fake_controller_
> > pairing_controller.cc
> >   A + chrome/browser/chromeos/pairing/fake_host_pairing_controller.h
> >   A + chrome/browser/chromeos/pairing/fake_host_pairing_controller.cc
> >   A + chrome/browser/chromeos/pairing/host_pairing_controller.h
> >   A + chrome/browser/chromeos/pairing/host_pairing_controller.cc
> >   M chrome/chrome_browser_chromeos.gypi
> >   M chromeos/chromeos.gyp
> >   D chromeos/pairing/OWNERS
> >   D chromeos/pairing/controller_pairing_controller.h
> >   D chromeos/pairing/controller_pairing_controller.cc
> >   D chromeos/pairing/fake_controller_pairing_controller.h
> >   D chromeos/pairing/fake_controller_pairing_controller.cc
> >   D chromeos/pairing/fake_host_pairing_controller.h
> >   D chromeos/pairing/fake_host_pairing_controller.cc
> >   D chromeos/pairing/host_pairing_controller.h
> >   D chromeos/pairing/host_pairing_controller.cc
> >
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nikita (slow), 2014-07-14 13:11:58]

> On 2014/07/12 06:58:45, chromium-reviews wrote:
> > Why it can't live in chromeos?

This was my reply, don't know why replying in the email didn't pick up the
correct user.

[Zachary Kuznia, 2014-07-14 15:03:01]

On 2014/07/14 13:11:58, Nikita Kostylev wrote:
> > On 2014/07/12 06:58:45, chromium-reviews wrote:
> > > Why it can't live in chromeos?
> 
> This was my reply, don't know why replying in the email didn't pick up the
> correct user.

See: https://codereview.chromium.org/372733004/

The short of it is that src/device/bluetooth depends on src/chromeos, so using
Bluetooth in src/chromeos/pairing would create a dependency loop.

[Nikita (slow), 2014-07-14 15:15:29]

On 2014/07/14 15:03:01, Zachary Kuznia wrote:
> On 2014/07/14 13:11:58, Nikita Kostylev wrote:
> > > On 2014/07/12 06:58:45, chromium-reviews wrote:
> > > > Why it can't live in chromeos?
> > 
> > This was my reply, don't know why replying in the email didn't pick up the
> > correct user.
> 
> See: https://codereview.chromium.org/372733004/
> 
> The short of it is that src/device/bluetooth depends on src/chromeos, so using
> Bluetooth in src/chromeos/pairing would create a dependency loop.

+Steven

We should think twice before adding more code into src/chrome since we're
actively trying to split it into components and actually move code *out* of
src/chrome.

Should this be component instead?

[stevenjb, 2014-07-14 16:51:29]

On 2014/07/14 15:15:29, Nikita Kostylev wrote:
> On 2014/07/14 15:03:01, Zachary Kuznia wrote:
> > On 2014/07/14 13:11:58, Nikita Kostylev wrote:
> > > > On 2014/07/12 06:58:45, chromium-reviews wrote:
> > > > > Why it can't live in chromeos?
> > > 
> > > This was my reply, don't know why replying in the email didn't pick up the
> > > correct user.
> > 
> > See: https://codereview.chromium.org/372733004/
> > 
> > The short of it is that src/device/bluetooth depends on src/chromeos, so
using
> > Bluetooth in src/chromeos/pairing would create a dependency loop.
> 
> +Steven
> 
> We should think twice before adding more code into src/chrome since we're
> actively trying to split it into components and actually move code *out* of
> src/chrome.
> 
> Should this be component instead?

Moving discussion to the issue.

[Zachary Kuznia, 2014-07-15 22:36:33]

On 2014/07/14 16:51:29, stevenjb wrote:
> On 2014/07/14 15:15:29, Nikita Kostylev wrote:
> > On 2014/07/14 15:03:01, Zachary Kuznia wrote:
> > > On 2014/07/14 13:11:58, Nikita Kostylev wrote:
> > > > > On 2014/07/12 06:58:45, chromium-reviews wrote:
> > > > > > Why it can't live in chromeos?
> > > > 
> > > > This was my reply, don't know why replying in the email didn't pick up
the
> > > > correct user.
> > > 
> > > See: https://codereview.chromium.org/372733004/
> > > 
> > > The short of it is that src/device/bluetooth depends on src/chromeos, so
> using
> > > Bluetooth in src/chromeos/pairing would create a dependency loop.
> > 
> > +Steven
> > 
> > We should think twice before adding more code into src/chrome since we're
> > actively trying to split it into components and actually move code *out* of
> > src/chrome.
> > 
> > Should this be component instead?
> 
> Moving discussion to the issue.

Moved to src/components/pairing, per discussion.

[dzhioev (left Google), 2014-07-16 12:28:25]

On 2014/07/15 22:36:33, Zachary Kuznia wrote:
> On 2014/07/14 16:51:29, stevenjb wrote:
> > On 2014/07/14 15:15:29, Nikita Kostylev wrote:
> > > On 2014/07/14 15:03:01, Zachary Kuznia wrote:
> > > > On 2014/07/14 13:11:58, Nikita Kostylev wrote:
> > > > > > On 2014/07/12 06:58:45, chromium-reviews wrote:
> > > > > > > Why it can't live in chromeos?
> > > > > 
> > > > > This was my reply, don't know why replying in the email didn't pick up
> the
> > > > > correct user.
> > > > 
> > > > See: https://codereview.chromium.org/372733004/
> > > > 
> > > > The short of it is that src/device/bluetooth depends on src/chromeos, so
> > using
> > > > Bluetooth in src/chromeos/pairing would create a dependency loop.
> > > 
> > > +Steven
> > > 
> > > We should think twice before adding more code into src/chrome since we're
> > > actively trying to split it into components and actually move code *out*
of
> > > src/chrome.
> > > 
> > > Should this be component instead?
> > 
> > Moving discussion to the issue.
> 
> Moved to src/components/pairing, per discussion.

LGTM

[Nikita (slow), 2014-07-16 12:29:55]

lgtm with nit

https://codereview.chromium.org/387163002/diff/60001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/387163002/diff/60001/components/components.gy...
components/components.gyp:41: 'pairing.gypi',
I think you need to place this under os == 'chromeos' condition below.

[stevenjb, 2014-07-16 17:56:49]

The pairing code is still in the chromeos namespace, but I don't see any actual
chromeos dependencies in the files.

We should either remove the pairing code from the chromeos namespace and
introduce a 'pairing' namespace, or we should name the component something like
'pairing_chromeos' and only include it on chromeos if we want to be able to use
chromeos specific code inside the component.

I think the former is better unless we anticipate a lot if chromeos
dependencies.

[Zachary Kuznia, 2014-07-16 18:01:15]

On 2014/07/16 17:56:49, stevenjb wrote:
> The pairing code is still in the chromeos namespace, but I don't see any
actual
> chromeos dependencies in the files.
> 
> We should either remove the pairing code from the chromeos namespace and
> introduce a 'pairing' namespace, or we should name the component something
like
> 'pairing_chromeos' and only include it on chromeos if we want to be able to
use
> chromeos specific code inside the component.
> 
> I think the former is better unless we anticipate a lot if chromeos
> dependencies.

The bluetooth implementation only depends on device/bluetooth, so I'm happy to
start with namespace pairing, and change it later if more dependencies appear.

[stevenjb, 2014-07-16 18:19:49]

On 2014/07/16 18:01:15, Zachary Kuznia wrote:
> On 2014/07/16 17:56:49, stevenjb wrote:
> > The pairing code is still in the chromeos namespace, but I don't see any
> actual
> > chromeos dependencies in the files.
> > 
> > We should either remove the pairing code from the chromeos namespace and
> > introduce a 'pairing' namespace, or we should name the component something
> like
> > 'pairing_chromeos' and only include it on chromeos if we want to be able to
> use
> > chromeos specific code inside the component.
> > 
> > I think the former is better unless we anticipate a lot if chromeos
> > dependencies.
> 
> The bluetooth implementation only depends on device/bluetooth, so I'm happy to
> start with namespace pairing, and change it later if more dependencies appear.

sgtm, thanks!

[Zachary Kuznia, 2014-07-16 20:22:14]

On 2014/07/16 18:19:49, stevenjb wrote:
> On 2014/07/16 18:01:15, Zachary Kuznia wrote:
> > On 2014/07/16 17:56:49, stevenjb wrote:
> > > The pairing code is still in the chromeos namespace, but I don't see any
> > actual
> > > chromeos dependencies in the files.
> > > 
> > > We should either remove the pairing code from the chromeos namespace and
> > > introduce a 'pairing' namespace, or we should name the component something
> > like
> > > 'pairing_chromeos' and only include it on chromeos if we want to be able
to
> > use
> > > chromeos specific code inside the component.
> > > 
> > > I think the former is better unless we anticipate a lot if chromeos
> > > dependencies.
> > 
> > The bluetooth implementation only depends on device/bluetooth, so I'm happy
to
> > start with namespace pairing, and change it later if more dependencies
appear.
> 
> sgtm, thanks!

I appear to have been mistaken; it does rely on chromeos::UserContext.  I'll put
it in pairing_chromeos namespace instead.

[Zachary Kuznia, 2014-07-16 20:50:45]

https://codereview.chromium.org/387163002/diff/60001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/387163002/diff/60001/components/components.gy...
components/components.gyp:41: 'pairing.gypi',
On 2014/07/16 12:29:55, Nikita Kostylev wrote:
> I think you need to place this under os == 'chromeos' condition below.

Done.

[Zachary Kuznia, 2014-07-16 20:53:44]

jochen: Could you do an OWNERS review of:
components/components.gyp
components/pairing.gypi

armansito: Could you do an OWNERS review for adding DEPS for
'+device/bluetooth'?

Thanks,
-Zach

[stevenjb, 2014-07-16 21:18:49]

On 2014/07/16 20:22:14, Zachary Kuznia wrote:
> On 2014/07/16 18:19:49, stevenjb wrote:
> > On 2014/07/16 18:01:15, Zachary Kuznia wrote:
> > > On 2014/07/16 17:56:49, stevenjb wrote:
> > > > The pairing code is still in the chromeos namespace, but I don't see any
> > > actual
> > > > chromeos dependencies in the files.
> > > > 
> > > > We should either remove the pairing code from the chromeos namespace and
> > > > introduce a 'pairing' namespace, or we should name the component
something
> > > like
> > > > 'pairing_chromeos' and only include it on chromeos if we want to be able
> to
> > > use
> > > > chromeos specific code inside the component.
> > > > 
> > > > I think the former is better unless we anticipate a lot if chromeos
> > > > dependencies.
> > > 
> > > The bluetooth implementation only depends on device/bluetooth, so I'm
happy
> to
> > > start with namespace pairing, and change it later if more dependencies
> appear.
> > 
> > sgtm, thanks!
> 
> I appear to have been mistaken; it does rely on chromeos::UserContext.  I'll
put
> it in pairing_chromeos namespace instead.

I assume that dependency is in a forthcoming CL?

It might be worthwhile to put that code in an _chromeos.cc file and namespace it
pairing::chromeos, leaving the rest of this in just pairing, but that could be
done later also. I am OK either way, lgtm.

[armansito, 2014-07-16 22:12:14]

components/pairing/DEPS lgtm

[jochen (gone - plz use gerrit), 2014-07-17 07:14:19]

https://codereview.chromium.org/387163002/diff/80001/components/pairing.gypi
File components/pairing.gypi (right):

https://codereview.chromium.org/387163002/diff/80001/components/pairing.gypi#...
components/pairing.gypi:5: {
can you also add a BUILD.gn version?

https://codereview.chromium.org/387163002/diff/80001/components/pairing/DEPS
File components/pairing/DEPS (right):

https://codereview.chromium.org/387163002/diff/80001/components/pairing/DEPS#...
components/pairing/DEPS:2: "+device/bluetooth",
if you depend on this, the gyp target should also depend on this

https://codereview.chromium.org/387163002/diff/80001/components/pairing/fake_...
File components/pairing/fake_host_pairing_controller.h (right):

https://codereview.chromium.org/387163002/diff/80001/components/pairing/fake_...
components/pairing/fake_host_pairing_controller.h:8: #include "base/macros.h"
if you use base, you should depend on it in gyp

https://codereview.chromium.org/387163002/diff/80001/components/pairing/pairi...
File components/pairing/pairing_export.h (right):

https://codereview.chromium.org/387163002/diff/80001/components/pairing/pairi...
components/pairing/pairing_export.h:5: #ifndef
COMPONENTS_PAIRING_PAIRING_EXPORT_H_
not required for a static library

[Zachary Kuznia, 2014-07-17 22:14:47]

https://codereview.chromium.org/387163002/diff/80001/components/pairing.gypi
File components/pairing.gypi (right):

https://codereview.chromium.org/387163002/diff/80001/components/pairing.gypi#...
components/pairing.gypi:5: {
On 2014/07/17 07:14:19, jochen wrote:
> can you also add a BUILD.gn version?

Done.

https://codereview.chromium.org/387163002/diff/80001/components/pairing/DEPS
File components/pairing/DEPS (right):

https://codereview.chromium.org/387163002/diff/80001/components/pairing/DEPS#...
components/pairing/DEPS:2: "+device/bluetooth",
On 2014/07/17 07:14:19, jochen wrote:
> if you depend on this, the gyp target should also depend on this

Done.

https://codereview.chromium.org/387163002/diff/80001/components/pairing/fake_...
File components/pairing/fake_host_pairing_controller.h (right):

https://codereview.chromium.org/387163002/diff/80001/components/pairing/fake_...
components/pairing/fake_host_pairing_controller.h:8: #include "base/macros.h"
On 2014/07/17 07:14:19, jochen wrote:
> if you use base, you should depend on it in gyp

Done.

https://codereview.chromium.org/387163002/diff/80001/components/pairing/pairi...
File components/pairing/pairing_export.h (right):

https://codereview.chromium.org/387163002/diff/80001/components/pairing/pairi...
components/pairing/pairing_export.h:5: #ifndef
COMPONENTS_PAIRING_PAIRING_EXPORT_H_
On 2014/07/17 07:14:19, jochen wrote:
> not required for a static library

This whole file and PAIRING_EXPORT is not required?

[jochen (gone - plz use gerrit), 2014-07-18 09:18:00]

On 2014/07/17 at 22:14:47, zork wrote: 
>
https://codereview.chromium.org/387163002/diff/80001/components/pairing/pairi...
> File components/pairing/pairing_export.h (right):
> 
>
https://codereview.chromium.org/387163002/diff/80001/components/pairing/pairi...
> components/pairing/pairing_export.h:5: #ifndef
COMPONENTS_PAIRING_PAIRING_EXPORT_H_
> On 2014/07/17 07:14:19, jochen wrote:
> > not required for a static library
> 
> This whole file and PAIRING_EXPORT is not required?

Yes. Alternatively, fix up the gyp file so this target is compiled as a shared
library

[Zachary Kuznia, 2014-07-18 17:35:23]

On 2014/07/18 09:18:00, jochen wrote:
> On 2014/07/17 at 22:14:47, zork wrote: 
> >
>
https://codereview.chromium.org/387163002/diff/80001/components/pairing/pairi...
> > File components/pairing/pairing_export.h (right):
> > 
> >
>
https://codereview.chromium.org/387163002/diff/80001/components/pairing/pairi...
> > components/pairing/pairing_export.h:5: #ifndef
> COMPONENTS_PAIRING_PAIRING_EXPORT_H_
> > On 2014/07/17 07:14:19, jochen wrote:
> > > not required for a static library
> > 
> > This whole file and PAIRING_EXPORT is not required?
> 
> Yes. Alternatively, fix up the gyp file so this target is compiled as a shared
> library

Removed PAIRING_EXPORT

[jochen (gone - plz use gerrit), 2014-07-21 11:25:43]

lgtm with nit addressed

https://codereview.chromium.org/387163002/diff/160001/components/pairing/BUIL...
File components/pairing/BUILD.gn (right):

https://codereview.chromium.org/387163002/diff/160001/components/pairing/BUIL...
components/pairing/BUILD.gn:5: component("pairing") {
should be source_set

[Zachary Kuznia, 2014-07-21 17:27:39]

https://codereview.chromium.org/387163002/diff/160001/components/pairing/BUIL...
File components/pairing/BUILD.gn (right):

https://codereview.chromium.org/387163002/diff/160001/components/pairing/BUIL...
components/pairing/BUILD.gn:5: component("pairing") {
On 2014/07/21 11:25:42, jochen wrote:
> should be source_set

Done.

[Zachary Kuznia, 2014-07-21 17:48:28]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-07-21 17:49:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zork@chromium.org/387163002/180001

[commit-bot: I haz the power, 2014-07-21 21:16:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-22 00:17:44]

Change committed as 284549