Add gamepad data fetcher for Linux

Add gamepad data fetcher for Linux

BUG=79050
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116724

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116752

[scottmg, 2011-12-15 03:25:21]

Not sure if there's a preferred reviewer for content/browser for linux-specific
stuff.

The addition to src/third_party can be ignored, waiting on o-s-t-p-r here:
http://codereview.chromium.org/8897018/

[darin (slow to review), 2011-12-15 05:18:48]

On Wed, Dec 14, 2011 at 7:25 PM, <scottmg@chromium.org> wrote:

> Reviewers: darin,
>
> Message:
> Not sure if there's a preferred reviewer for content/browser for
> linux-specific
> stuff.
>

^^^ Probably Elliot


>
> Please review this at
http://codereview.chromium.**org/8899017/<http://codereview.chromium.org/8899...
>

[darin (slow to review), 2011-12-15 05:22:23]

http://codereview.chromium.org/8899017/diff/4012/content/browser/gamepad/game...
File content/browser/gamepad/gamepad_provider.cc (right):

http://codereview.chromium.org/8899017/diff/4012/content/browser/gamepad/game...
content/browser/gamepad/gamepad_provider.cc:20: #if defined(OS_WIN)
how about putting all of this gunk into a data_fetcher.h file?

http://codereview.chromium.org/8899017/diff/4012/content/browser/gamepad/game...
content/browser/gamepad/gamepad_provider.cc:34: typedef
GamepadDataFetcherWindows GamepadPlatformDataFetcher;
why is the typedef from FooBarWindows to FooPlatformBar?  is the "Platform" bit
helpful?  if so, shouldn't it be in the platform-specific names too?

[Elliot Glaysher, 2011-12-15 19:16:43]

Pawel, what do you think of the third part include here? Will this be a problem
for gentoo?

[scottmg, 2011-12-15 19:30:49]

On 2011/12/15 05:22:23, darin wrote:
>
http://codereview.chromium.org/8899017/diff/4012/content/browser/gamepad/game...
> File content/browser/gamepad/gamepad_provider.cc (right):
> 
>
http://codereview.chromium.org/8899017/diff/4012/content/browser/gamepad/game...
> content/browser/gamepad/gamepad_provider.cc:20: #if defined(OS_WIN)
> how about putting all of this gunk into a data_fetcher.h file?

Sure. data_fetcher.h is the interface base class, maybe platform_data_fetcher.h?

>
http://codereview.chromium.org/8899017/diff/4012/content/browser/gamepad/game...
> content/browser/gamepad/gamepad_provider.cc:34: typedef
> GamepadDataFetcherWindows GamepadPlatformDataFetcher;
> why is the typedef from FooBarWindows to FooPlatformBar?  is the "Platform"
bit
> helpful?  if so, shouldn't it be in the platform-specific names too?

hmm, Gamepad (Foo) is mostly a "namespace". So I was viewing it as being
remapped from BarWindows/BarMac to PlatformBar because BarPlatform sounded
awkward (the data fetchers for various platforms, and then a typedef to a
specific platform). so, ... I guess it could be
GamepadPlatformDataFetcherWindows, it just seemed a bit unwieldy.

[scottmg, 2011-12-15 19:32:27]

Gah! The diff doesn't track renames, sorry. There are no substantial changes in
any of the non-linux code here, just renames.

[Paweł Hajdan Jr., 2011-12-16 12:47:09]

On 2011/12/15 19:16:43, Elliot Glaysher wrote:
> Pawel, what do you think of the third part include here? Will this be a
problem
> for gentoo?

Thank you for adding me. Note that the issues here are not Gentoo-specific, just
apply to any distro that recompiles the code on Linux (Debian, Fedora, OpenSUSE,
Arch, Gentoo, and so on).

Could you please provide an option to use system libudev, which means using
system header and not the bundled one, and linking directly (linker -l flag, no
dlsym) to the library at compile time?

Here are some general guidelines and examples you can use:

- search for linux_link_gnome_keyring in gyp files, and do something similar
here (also search for DLOPEN_GNOME_KEYRING in the code); I don't remember _any_
troubles with that code since it has been introduced loooong ago, so don't worry
about maintenance. Distros can understand those switches might break once in a
while and are not tested by Google. That's fine.

- search for use_system_speex for a good example how to handle header files

And of course I can answer any remaining questions.

