Migrating majority of gamepad from content/browser/ to device/

Migrating majority of gamepad from content/browser/ to device/

BUG=612330
Committed: https://crrev.com/b9bb4c42d6b19a7d7131af942d3769c54e89981c
Cr-Commit-Position: refs/heads/master@{#403998}

[bajones, 2016-06-22 22:25:06]

bajones@chromium.org changed reviewers:
+ jam@chromium.org, piman@chromium.org, rockot@chromium.org,
scottmg@chromium.org

[bajones, 2016-06-22 22:25:07]

piman@: Please review changes in content/ (mostly removals)

rockot@: Please review changes in device/ (mostly moves from content/ with the
appropriate renames)

This is step one of the migration of Gamepad from content/browser/ to device/.
The next step will be to convert it to mojo, which will allow us to move
GamepadService into device/ as well. Left that out of this CL for the sake of
reviewability, since this is already a large patch.

Aside from the mechanical moves/renames the biggest changes here are that
GamepadProvider no longer has direct visibility of the GamepadHardwareBuffer
(Access to it has been abstracted through GamepadSharedBuffer) or GamepadService
(now provided as a GamepadConnectionChangeClient). I intend to remove both of
these abstractions once the mojo change is complete, as they only really exists
to break the dependency to content/ while some parts of Gamepad still live
there.

[piman, 2016-06-22 22:53:00]

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
File content/browser/gamepad/gamepad_service.cc (right):

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
content/browser/gamepad/gamepad_service.cc:34:
std::unique_ptr<device::GamepadSharedBuffer>(
nit: base::MakeUnique<GamepadSharedBufferImpl>() should work (the unique_ptr
should upcast correctly).

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
content/browser/gamepad/gamepad_service.cc:65:
std::unique_ptr<device::GamepadSharedBuffer>(
nit: ditto.

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
content/browser/gamepad/gamepad_service.cc:120: base::Unretained(this), index,
pad));
What makes Unretained safe?

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
content/browser/gamepad/gamepad_service.cc:124: base::Unretained(this), index,
pad));
ditto

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
File content/browser/gamepad/gamepad_shared_buffer_impl.cc (right):

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
content/browser/gamepad/gamepad_shared_buffer_impl.cc:16: memset(hwbuf, 0,
sizeof(GamepadHardwareBuffer));
nit: use placement new? GamepadHardwareBuffer is not an aggregate/POD, so
accessing without constructing is undefined behavior.

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
content/browser/gamepad/gamepad_shared_buffer_impl.cc:33: CHECK(mem);
nit: DCHECK

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
File content/browser/gamepad/gamepad_shared_buffer_impl.h (right):

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
content/browser/gamepad/gamepad_shared_buffer_impl.h:15: class
GamepadSharedBufferImpl : public device::GamepadSharedBuffer{
nit: space before {

[jam, 2016-06-22 23:07:02]

I'm added as a reviewer but you didn't mention any files for me to look at, so I
just commented on DEPS.

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
File content/browser/gamepad/DEPS (right):

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
content/browser/gamepad/DEPS:2: "+device/gamepad",
not needed because you added this to content/DEPS

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/DEPS
File device/gamepad/DEPS (right):

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/DEPS#new...
device/gamepad/DEPS:4: "+third_party/WebKit/public/platform/WebGamepads.h",
to be clear, both of these would be removed once mojo conversion happens right?
i.e. because the structs would be defined by mojo files in device/gamepad/ ?

[bajones, 2016-06-22 23:16:37]

Thanks for the feedback! Working on fixes now with one follow up question.

On 2016/06/22 22:53:00, piman wrote:
>
https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
> content/browser/gamepad/gamepad_shared_buffer_impl.cc:16: memset(hwbuf, 0,
> sizeof(GamepadHardwareBuffer));
> nit: use placement new? GamepadHardwareBuffer is not an aggregate/POD, so
> accessing without constructing is undefined behavior.

I can see the problem you're referring to but I'm not sure what you mean by "use
placement new?"

FWIW this was a direct port from the existing code
(https://cs.chromium.org/chromium/src/content/browser/gamepad/gamepad_provider...)
so this is probably a long-standing bug.

[bajones, 2016-06-22 23:20:52]

On 2016/06/22 23:07:02, jam wrote:
> I'm added as a reviewer but you didn't mention any files for me to look at, so
I
> just commented on DEPS.

Yeah, apologies. I added you simply because you were the one that requested the
migration.
 
>
https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/DEPS#new...
> device/gamepad/DEPS:4: "+third_party/WebKit/public/platform/WebGamepads.h",
> to be clear, both of these would be removed once mojo conversion happens
right?
> i.e. because the structs would be defined by mojo files in device/gamepad/ ?

That's correct.

[piman, 2016-06-22 23:21:13]

On 2016/06/22 23:16:37, bajones wrote:
> Thanks for the feedback! Working on fixes now with one follow up question.
> 
> On 2016/06/22 22:53:00, piman wrote:
> >
>
https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
> > content/browser/gamepad/gamepad_shared_buffer_impl.cc:16: memset(hwbuf, 0,
> > sizeof(GamepadHardwareBuffer));
> > nit: use placement new? GamepadHardwareBuffer is not an aggregate/POD, so
> > accessing without constructing is undefined behavior.
> 
> I can see the problem you're referring to but I'm not sure what you mean by
"use
> placement new?"
> 
> FWIW this was a direct port from the existing code
>
(https://cs.chromium.org/chromium/src/content/browser/gamepad/gamepad_provider...)
> so this is probably a long-standing bug.

I think the following should work:
new (shared_memory_.memory()) GamepadHardwareBuffer();

(instead of memset)

[bajones, 2016-06-23 01:50:05]

On 2016/06/22 23:21:13, piman wrote:
> I think the following should work:
> new (shared_memory_.memory()) GamepadHardwareBuffer();
> 
> (instead of memset)

Thanks for the clarification! kbr@ explained it to me as well. Didn't realize
this was a thing you could do in C++, so thanks for teaching me something new
today! :D

[Ken Rockot(use gerrit already), 2016-06-23 02:17:04]

All seems fairly reasonable to me. One concern I have though (sorry!): now that
//components no longer has any blanket DEPS restrictions (i.e. it's effectively
a mass of independent top-level-ish things) I'd like to kill //device. IIRC from
discussions with other owners in device we have some consensus there. How do you
feel about moving this to //components/gamepad instead?

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/BUILD.gn
File device/gamepad/BUILD.gn (right):

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/BUILD.gn...
device/gamepad/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights
reserved.
Missing gyp rules for all this? I know, I wish it were gone too. ;)

[bajones, 2016-06-23 03:25:46]

On 2016/06/23 02:17:04, Ken Rockot wrote:
> All seems fairly reasonable to me. One concern I have though (sorry!): now
that
> //components no longer has any blanket DEPS restrictions (i.e. it's
effectively
> a mass of independent top-level-ish things) I'd like to kill //device. IIRC
from
> discussions with other owners in device we have some consensus there. How do
you
> feel about moving this to //components/gamepad instead?

Going to defer to jam@ on that one, since he's the one that originally requested
it be moved here. Doesn't seem like there's a big difference in the code either
way, aside from updating the namespaces.

> https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/BUILD.gn
> File device/gamepad/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/BUILD.gn...
> device/gamepad/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights
> reserved.
> Missing gyp rules for all this? I know, I wish it were gone too. ;)

