Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(37)

Issue 12374068: Make it possible to disable udev in linux (Closed)

Created:
7 years, 9 months ago by Mostyn Bramley-Moore
Modified:
7 years, 1 month ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, rogerj
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Make it possible to disable udev in the content API on linux This is useful for embedded linux setups, which often don't include udev support. TEST=Build content_shell on linux with use_udev=0 then run ldd on output binaries to verify libudev is not listed (and is listed if built with use_udev=1 or unspecified) BUG=318315, 318413 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236204

Patch Set 1 #

Patch Set 2 : move use_udev setting to build/common.gypi #

Total comments: 2

Patch Set 3 : move USE_UDEV define to a linux-specific spot #

Total comments: 2

Patch Set 4 : dust off and rebase on master #

Total comments: 4

Patch Set 5 : review fixups #

Total comments: 6

Patch Set 6 : fix jam's nits #

Total comments: 4

Patch Set 7 : rename device_monitor_linux.* to device_monitor_udev.* #

Total comments: 4

Patch Set 8 : update guard #

Patch Set 9 : clarify use_udev scope #

Total comments: 3

Patch Set 10 : unbreak mac #

Unified diffs Side-by-side diffs Delta from patch set Stats (+36 lines, -144 lines) Patch
M build/common.gypi View 1 2 3 4 5 6 7 8 3 chunks +11 lines, -0 lines 0 comments Download
M content/browser/browser_main_loop.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M content/browser/browser_main_loop.cc View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
M content/browser/device_monitor_linux.h View 1 2 3 4 5 6 1 chunk +0 lines, -44 lines 0 comments Download
D content/browser/device_monitor_linux.cc View 1 2 3 4 5 6 1 chunk +0 lines, -87 lines 0 comments Download
A + content/browser/device_monitor_udev.h View 1 2 3 4 5 6 7 2 chunks +3 lines, -3 lines 0 comments Download
A + content/browser/device_monitor_udev.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M content/browser/gamepad/gamepad_platform_data_fetcher.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 3 chunks +16 lines, -4 lines 0 comments Download

Messages