By the way, feel free to do the above as a _timely_ follow-up after committing
this CL.

[scottmg, 2011-12-16 18:44:22]

OK, thanks. If I'm going to have that as an option, I should also take a
step back here and confirm that I really need to dlopen libudev at all.

I did that because libjingle (part of the talk plugin) does so, and at
least by default I don't have libudev-dev installed. Getting everyone to
install it is annoying, but I guess a relatively minor hassle.

Is there a guideline written or otherwise for when it's OK to add a hard
dependency. (Elliot?) http://en.wikipedia.org/wiki/Udev

On Fri, Dec 16, 2011 at 4:47 AM, <phajdan.jr@chromium.org> wrote:

> On 2011/12/15 19:16:43, Elliot Glaysher wrote:
>
>> Pawel, what do you think of the third part include here? Will this be a
>>
> problem
>
>> for gentoo?
>>
>
> Thank you for adding me. Note that the issues here are not
> Gentoo-specific, just
> apply to any distro that recompiles the code on Linux (Debian, Fedora,
> OpenSUSE,
> Arch, Gentoo, and so on).
>
> Could you please provide an option to use system libudev, which means using
> system header and not the bundled one, and linking directly (linker -l
> flag, no
> dlsym) to the library at compile time?
>
> Here are some general guidelines and examples you can use:
>
> - search for linux_link_gnome_keyring in gyp files, and do something
> similar
> here (also search for DLOPEN_GNOME_KEYRING in the code); I don't remember
> _any_
> troubles with that code since it has been introduced loooong ago, so don't
> worry
> about maintenance. Distros can understand those switches might break once
> in a
> while and are not tested by Google. That's fine.
>
> - search for use_system_speex for a good example how to handle header files
>
> And of course I can answer any remaining questions.
>
> By the way, feel free to do the above as a _timely_ follow-up after
> committing
> this CL.
>
>
http://codereview.chromium.**org/8899017/<http://codereview.chromium.org/8899...
>

[Elliot Glaysher, 2011-12-16 18:54:36]

Thoughts:

- Add libudev-dev to the list of packages to install in
build/install-build-deps.sh
- I'm not sure you do need to dlopen udev. We dlopen, for example, libunity
because that's an integration library for a single desktop environment on a
single distribution where our Debian packages need to install on both
Debian/Ubuntu/other Debian derivatives. I *think* (but am not sure) that udev is
installed on all distributions we want to support.

[scottmg, 2011-12-16 19:07:30]

On Fri, Dec 16, 2011 at 10:54 AM, <erg@chromium.org> wrote:

> Thoughts:
>
> - Add libudev-dev to the list of packages to install in
> build/install-build-deps.sh
> - I'm not sure you do need to dlopen udev. We dlopen, for example, libunity
> because that's an integration library for a single desktop environment on a
> single distribution where our Debian packages need to install on both
> Debian/Ubuntu/other Debian derivatives. I *think* (but am not sure) that
> udev is
> installed on all distributions we want to support.
>
>
Based cursory searches for "$distroname libudev" from distronames taken
from here http://code.google.com/p/chromium/wiki/LinuxChromiumPackages it
seems that all have libudev available. I'm not sure about the version
requirements or how far back we try to support though.

So, I'm going to rework this to go with a static dependency, and can add
the dlopen option if someone gets upset.

[scottmg, 2011-12-16 23:40:29]

ptal

Elliot, could you specifically look at platform_data_fetcher_linux.cc/h for
linux functionality?

The rest is renames and some relatively trivial code (in
gamepad_standard_mappings_linux).

linux try won't succeed until bots are updated.

[Elliot Glaysher, 2011-12-16 23:53:27]

On 2011/12/16 23:40:29, scottmg wrote:
> ptal
> 
> Elliot, could you specifically look at platform_data_fetcher_linux.cc/h for
> linux functionality?
> 
> The rest is renames and some relatively trivial code (in
> gamepad_standard_mappings_linux).
> 
> linux try won't succeed until bots are updated.

I read through platform_data_fetcher_linux.cc. I barely understand it. LGTM.
Break trunk and iterate. You probably also want to get Pawel's approval.

(Also, I will be gone starting Monday for December holidays.)

[scottmg, 2011-12-16 23:59:00]

On 2011/12/16 23:53:27, Elliot Glaysher wrote:

Thanks for a somewhat-sane check at least Elliot. :)