*sigh* Thanks for catching that. I knew this somehow felt too easy. Gyp is
completely dead on Android at least, right?

[jam, 2016-06-23 16:05:42]

On 2016/06/23 03:25:46, bajones wrote:
> On 2016/06/23 02:17:04, Ken Rockot wrote:
> > All seems fairly reasonable to me. One concern I have though (sorry!): now
> that
> > //components no longer has any blanket DEPS restrictions (i.e. it's
> effectively
> > a mass of independent top-level-ish things) I'd like to kill //device. IIRC
> from
> > discussions with other owners in device we have some consensus there. How do
> you
> > feel about moving this to //components/gamepad instead?
> 
> Going to defer to jam@ on that one, since he's the one that originally
requested
> it be moved here. Doesn't seem like there's a big difference in the code
either
> way, aside from updating the namespaces.

Once all the hardware related features move out of content, device/ will have
nearly 20 directories. It seems like a large enough grouping, one that will
probably continue getting larger, to separate out from components/. In the long
term, most likely they will all run together in one (device) process.

[Ken Rockot(use gerrit already), 2016-06-23 16:20:46]

On 2016/06/23 at 16:05:42, jam wrote:
> On 2016/06/23 03:25:46, bajones wrote:
> > On 2016/06/23 02:17:04, Ken Rockot wrote:
> > > All seems fairly reasonable to me. One concern I have though (sorry!): now
> > that
> > > //components no longer has any blanket DEPS restrictions (i.e. it's
> > effectively
> > > a mass of independent top-level-ish things) I'd like to kill //device.
IIRC
> > from
> > > discussions with other owners in device we have some consensus there. How
do
> > you
> > > feel about moving this to //components/gamepad instead?
> > 
> > Going to defer to jam@ on that one, since he's the one that originally
requested
> > it be moved here. Doesn't seem like there's a big difference in the code
either
> > way, aside from updating the namespaces.
> 
> Once all the hardware related features move out of content, device/ will have
nearly 20 directories. It seems like a large enough grouping, one that will
probably continue getting larger, to separate out from components/. In the long
term, most likely they will all run together in one (device) process.