Total messages: 41 (0 generated)
Mostyn Bramley-Moore
This patch makes it possible to build for linux systems without udev (eg embedded systems).
7 years, 9 months ago (2013-03-02 21:17:13 UTC) #1
Lei Zhang
Can you put the use_udev gyp variable in build/common.gypi instead?
7 years, 9 months ago (2013-03-04 20:35:01 UTC) #2
Mostyn Bramley-Moore
If you prefer- done in the second patch set.
7 years, 9 months ago (2013-03-04 21:00:44 UTC) #3
Jorge Lucangeli Obes
lgtm but wait for Lei.
7 years, 9 months ago (2013-03-04 21:06:08 UTC) #4
Lei Zhang
There's more udev usage in chrome. If we are going to disable udev, we probably ...
7 years, 9 months ago (2013-03-04 21:10:25 UTC) #5
Mostyn Bramley-Moore
https://codereview.chromium.org/12374068/diff/7003/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/7003/content/content_browser.gypi#newcode1157 content/content_browser.gypi:1157: 'defines': [ On 2013/03/04 21:10:25, Lei Zhang wrote: > ...
7 years, 9 months ago (2013-03-05 00:02:33 UTC) #6
Mostyn Bramley-Moore
On 2013/03/04 21:10:25, Lei Zhang wrote: > There's more udev usage in chrome. If we ...
7 years, 9 months ago (2013-03-05 00:05:23 UTC) #7
Lei Zhang
+jam since we need a content/ OWNER anyway. Do you have an opinion on content-only ...
7 years, 9 months ago (2013-03-05 00:27:24 UTC) #8
jam
On 2013/03/05 00:27:24, Lei Zhang wrote: > +jam since we need a content/ OWNER anyway. ...
7 years, 9 months ago (2013-03-05 17:48:41 UTC) #9
Mostyn Bramley-Moore
@rjkroege: is this relevant to ozone embedded content_shell / crbug.com/318315 ? If so, I will ...
7 years, 1 month ago (2013-11-13 23:03:22 UTC) #10
spang
On 2013/11/13 23:03:22, Mostyn Bramley-Moore wrote: > @rjkroege: is this relevant to ozone embedded content_shell ...
7 years, 1 month ago (2013-11-13 23:45:09 UTC) #11
Mostyn Bramley-Moore
Rebased and retested, this still seems to work. https://codereview.chromium.org/12374068/diff/3004/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/3004/content/content_browser.gypi#newcode1147 content/content_browser.gypi:1147: ['OS=="linux" ...
7 years, 1 month ago (2013-11-14 00:26:32 UTC) #12
Lei Zhang
content/browser/device_monitor_linux.h can use a defensive check: #if !defined(USE_UDEV) #error USE_UDEV must be defined. #endif https://codereview.chromium.org/12374068/diff/34001/content/content_browser.gypi ...
7 years, 1 month ago (2013-11-14 00:35:00 UTC) #13
Mostyn Bramley-Moore
Done. https://codereview.chromium.org/12374068/diff/34001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/34001/content/content_browser.gypi#newcode1413 content/content_browser.gypi:1413: 'browser/device_monitor_linux.cc', On 2013/11/14 00:35:00, Lei Zhang wrote: > ...
7 years, 1 month ago (2013-11-14 01:15:37 UTC) #14
Lei Zhang
lgtm, but you probably still need an OWNER
7 years, 1 month ago (2013-11-14 01:28:37 UTC) #15
Mostyn Bramley-Moore
On 2013/11/14 01:28:37, Lei Zhang wrote: > lgtm, but you probably still need an OWNER ...
7 years, 1 month ago (2013-11-14 07:50:37 UTC) #16
Mostyn Bramley-Moore
On 2013/11/14 07:50:37, Mostyn Bramley-Moore wrote: > On 2013/11/14 01:28:37, Lei Zhang wrote: > > ...
7 years, 1 month ago (2013-11-14 07:56:36 UTC) #17
rjkroege
https://codereview.chromium.org/12374068/diff/94001/content/browser/device_monitor_linux.h File content/browser/device_monitor_linux.h (right): https://codereview.chromium.org/12374068/diff/94001/content/browser/device_monitor_linux.h#newcode13 content/browser/device_monitor_linux.h:13: #endif this doesn't seem very pretty to me Given ...
7 years, 1 month ago (2013-11-14 15:44:22 UTC) #18
Mostyn Bramley-Moore
https://codereview.chromium.org/12374068/diff/94001/content/browser/device_monitor_linux.h File content/browser/device_monitor_linux.h (right): https://codereview.chromium.org/12374068/diff/94001/content/browser/device_monitor_linux.h#newcode13 content/browser/device_monitor_linux.h:13: #endif On 2013/11/14 15:44:23, rjkroege wrote: > this doesn't ...
7 years, 1 month ago (2013-11-14 16:42:01 UTC) #19
jam
https://codereview.chromium.org/12374068/diff/94001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/94001/content/content_browser.gypi#newcode1408 content/content_browser.gypi:1408: 'USE_UDEV', nit: the convention is that this is done ...
7 years, 1 month ago (2013-11-14 17:37:28 UTC) #20
Mostyn Bramley-Moore
https://codereview.chromium.org/12374068/diff/94001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/94001/content/content_browser.gypi#newcode1408 content/content_browser.gypi:1408: 'USE_UDEV', On 2013/11/14 17:37:29, jam wrote: > nit: the ...
7 years, 1 month ago (2013-11-14 19:58:21 UTC) #21
jam
lgtm
7 years, 1 month ago (2013-11-15 17:36:54 UTC) #22
Mostyn Bramley-Moore
On 2013/11/14 16:42:01, Mostyn Bramley-Moore wrote: > https://codereview.chromium.org/12374068/diff/94001/content/browser/device_monitor_linux.h > File content/browser/device_monitor_linux.h (right): > > https://codereview.chromium.org/12374068/diff/94001/content/browser/device_monitor_linux.h#newcode13 ...
7 years, 1 month ago (2013-11-15 20:44:14 UTC) #23
wjia(left Chromium)
lgtm
7 years, 1 month ago (2013-11-15 23:06:13 UTC) #24
rjkroege
https://codereview.chromium.org/12374068/diff/214001/content/browser/browser_main_loop.h File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/12374068/diff/214001/content/browser/browser_main_loop.h#newcode144 content/browser/browser_main_loop.h:144: #elif defined(OS_LINUX) && defined(USE_UDEV) isn't the defined(OS_LINUX) superfluous here ...
7 years, 1 month ago (2013-11-15 23:16:31 UTC) #25
Mostyn Bramley-Moore
https://codereview.chromium.org/12374068/diff/214001/content/browser/browser_main_loop.h File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/12374068/diff/214001/content/browser/browser_main_loop.h#newcode144 content/browser/browser_main_loop.h:144: #elif defined(OS_LINUX) && defined(USE_UDEV) On 2013/11/15 23:16:31, rjkroege wrote: ...
7 years, 1 month ago (2013-11-15 23:49:42 UTC) #26
rjkroege
https://codereview.chromium.org/12374068/diff/324001/content/browser/device_monitor_udev.h File content/browser/device_monitor_udev.h (right): https://codereview.chromium.org/12374068/diff/324001/content/browser/device_monitor_udev.h#newcode8 content/browser/device_monitor_udev.h:8: #ifndef CONTENT_BROWSER_DEVICE_MONITOR_LINUX_H_ you need to change this and the ...
7 years, 1 month ago (2013-11-18 22:32:18 UTC) #27
Mostyn Bramley-Moore
https://codereview.chromium.org/12374068/diff/324001/content/browser/device_monitor_udev.h File content/browser/device_monitor_udev.h (right): https://codereview.chromium.org/12374068/diff/324001/content/browser/device_monitor_udev.h#newcode8 content/browser/device_monitor_udev.h:8: #ifndef CONTENT_BROWSER_DEVICE_MONITOR_LINUX_H_ On 2013/11/18 22:32:19, rjkroege wrote: > you ...
7 years, 1 month ago (2013-11-18 23:32:25 UTC) #28
spang
On 2013/11/18 23:32:25, Mostyn Bramley-Moore wrote: > https://codereview.chromium.org/12374068/diff/324001/content/browser/device_monitor_udev.h > File content/browser/device_monitor_udev.h (right): > > https://codereview.chromium.org/12374068/diff/324001/content/browser/device_monitor_udev.h#newcode8 ...
7 years, 1 month ago (2013-11-19 16:28:22 UTC) #29
Mostyn Bramley-Moore
On 2013/11/19 16:28:22, spang wrote: > On 2013/11/18 23:32:25, Mostyn Bramley-Moore wrote: > > > ...
7 years, 1 month ago (2013-11-19 17:15:45 UTC) #30
spang
On 2013/11/19 17:15:45, Mostyn Bramley-Moore wrote: > On 2013/11/19 16:28:22, spang wrote: > > On ...
7 years, 1 month ago (2013-11-19 17:40:24 UTC) #31
Mostyn Bramley-Moore
> > The product I'm working on is only interested in the content API, so ...
7 years, 1 month ago (2013-11-19 17:47:08 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/12374068/484001
7 years, 1 month ago (2013-11-19 17:47:56 UTC) #33
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=190194
7 years, 1 month ago (2013-11-19 18:06:59 UTC) #34
spang
https://codereview.chromium.org/12374068/diff/484001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/484001/content/content_browser.gypi#newcode1264 content/content_browser.gypi:1264: ['OS!="linux" or use_udev==0', { Oops, you broke mac here.
7 years, 1 month ago (2013-11-19 18:19:34 UTC) #35
Mostyn Bramley-Moore
https://codereview.chromium.org/12374068/diff/484001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/484001/content/content_browser.gypi#newcode1264 content/content_browser.gypi:1264: ['OS!="linux" or use_udev==0', { On 2013/11/19 18:19:35, spang wrote: ...
7 years, 1 month ago (2013-11-19 18:28:35 UTC) #36
spang
https://codereview.chromium.org/12374068/diff/484001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/484001/content/content_browser.gypi#newcode1264 content/content_browser.gypi:1264: ['OS!="linux" or use_udev==0', { On 2013/11/19 18:28:36, Mostyn Bramley-Moore ...
7 years, 1 month ago (2013-11-19 18:36:07 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/12374068/744002
7 years, 1 month ago (2013-11-19 18:39:21 UTC) #38
commit-bot: I haz the power
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=176388
7 years, 1 month ago (2013-11-20 02:21:16 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/12374068/744002
7 years, 1 month ago (2013-11-20 08:18:53 UTC) #40
commit-bot: I haz the power
7 years, 1 month ago (2013-11-20 08:36:43 UTC) #41
Message was sent while issue was closed.
Change committed as 236204

Powered by Google App Engine
This is Rietveld 408576698