Paweł, does the build infrastructure part look OK to you now?

[Ryan Sleevi, 2011-12-17 04:31:14]

Just a few drive-by comments and style nits. Please don't let them hold up the
review.

However, two of the comments may be bugs (annotated BUG? below), so please check
those out and see if there is anything to be concerned about.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
File content/browser/gamepad/platform_data_fetcher_linux.cc (right):

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:12: #include <dlfcn.h>
style nit: C headers before Chromium headers

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:51: bool is =
strncmp(kJoystickRoot, node_path, sizeof(kJoystickRoot) - 1) == 0;
nit: is -> is_gamepad ?

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:53: index =
node_path[sizeof(kJoystickRoot) - 1] - '0';
BUG? It seems possible for there to be a /dev/input/js10, /dev/input/js11 or
more (at least, judging from quickly Google searching)

What impact, if any, will that have on this code? 10 == 1 seems like it could
cause trouble?

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:85: device_fd =
open(node_path.c_str(), O_RDONLY | O_NONBLOCK);
Normally these calls are wrapped in the HANDLE_EINTR macro (at least, liberally
throughout base)

Does that need to be something you do here as well?

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:101: // to WebUChar to
build the id string.
BUG? Because they're UTF-8 strings, what risk is there, if any, if the strings
are overlong and thus split on an invalid sequence?

Also, is idLengthCap the length of characters or bytes (my guess, characters?)
Would seem to matter here as well.