I think it's potentially confusing to have this arbitrary line drawn between
device-oriented components and all other components. It's not perfectly clear
what should and shouldn't fall on one side or the other either. I'm not entirely
sure media_transfer_protocol makes sense in device - maybe it does, it has
something to do with devices albeit indirectly. Maybe things like audio_modem,
cloud_devices, copresence, pairing, or even onc belong in device. There seems
like a lot of gray area and we don't make this sort of distinction for other
types of components.

[bajones, 2016-06-23 16:29:56]

Since it seems like there's still some question about where these things should
be located long term I'm going to continue with the move to device/ unless I get
a strong signal otherwise. From my perspective the exact directory that this
code lives in isn't too important, what really matters with this patch and it's
follow ups is breaking the dependency on content/.

[Ken Rockot(use gerrit already), 2016-06-23 16:31:09]

SGTM, agreed it's not worth blocking on that discussion. If it can move
once it can move twice!

On Thu, Jun 23, 2016 at 9:29 AM, <bajones@chromium.org> wrote:

> Since it seems like there's still some question about where these things
> should
> be located long term I'm going to continue with the move to device/ unless
> I get
> a strong signal otherwise. From my perspective the exact directory that
> this
> code lives in isn't too important, what really matters with this patch and
> it's
> follow ups is breaking the dependency on content/.
>
>
> https://codereview.chromium.org/2081583002/
>

-- 
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 chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2016-06-23 17:27:19]

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
File content/browser/gamepad/gamepad_shared_buffer_impl.h (right):

https://codereview.chromium.org/2081583002/diff/80001/content/browser/gamepad...
content/browser/gamepad/gamepad_shared_buffer_impl.h:5: #ifndef
CONTENT_COMMON_GAMEPAD_SHARED_BUFFER_IMPL_H_
This doesn't match the filename.

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/android/...
File
device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadList.java
(right):

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/android/...
device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadList.java:19:
// import org.chromium.content.browser.ContentView;
Remove.

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/gamepad_...
File device/gamepad/gamepad_provider.h (right):

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/gamepad_...
device/gamepad/gamepad_provider.h:166: // base::SharedMemory
gamepad_shared_memory_;
?

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/gamepad_...
File device/gamepad/gamepad_provider_unittest.cc (right):

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/gamepad_...
device/gamepad/gamepad_provider_unittest.cc:48: NULL,
std::unique_ptr<GamepadDataFetcher>(mock_data_fetcher_)));
nullptr

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/gamepad_...
File device/gamepad/gamepad_shared_buffer.h (right):

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/gamepad_...
device/gamepad/gamepad_shared_buffer.h:5: #ifndef
DEVICE_GAMEPAD_HARDWARE_BUFFER_H_
This doesn't match the file name.

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/gamepad_...
device/gamepad/gamepad_shared_buffer.h:15: public:
I think you have to have a virtual ~GamepadSharedBuffer() here, else
~GamepadSharedBufferImpl() mightn't get called.

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/gamepad_...
File device/gamepad/gamepad_standard_mappings.h (right):

https://codereview.chromium.org/2081583002/diff/80001/device/gamepad/gamepad_...
device/gamepad/gamepad_standard_mappings.h:10: namespace device {
\n before

[bajones, 2016-06-23 19:09:50]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 19:10:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2081583002/100001

[commit-bot: I haz the power, 2016-06-23 19:15:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-23 19:15:33]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[bajones, 2016-06-23 21:28:33]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 21:29:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2081583002/120001

[commit-bot: I haz the power, 2016-06-23 21:35:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-23 21:35:48]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bajones, 2016-06-23 22:39:21]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 22:39:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2081583002/140001

[commit-bot: I haz the power, 2016-06-23 22:48:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-23 22:48:49]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[bajones, 2016-06-24 19:05:05]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 19:05:55]

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

[bajones, 2016-06-24 19:17:24]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 19:18:22]