Perhaps you should use base::StringPrintF to take in the whole string as UTF-8,
then run through UTF8toUTF16 to convert to WebUChar/UTF-16, and then split as
appropriate.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:146:
GamepadPlatformDataFetcherLinux::~GamepadPlatformDataFetcherLinux() {
style nit: There was a big clean-up effort to have all of the .cc files match
the declaration order as in the header files (yay clang!). 

It would be nice as a style nit to be able to correct that here and elsewhere in
this CL.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:166: if
(select(monitor_fd_ + 1, &fds, NULL, NULL, &tv) > 0
HANDLE_EINTR(select( ?

Is there a reason not to use base::MessagePumpLibEvent::FileDescriptorWatcher?
Certainly seems more Chromium-y (at least, excluding CrOS-specific code)

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:182: while (read(fd,
&event, sizeof(struct js_event)) > 0) {
HANDLE_EINTR(read( ?

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
File content/browser/gamepad/platform_data_fetcher_linux.h (right):

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.h:15: #include <string>
random drive-by style nit: #include <string> before the Chromium headers, and
you don't need to treat build_config.h as a special header

Ordering:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

build_config.h is not special:
http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_frm/thread...

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.h:27: virtual void
GetGamepadData(WebKit::WebGamepads* pads,
style nit: Declare DTORs before methods

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Also, see the Chromium style guide rules for virtual interfaces
http://www.chromium.org/developers/coding-style#TOC-Code-formatting

When you implement an interface, group those functions in your header file in
one section, clearly labeled as implementing that interface. Be sure to preserve
the order of the virtual base class, and do not add comments or whitespace
between them, as the reader can refer to the virtual base class for more
information. If you have implementation notes, put them in the implementation of
the function in your .cc file.  Use the OVERRIDE specifier (see details below).

     // RenderViewHost::RendererManagement:
     virtual void ShouldClosePage(bool for_cross_site_transition,
                                  bool proceed) OVERRIDE;

[Paweł Hajdan Jr., 2011-12-19 08:07:12]

LGTM, thank you!

This is done _exactly_ the right way, I appreciate that.

[scottmg, 2011-12-19 19:41:44]

Thanks Ryan.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
File content/browser/gamepad/platform_data_fetcher_linux.cc (right):

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:12: #include <dlfcn.h>
On 2011/12/17 04:31:14, Ryan Sleevi wrote:
> style nit: C headers before Chromium headers

Done.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:51: bool is =
strncmp(kJoystickRoot, node_path, sizeof(kJoystickRoot) - 1) == 0;
On 2011/12/17 04:31:14, Ryan Sleevi wrote:
> nit: is -> is_gamepad ?

Done.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:53: index =
node_path[sizeof(kJoystickRoot) - 1] - '0';
On 2011/12/17 04:31:14, Ryan Sleevi wrote:
> BUG? It seems possible for there to be a /dev/input/js10, /dev/input/js11 or
> more (at least, judging from quickly Google searching)
> 
> What impact, if any, will that have on this code? 10 == 1 seems like it could
> cause trouble?

Done.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:85: device_fd =
open(node_path.c_str(), O_RDONLY | O_NONBLOCK);
On 2011/12/17 04:31:14, Ryan Sleevi wrote:
> Normally these calls are wrapped in the HANDLE_EINTR macro (at least,
liberally
> throughout base)
> 
> Does that need to be something you do here as well?

Done.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:101: // to WebUChar to
build the id string.
On 2011/12/17 04:31:14, Ryan Sleevi wrote:
> BUG? Because they're UTF-8 strings, what risk is there, if any, if the strings
> are overlong and thus split on an invalid sequence?
> 
> Also, is idLengthCap the length of characters or bytes (my guess, characters?)
> Would seem to matter here as well.
> 
> Perhaps you should use base::StringPrintF to take in the whole string as
UTF-8,
> then run through UTF8toUTF16 to convert to WebUChar/UTF-16, and then split as
> appropriate.

Done. There's no Truncate for utf16 in base so I (conservatively) truncated the
utf-8 version.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:146:
GamepadPlatformDataFetcherLinux::~GamepadPlatformDataFetcherLinux() {
On 2011/12/17 04:31:14, Ryan Sleevi wrote:
> style nit: There was a big clean-up effort to have all of the .cc files match
> the declaration order as in the header files (yay clang!). 
> 
> It would be nice as a style nit to be able to correct that here and elsewhere
in
> this CL.

Done.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:166: if
(select(monitor_fd_ + 1, &fds, NULL, NULL, &tv) > 0
On 2011/12/17 04:31:14, Ryan Sleevi wrote:
> HANDLE_EINTR(select( ?
> 
> Is there a reason not to use base::MessagePumpLibEvent::FileDescriptorWatcher?
> Certainly seems more Chromium-y (at least, excluding CrOS-specific code)

Done.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:182: while (read(fd,
&event, sizeof(struct js_event)) > 0) {
On 2011/12/17 04:31:14, Ryan Sleevi wrote:
> HANDLE_EINTR(read( ?

Done.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
File content/browser/gamepad/platform_data_fetcher_linux.h (right):

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.h:15: #include <string>
On 2011/12/17 04:31:14, Ryan Sleevi wrote:
> random drive-by style nit: #include <string> before the Chromium headers, and
> you don't need to treat build_config.h as a special header
> 
> Ordering:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...
> 
> build_config.h is not special:
> http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code
>
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_frm/thread...

Done.

http://codereview.chromium.org/8899017/diff/21001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.h:27: virtual void
GetGamepadData(WebKit::WebGamepads* pads,
On 2011/12/17 04:31:14, Ryan Sleevi wrote:
> style nit: Declare DTORs before methods
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> 
> Also, see the Chromium style guide rules for virtual interfaces
> http://www.chromium.org/developers/coding-style#TOC-Code-formatting
> 
> When you implement an interface, group those functions in your header file in
> one section, clearly labeled as implementing that interface. Be sure to
preserve
> the order of the virtual base class, and do not add comments or whitespace
> between them, as the reader can refer to the virtual base class for more
> information. If you have implementation notes, put them in the implementation
of
> the function in your .cc file.  Use the OVERRIDE specifier (see details
below).
> 
>      // RenderViewHost::RendererManagement:
>      virtual void ShouldClosePage(bool for_cross_site_transition,
>                                   bool proceed) OVERRIDE;

Done.

[Ryan Sleevi, 2011-12-19 22:51:58]

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
File content/browser/gamepad/platform_data_fetcher_linux.cc (right):

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:82: }
BUG? Did you mean to close() device_fds_ ?

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:99: index =
strtoul(&node_path[sizeof(kJoystickRoot) - 1], NULL, 10);
See base/string_number_conversions.h

It can return a bool for any invalid inputs.

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:131: device_fd =
HANDLE_EINTR(open(node_path.c_str(), O_RDONLY | O_NONBLOCK));
BUG?

If this node path was already opened by a previous call to RefreshDevice, this
means the descriptor is leaked, correct?

Alternatively, if RefreshDevice is called on an existing open connection, line
126 could also leak, correct?

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:163: tmp16.copy(pad.id,
sizeof(pad.id) - 1);
BUG:
tmp16.copy is copying characters, but you're providing byte size
(sizeof(pad.id))

You could use the arraysize() macro here to avoid this. Or use
WebGamepad::idLengthCap - 1

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
File content/browser/gamepad/platform_data_fetcher_linux.h (right):

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.h:40: virtual
~GamepadPlatformDataFetcherLinux();
BUG: Having this method be private means you cannot delete
GamepadPlatformDataFetcherLinux* (more aptly, attempting to delete it
will/should resolve to the public interface from the base class, since a derived
class cannot specify less visibility)

In short - the dtor won't run as expected (if this even compiles). It needs to
be public - especially because it's implementing interfaces.

[scottmg, 2011-12-19 23:23:53]

Thanks a second time, for catching much dumbness Ryan.

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
File content/browser/gamepad/platform_data_fetcher_linux.cc (right):

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:82: }
On 2011/12/19 22:51:58, Ryan Sleevi wrote:
> BUG? Did you mean to close() device_fds_ ?

Done.

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:99: index =
strtoul(&node_path[sizeof(kJoystickRoot) - 1], NULL, 10);
On 2011/12/19 22:51:58, Ryan Sleevi wrote:
> See base/string_number_conversions.h
> 
> It can return a bool for any invalid inputs.

Done.

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:131: device_fd =
HANDLE_EINTR(open(node_path.c_str(), O_RDONLY | O_NONBLOCK));
On 2011/12/19 22:51:58, Ryan Sleevi wrote:
> BUG?
> 
> If this node path was already opened by a previous call to RefreshDevice, this
> means the descriptor is leaked, correct?
> 
> Alternatively, if RefreshDevice is called on an existing open connection, line
> 126 could also leak, correct?

Done.

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:163: tmp16.copy(pad.id,
sizeof(pad.id) - 1);
On 2011/12/19 22:51:58, Ryan Sleevi wrote:
> BUG:
> tmp16.copy is copying characters, but you're providing byte size
> (sizeof(pad.id))
> 
> You could use the arraysize() macro here to avoid this. Or use
> WebGamepad::idLengthCap - 1

Done.

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
File content/browser/gamepad/platform_data_fetcher_linux.h (right):

http://codereview.chromium.org/8899017/diff/26001/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.h:40: virtual
~GamepadPlatformDataFetcherLinux();
On 2011/12/19 22:51:58, Ryan Sleevi wrote:
> BUG: Having this method be private means you cannot delete
> GamepadPlatformDataFetcherLinux* (more aptly, attempting to delete it
> will/should resolve to the public interface from the base class, since a
derived
> class cannot specify less visibility)
> 
> In short - the dtor won't run as expected (if this even compiles). It needs to
> be public - especially because it's implementing interfaces.

Done.

[Ryan Sleevi, 2011-12-19 23:32:58]

LGTM. Obscure trivia below, but no functional impact.

http://codereview.chromium.org/8899017/diff/25004/content/browser/gamepad/pla...
File content/browser/gamepad/platform_data_fetcher_linux.cc (right):

http://codereview.chromium.org/8899017/diff/25004/content/browser/gamepad/pla...
content/browser/gamepad/platform_data_fetcher_linux.cc:49:
HANDLE_EINTR(close(device_fds_[i]));
Obscure POSIX trivia - Turns out there is no need to wrap close() in
HANDLE_EINTR. The semantics of close() are ill-defined on an EINTR return, which
is why no sane implementations return it (
http://www.daemonology.net/blog/2011-12-17-POSIX-close-is-broken.html )

Same for line 125

[scottmg, 2011-12-19 23:47:02]

On 2011/12/19 23:32:58, Ryan Sleevi wrote:
> LGTM. Obscure trivia below, but no functional impact.
> 
> http://www.daemonology.net/blog/2011-12-17-POSIX-close-is-broken.html )
> 
> Same for line 125

Interesting. :) Given that much code seems to do it
http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=hand...
I'll think I'll leave it for consistency (with other code and with other
fd-related calls). Perhaps we should fix that globally if we think it's anything
worse than "unnecessary".

[darin (slow to review), 2012-01-05 20:40:16]

OWNERS LGTM

[commit-bot: I haz the power, 2012-01-06 17:11:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/8899017/32002

[commit-bot: I haz the power, 2012-01-06 17:45:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/8899017/43001

[commit-bot: I haz the power, 2012-01-06 18:25:40]

Try job failure for 8899017-43001 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.