Dry run: 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-06-24 19:41:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-24 19:41:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bajones, 2016-06-24 22:50:55]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 22:51:14]

Dry run: 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-06-24 23:00:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-24 23:00:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bajones, 2016-06-24 23:44:38]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 23:45:14]

Dry run: 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-06-25 00:02:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-25 00:02:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bajones, 2016-06-29 22:36:51]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 22:38:52]

Dry run: 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-06-29 23:06:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 23:06:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bajones, 2016-06-30 22:55:04]

Alright, I'm pretty thoroughly stuck on this one now.

I haven't been able to repro the compile errors shown on the mac and windows
builds locally no matter what I try, on either GN or GYP builds, and I can't see
why those dependencies aren't linking by simply inspecting the files. I'd
appreciate any hints the reviewers might have on how to diagnose the problem?

[piman, 2016-06-30 23:22:33]

On 2016/06/30 22:55:04, bajones wrote:
> Alright, I'm pretty thoroughly stuck on this one now.
> 
> I haven't been able to repro the compile errors shown on the mac and windows
> builds locally no matter what I try, on either GN or GYP builds, and I can't
see
> why those dependencies aren't linking by simply inspecting the files. I'd
> appreciate any hints the reviewers might have on how to diagnose the problem?

For the windows side, try adding an empty virtual destructor to GamepadConsumer.
I suspect the error only happens when building in component mode.
I have no idea about the mac side.

[denniskempin, 2016-07-01 17:26:55]

On 2016/06/30 23:22:33, piman OOO back 2016-7-6 wrote:
> On 2016/06/30 22:55:04, bajones wrote:
> > Alright, I'm pretty thoroughly stuck on this one now.
> > 
> > I haven't been able to repro the compile errors shown on the mac and windows
> > builds locally no matter what I try, on either GN or GYP builds, and I can't
> see
> > why those dependencies aren't linking by simply inspecting the files. I'd
> > appreciate any hints the reviewers might have on how to diagnose the
problem?
> 
> For the windows side, try adding an empty virtual destructor to
GamepadConsumer.
> I suspect the error only happens when building in component mode.
> I have no idea about the mac side.

For the mac errors:

it looks like it's missing some osx standard libraries. I checked the build file
for content/browser and it adds these libraries:

    libs += [
      "Carbon.framework",
      "QuartzCore.framework",
      "IOSurface.framework",
    ]

Maybe it's missing one of those?

[denniskempin, 2016-07-01 17:29:46]

On 2016/07/01 17:26:55, denniskempin wrote:
> On 2016/06/30 23:22:33, piman OOO back 2016-7-6 wrote:
> > On 2016/06/30 22:55:04, bajones wrote:
> > > Alright, I'm pretty thoroughly stuck on this one now.
> > > 
> > > I haven't been able to repro the compile errors shown on the mac and
windows
> > > builds locally no matter what I try, on either GN or GYP builds, and I
can't
> > see
> > > why those dependencies aren't linking by simply inspecting the files. I'd
> > > appreciate any hints the reviewers might have on how to diagnose the
> problem?
> > 
> > For the windows side, try adding an empty virtual destructor to
> GamepadConsumer.
> > I suspect the error only happens when building in component mode.
> > I have no idea about the mac side.
> 
> For the mac errors:
> 
> it looks like it's missing some osx standard libraries. I checked the build
file
> for content/browser and it adds these libraries:
> 
>     libs += [
>       "Carbon.framework",
>       "QuartzCore.framework",
>       "IOSurface.framework",
>     ]
> 
> Maybe it's missing one of those?

adlr@ also found this in base/BUILD.gn:
https://cs.chromium.org/chromium/src/base/BUILD.gn?rcl=0&l=1280

I think it's missing IOKit and CoreFoundation

[Robert Sesek, 2016-07-01 17:40:16]

rsesek@chromium.org changed reviewers:
+ rsesek@chromium.org

[Robert Sesek, 2016-07-01 17:40:18]

https://codereview.chromium.org/2081583002/diff/260001/device/gamepad/BUILD.gn
File device/gamepad/BUILD.gn (right):

https://codereview.chromium.org/2081583002/diff/260001/device/gamepad/BUILD.g...
device/gamepad/BUILD.gn:65: }
To fix the Mac errors, you need:

if (is_mac) {
  libs = [
    "CoreFoundation.framework",
    "Foundation.framework",
    "IOKit.framework",
  ]
}

[bajones, 2016-07-01 18:20:03]

Thanks for the assistance everyone! Trying out the updated patch now!

[bajones, 2016-07-01 18:20:22]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-01 18:20:41]

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

[bajones, 2016-07-01 19:30:42]

Looks like this version is gonna actually pass the CQ dry run, so can I ask that
all my very patient reviewers come back and give it one more pass? Thanks!

[commit-bot: I haz the power, 2016-07-01 19:32:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-01 19:32:50]

Dry run: This issue passed the CQ dry run.

[scottmg, 2016-07-01 22:25:39]

lgtm

https://codereview.chromium.org/2081583002/diff/280001/content/browser/gamepa...
File content/browser/gamepad/gamepad_service_test_helpers.h (right):

https://codereview.chromium.org/2081583002/diff/280001/content/browser/gamepa...
content/browser/gamepad/gamepad_service_test_helpers.h:5: #ifndef
CONTENT_BROWSER_GAMEPAD_GAMEPAD_TEST_HELPERS_H_
Match up the guard with the filename.

https://codereview.chromium.org/2081583002/diff/280001/content/browser/gamepa...
File content/browser/gamepad/gamepad_shared_buffer_impl.cc (right):

https://codereview.chromium.org/2081583002/diff/280001/content/browser/gamepa...
content/browser/gamepad/gamepad_shared_buffer_impl.cc:18: // Placement new
This is a weird comment (unnecessary I think).

https://codereview.chromium.org/2081583002/diff/280001/content/browser/gamepa...
content/browser/gamepad/gamepad_shared_buffer_impl.cc:21:
//GamepadHardwareBuffer* hwbuf = hardware_buffer();
Remove these lines.

https://codereview.chromium.org/2081583002/diff/280001/content/browser/gamepa...
content/browser/gamepad/gamepad_shared_buffer_impl.cc:26: 
nit; remove empty line

https://codereview.chromium.org/2081583002/diff/280001/device/gamepad/gamepad...
File device/gamepad/gamepad_shared_buffer.h (right):

https://codereview.chromium.org/2081583002/diff/280001/device/gamepad/gamepad...
device/gamepad/gamepad_shared_buffer.h:16: virtual ~GamepadSharedBuffer(){};
no ; and this looks non-format'd.

[bajones, 2016-07-01 22:39:11]

On 2016/07/01 22:25:39, scottmg (OOO until August) wrote:

Thanks! Done, done, and done.

[bajones, 2016-07-01 22:49:54]

bajones@chromium.org changed reviewers:
+ jochen@chromium.org

[bajones, 2016-07-01 22:49:55]

rockot@: Could I get a follow up review on device/?

jochen@: Since it seems that piman@ is out for a bit, could you review the
changes to content/ (mostly removals) that scottmg@'s review didn't cover?

[jochen (gone - plz use gerrit), 2016-07-05 13:49:07]

lgtm

https://codereview.chromium.org/2081583002/diff/280001/content/browser/gamepa...
File content/browser/gamepad/gamepad_shared_buffer_impl.h (right):

https://codereview.chromium.org/2081583002/diff/280001/content/browser/gamepa...
content/browser/gamepad/gamepad_shared_buffer_impl.h:29: };
disallow copy/assign

[Ken Rockot(use gerrit already), 2016-07-05 13:59:42]

Sorry for the delay - device lgtm

[bajones, 2016-07-06 16:58:49]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-07-06 16:58:50]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jochen@chromium.org, scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2081583002/#ps300001
(title: "Final tweaks")

[commit-bot: I haz the power, 2016-07-06 16:59:33]

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-06 18:55:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-06 18:55:50]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bajones, 2016-07-06 18:58:46]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2016-07-06 18:59:15]

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-06 22:50:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-06 22:50:36]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bajones, 2016-07-06 23:06:30]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2016-07-06 23:07:12]

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-07 00:11:25]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-07-07 00:11:45]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 00:13:13]

Description was changed from

==========
Migrating majority of gamepad from content/browser/ to device/

BUG=612330
==========

to

==========
Migrating majority of gamepad from content/browser/ to device/

BUG=612330

Committed: https://crrev.com/b9bb4c42d6b19a7d7131af942d3769c54e89981c
Cr-Commit-Position: refs/heads/master@{#403998}
==========

[commit-bot: I haz the power, 2016-07-07 00:13:15]

Patchset 16 (id:??) landed as
https://crrev.com/b9bb4c42d6b19a7d7131af942d3769c54e89981c
Cr-Commit-Position: refs/heads/master@{#403998}