Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7467031:
Add libdbus dependency for Linux. (Closed)
|
Created:
9 years, 5 months ago by satorux1 Modified:
9 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd libdbus dependency for Linux.
This is the first patch for our own D-Bus client library.
The D-Bus client code will be placed under 'dbus' on the top level directory
More patches will follow.
BUG=90036
TEST=try bot to make sure this won't break builds.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93658
Patch Set 1 #Patch Set 2 : moved to the top-level directory #Patch Set 3 : sort lines #
Total comments: 2
Patch Set 4 : revert all.gyp #
Messages
Total messages: 25 (0 generated)
I've heard Evan likes small patches, so I'm sending this out the small patch first. Also Cc'ing willchan as I need approval from someone in net/OWNERS. I hope net/dbus would be the right place for D-Bus client library. This is far from codereview-ready, but the work-in-progress version of the D-Bus client library can be found at: http://codereview.chromium.org/7413004/ (dbus.h/dbus.cc will be split into smaller files).
On 2011/07/21 17:08:41, satorux1 wrote: > I > hope net/dbus would be the right place for D-Bus client library. Hm, I'm not sure. dbus isn't really used for networking, is it? Maybe we use dbus for getting the proxy config? Or is that done in Chrome and then passed down to the network layer?
On 2011/07/21 17:23:01, Evan Martin wrote: > On 2011/07/21 17:08:41, satorux1 wrote: > > I > > hope net/dbus would be the right place for D-Bus client library. > > Hm, I'm not sure. dbus isn't really used for networking, is it? Maybe we use > dbus for getting the proxy config? Or is that done in Chrome and then passed > down to the network layer? D-Bus uses the unix domain socket to do IPC, so I thought 'net' would be appropriate. Otherwise, where would be a better place for this? Currently we use D-Bus for two things for Linux. - for geolocation, talk to NetworkManager to get wireless access points information, - for passowrd manager, talk to kwallet application For Chrome OS, we use D-Bus for many other things, via a library called libcros, which we plan to get rid of.
Is there a reason this goes into the Chromium source tree rather than as an external project pulled in by DEPS? Are the APIs supposed to change that often that we want to minimize the churn by putting this directly in src/? On Thu, Jul 21, 2011 at 8:36 PM, <satorux@chromium.org> wrote: > On 2011/07/21 17:23:01, Evan Martin wrote: >> >> On 2011/07/21 17:08:41, satorux1 wrote: >> > I >> > hope net/dbus would be the right place for D-Bus client library. > >> Hm, I'm not sure. dbus isn't really used for networking, is it? Maybe we >> use >> dbus for getting the proxy config? Or is that done in Chrome and then >> passed >> down to the network layer? > > D-Bus uses the unix domain socket to do IPC, so I thought 'net' would be > appropriate. Otherwise, where would be a better place for this? > > Currently we use D-Bus for two things for Linux. > > - for geolocation, talk to NetworkManager to get wireless access points > information, > - for passowrd manager, talk to kwallet application > > For Chrome OS, we use D-Bus for many other things, via a library called > libcros, > which we plan to get rid of. > > http://codereview.chromium.org/7467031/ >
On 2011/07/21 17:40:42, willchan wrote: > Is there a reason this goes into the Chromium source tree rather than > as an external project pulled in by DEPS? Are the APIs supposed to > change that often that we want to minimize the churn by putting this > directly in src/? The implementation will be using lots of code from 'base' so it would be difficult to have this outside the chrome tree. Besides, we don't plan to use it for anything other than Chrome. We plan to keep improving the D-Bus library actively until it becomes feature complete and very stable. > > On Thu, Jul 21, 2011 at 8:36 PM, <mailto:satorux@chromium.org> wrote: > > On 2011/07/21 17:23:01, Evan Martin wrote: > >> > >> On 2011/07/21 17:08:41, satorux1 wrote: > >> > I > >> > hope net/dbus would be the right place for D-Bus client library. > > > >> Hm, I'm not sure. dbus isn't really used for networking, is it? Maybe we > >> use > >> dbus for getting the proxy config? Or is that done in Chrome and then > >> passed > >> down to the network layer? > > > > D-Bus uses the unix domain socket to do IPC, so I thought 'net' would be > > appropriate. Otherwise, where would be a better place for this? > > > > Currently we use D-Bus for two things for Linux. > > > > - for geolocation, talk to NetworkManager to get wireless access points > > information, > > - for passowrd manager, talk to kwallet application > > > > For Chrome OS, we use D-Bus for many other things, via a library called > > libcros, > > which we plan to get rid of. > > > > http://codereview.chromium.org/7467031/ > >
On Thu, Jul 21, 2011 at 8:45 PM, <satorux@chromium.org> wrote: > On 2011/07/21 17:40:42, willchan wrote: >> >> Is there a reason this goes into the Chromium source tree rather than >> as an external project pulled in by DEPS? Are the APIs supposed to >> change that often that we want to minimize the churn by putting this >> directly in src/? > > The implementation will be using lots of code from 'base' so it would be > difficult to have this outside the chrome tree. Besides, we don't plan to > use it > for anything other than Chrome. We plan to keep improving the D-Bus library > actively until it becomes feature complete and very stable. OIC. That sounds reasonable. I'm not sure I'm fond of having this in net/, although I'm definitely open to this. Is it only used in chrome/ code? If so, can we put it there? > > >> On Thu, Jul 21, 2011 at 8:36 PM, <mailto:satorux@chromium.org> wrote: >> > On 2011/07/21 17:23:01, Evan Martin wrote: >> >> >> >> On 2011/07/21 17:08:41, satorux1 wrote: >> >> > I >> >> > hope net/dbus would be the right place for D-Bus client library. >> > >> >> Hm, I'm not sure. dbus isn't really used for networking, is it? > > Maybe we >> >> >> use >> >> dbus for getting the proxy config? Or is that done in Chrome and >> >> then >> >> passed >> >> down to the network layer? >> > >> > D-Bus uses the unix domain socket to do IPC, so I thought 'net' would be >> > appropriate. Otherwise, where would be a better place for this? >> > >> > Currently we use D-Bus for two things for Linux. >> > >> > - for geolocation, talk to NetworkManager to get wireless access points >> > information, >> > - for passowrd manager, talk to kwallet application >> > >> > For Chrome OS, we use D-Bus for many other things, via a library called >> > libcros, >> > which we plan to get rid of. >> > >> > http://codereview.chromium.org/7467031/ >> > > > > > http://codereview.chromium.org/7467031/ >
On 2011/07/21 18:06:05, willchan wrote: > On Thu, Jul 21, 2011 at 8:45 PM, <mailto:satorux@chromium.org> wrote: > > On 2011/07/21 17:40:42, willchan wrote: > >> > >> Is there a reason this goes into the Chromium source tree rather than > >> as an external project pulled in by DEPS? Are the APIs supposed to > >> change that often that we want to minimize the churn by putting this > >> directly in src/? > > > > The implementation will be using lots of code from 'base' so it would be > > difficult to have this outside the chrome tree. Besides, we don't plan to > > use it > > for anything other than Chrome. We plan to keep improving the D-Bus library > > actively until it becomes feature complete and very stable. > > OIC. That sounds reasonable. I'm not sure I'm fond of having this in > net/, although I'm definitely open to this. Is it only used in chrome/ > code? If so, can we put it there? Well, we have clients in content and chrome: content/browser/geolocation/wifi_data_provider_linux.cc chrome/browser/password_manager/native_backend_kwallet_x.cc > > > > > > >> On Thu, Jul 21, 2011 at 8:36 PM, <mailto:satorux@chromium.org> wrote: > >> > On 2011/07/21 17:23:01, Evan Martin wrote: > >> >> > >> >> On 2011/07/21 17:08:41, satorux1 wrote: > >> >> > I > >> >> > hope net/dbus would be the right place for D-Bus client library. > >> > > >> >> Hm, I'm not sure. dbus isn't really used for networking, is it? > > > > Maybe we > >> > >> >> use > >> >> dbus for getting the proxy config? Or is that done in Chrome and > >> >> then > >> >> passed > >> >> down to the network layer? > >> > > >> > D-Bus uses the unix domain socket to do IPC, so I thought 'net' would be > >> > appropriate. Otherwise, where would be a better place for this? > >> > > >> > Currently we use D-Bus for two things for Linux. > >> > > >> > - for geolocation, talk to NetworkManager to get wireless access points > >> > information, > >> > - for passowrd manager, talk to kwallet application > >> > > >> > For Chrome OS, we use D-Bus for many other things, via a library called > >> > libcros, > >> > which we plan to get rid of. > >> > > >> > http://codereview.chromium.org/7467031/ > >> > > > > > > > > > http://codereview.chromium.org/7467031/ > >
On Thu, Jul 21, 2011 at 9:11 PM, <satorux@chromium.org> wrote: > On 2011/07/21 18:06:05, willchan wrote: >> >> On Thu, Jul 21, 2011 at 8:45 PM, <mailto:satorux@chromium.org> wrote: >> > On 2011/07/21 17:40:42, willchan wrote: >> >> >> >> Is there a reason this goes into the Chromium source tree rather than >> >> as an external project pulled in by DEPS? Are the APIs supposed to >> >> change that often that we want to minimize the churn by putting this >> >> directly in src/? >> > >> > The implementation will be using lots of code from 'base' so it would be >> > difficult to have this outside the chrome tree. Besides, we don't plan >> > to >> > use it >> > for anything other than Chrome. We plan to keep improving the D-Bus >> > library >> > actively until it becomes feature complete and very stable. > >> OIC. That sounds reasonable. I'm not sure I'm fond of having this in >> net/, although I'm definitely open to this. Is it only used in chrome/ >> code? If so, can we put it there? > > Well, we have clients in content and chrome: > > content/browser/geolocation/wifi_data_provider_linux.cc > chrome/browser/password_manager/native_backend_kwallet_x.cc Hm, I guess I'd suggest putting this in content/ then. > > >> > >> > >> >> On Thu, Jul 21, 2011 at 8:36 PM, <mailto:satorux@chromium.org> >> >> wrote: >> >> > On 2011/07/21 17:23:01, Evan Martin wrote: >> >> >> >> >> >> On 2011/07/21 17:08:41, satorux1 wrote: >> >> >> > I >> >> >> > hope net/dbus would be the right place for D-Bus client library. >> >> > >> >> >> Hm, I'm not sure. dbus isn't really used for networking, is >> >> >> it? >> > >> > Maybe we >> >> >> >> >> use >> >> >> dbus for getting the proxy config? Or is that done in Chrome >> >> >> and >> >> >> then >> >> >> passed >> >> >> down to the network layer? >> >> > >> >> > D-Bus uses the unix domain socket to do IPC, so I thought 'net' would >> >> > be >> >> > appropriate. Otherwise, where would be a better place for this? >> >> > >> >> > Currently we use D-Bus for two things for Linux. >> >> > >> >> > - for geolocation, talk to NetworkManager to get wireless access >> >> > points >> >> > information, >> >> > - for passowrd manager, talk to kwallet application >> >> > >> >> > For Chrome OS, we use D-Bus for many other things, via a library >> >> > called >> >> > libcros, >> >> > which we plan to get rid of. >> >> > >> >> > http://codereview.chromium.org/7467031/ >> >> > >> > >> > >> > >> > http://codereview.chromium.org/7467031/ >> > > > > > http://codereview.chromium.org/7467031/ >
On 2011/07/21 18:24:05, willchan wrote: > On Thu, Jul 21, 2011 at 9:11 PM, <mailto:satorux@chromium.org> wrote: > > On 2011/07/21 18:06:05, willchan wrote: > >> > >> On Thu, Jul 21, 2011 at 8:45 PM, <mailto:satorux@chromium.org> wrote: > >> > On 2011/07/21 17:40:42, willchan wrote: > >> >> > >> >> Is there a reason this goes into the Chromium source tree rather than > >> >> as an external project pulled in by DEPS? Are the APIs supposed to > >> >> change that often that we want to minimize the churn by putting this > >> >> directly in src/? > >> > > >> > The implementation will be using lots of code from 'base' so it would be > >> > difficult to have this outside the chrome tree. Besides, we don't plan > >> > to > >> > use it > >> > for anything other than Chrome. We plan to keep improving the D-Bus > >> > library > >> > actively until it becomes feature complete and very stable. > > > >> OIC. That sounds reasonable. I'm not sure I'm fond of having this in > >> net/, although I'm definitely open to this. Is it only used in chrome/ > >> code? If so, can we put it there? > > > > Well, we have clients in content and chrome: > > > > content/browser/geolocation/wifi_data_provider_linux.cc > > chrome/browser/password_manager/native_backend_kwallet_x.cc > > Hm, I guess I'd suggest putting this in content/ then. I'm fine with that, though most of new clients code will probably in chrome/ (i.e. chrome os clients will be in chrome/browser/chromeos). What about content/common/dbus ? Satoru > > > > > > >> > > >> > > >> >> On Thu, Jul 21, 2011 at 8:36 PM, <mailto:satorux@chromium.org> > >> >> wrote: > >> >> > On 2011/07/21 17:23:01, Evan Martin wrote: > >> >> >> > >> >> >> On 2011/07/21 17:08:41, satorux1 wrote: > >> >> >> > I > >> >> >> > hope net/dbus would be the right place for D-Bus client library. > >> >> > > >> >> >> Hm, I'm not sure. dbus isn't really used for networking, is > >> >> >> it? > >> > > >> > Maybe we > >> >> > >> >> >> use > >> >> >> dbus for getting the proxy config? Or is that done in Chrome > >> >> >> and > >> >> >> then > >> >> >> passed > >> >> >> down to the network layer? > >> >> > > >> >> > D-Bus uses the unix domain socket to do IPC, so I thought 'net' would > >> >> > be > >> >> > appropriate. Otherwise, where would be a better place for this? > >> >> > > >> >> > Currently we use D-Bus for two things for Linux. > >> >> > > >> >> > - for geolocation, talk to NetworkManager to get wireless access > >> >> > points > >> >> > information, > >> >> > - for passowrd manager, talk to kwallet application > >> >> > > >> >> > For Chrome OS, we use D-Bus for many other things, via a library > >> >> > called > >> >> > libcros, > >> >> > which we plan to get rid of. > >> >> > > >> >> > http://codereview.chromium.org/7467031/ > >> >> > > >> > > >> > > >> > > >> > http://codereview.chromium.org/7467031/ > >> > > > > > > > > > http://codereview.chromium.org/7467031/ > >
I hate to say it, but content seems weird to me to. Maybe ui/? Really the "app" directory was exactly for stuff like this, I don't know why we deleted it. :\
On 2011/07/21 19:24:32, Evan Martin wrote: > I hate to say it, but content seems weird to me to. Maybe ui/? Really the > "app" directory was exactly for stuff like this, I don't know why we deleted it. > :\ I like bikeshedding so let me continue this game. "ui" sounds awkward as D-Bus is essentially an IPC layer that can be used for anything not only for UI. "ipc" directory may be an option but the directory only contains the chrome IPC system, so having D-Bus there would also be awkward there even in a sub directory. :~(
On 2011/07/21 19:33:43, satorux1 wrote: > On 2011/07/21 19:24:32, Evan Martin wrote: > > I hate to say it, but content seems weird to me to. Maybe ui/? Really the > > "app" directory was exactly for stuff like this, I don't know why we deleted > it. > > :\ > > I like bikeshedding so let me continue this game. "ui" sounds awkward as D-Bus > is essentially an IPC layer that can be used for anything not only for UI. "ipc" > directory may be an option but the directory only contains the chrome IPC > system, so having D-Bus there would also be awkward there even in a sub > directory. :~( We had a similar issue for our sqlite bindings. I wonder where they ended up? ... hahah, they're a top-level directory now. http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre... http://code.google.com/p/chromium/issues/detail?id=72317 I wonder if it's worth bringing app/ back. I see I even commented on the original thread saying that we should've kept it. :~(
On 2011/07/21 19:40:00, Evan Martin wrote: > On 2011/07/21 19:33:43, satorux1 wrote: > > On 2011/07/21 19:24:32, Evan Martin wrote: > > > I hate to say it, but content seems weird to me to. Maybe ui/? Really the > > > "app" directory was exactly for stuff like this, I don't know why we deleted > > it. > > > :\ > > > > I like bikeshedding so let me continue this game. "ui" sounds awkward as D-Bus > > is essentially an IPC layer that can be used for anything not only for UI. > "ipc" > > directory may be an option but the directory only contains the chrome IPC > > system, so having D-Bus there would also be awkward there even in a sub > > directory. :~( > > We had a similar issue for our sqlite bindings. I wonder where they ended up? > > ... hahah, they're a top-level directory now. > > http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre... > > http://code.google.com/p/chromium/issues/detail?id=72317 > > I wonder if it's worth bringing app/ back. I see I even commented on the > original thread saying that we should've kept it. :~( Heh, I wasn't aware of the thread. I'd rather not to bring back 'app' as it would likely cause a larger bikeshed thread that would be no longer fun. That said, net/dbus may still be a reasonable candidate. I'm also fine with content/common/dbus.
On 2011/07/21 19:55:43, satorux1 wrote: > On 2011/07/21 19:40:00, Evan Martin wrote: > > On 2011/07/21 19:33:43, satorux1 wrote: > > > On 2011/07/21 19:24:32, Evan Martin wrote: > > > > I hate to say it, but content seems weird to me to. Maybe ui/? Really > the > > > > "app" directory was exactly for stuff like this, I don't know why we > deleted > > > it. > > > > :\ > > > > > > I like bikeshedding so let me continue this game. "ui" sounds awkward as > D-Bus > > > is essentially an IPC layer that can be used for anything not only for UI. > > "ipc" > > > directory may be an option but the directory only contains the chrome IPC > > > system, so having D-Bus there would also be awkward there even in a sub > > > directory. :~( > > > > We had a similar issue for our sqlite bindings. I wonder where they ended up? > > > > ... hahah, they're a top-level directory now. > > > > > http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre... > > > > http://code.google.com/p/chromium/issues/detail?id=72317 > > > > I wonder if it's worth bringing app/ back. I see I even commented on the > > original thread saying that we should've kept it. :~( > > Heh, I wasn't aware of the thread. I'd rather not to bring back 'app' as it > would likely cause a larger bikeshed thread that would be no longer fun. > > That said, net/dbus may still be a reasonable candidate. I'm also fine with > content/common/dbus. Maybe we could introduce a new top-level directory named 'util' that may be better than 'misc' or 'app'. Don't know if it could upset someone.
The reason I prefer content/ is because I am under the impression we've been pushed code out to the leaves unless we need to share them. I don't think we put code in base/ just because it's generally useful, but only if it needs to be shared at that level. net/ feels weird to me since it's only tangentially related to net/. But it doesn't really matter I guess. On Thu, Jul 21, 2011 at 11:01 PM, <satorux@chromium.org> wrote: > On 2011/07/21 19:55:43, satorux1 wrote: >> >> On 2011/07/21 19:40:00, Evan Martin wrote: >> > On 2011/07/21 19:33:43, satorux1 wrote: >> > > On 2011/07/21 19:24:32, Evan Martin wrote: >> > > > I hate to say it, but content seems weird to me to. Maybe ui/? >> > > > Really >> the >> > > > "app" directory was exactly for stuff like this, I don't know why we >> deleted >> > > it. >> > > > :\ >> > > >> > > I like bikeshedding so let me continue this game. "ui" sounds awkward >> > > as >> D-Bus >> > > is essentially an IPC layer that can be used for anything not only for >> > > UI. >> > "ipc" >> > > directory may be an option but the directory only contains the chrome >> > > IPC >> > > system, so having D-Bus there would also be awkward there even in a >> > > sub >> > > directory. :~( >> > >> > We had a similar issue for our sqlite bindings. I wonder where they >> > ended > > up? >> >> > >> > ... hahah, they're a top-level directory now. >> > >> > > > http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre... >> >> > >> > http://code.google.com/p/chromium/issues/detail?id=72317 >> > >> > I wonder if it's worth bringing app/ back. I see I even commented on >> > the >> > original thread saying that we should've kept it. :~( > >> Heh, I wasn't aware of the thread. I'd rather not to bring back 'app' as >> it >> would likely cause a larger bikeshed thread that would be no longer fun. > >> That said, net/dbus may still be a reasonable candidate. I'm also fine >> with >> content/common/dbus. > > Maybe we could introduce a new top-level directory named 'util' that may be > better than 'misc' or 'app'. Don't know if it could upset someone. > > http://codereview.chromium.org/7467031/ >
I polled my office. They think net/ is the least bad of the options so far proposed, but it's still bad. Tony points out that we've moved many similar libraries (breakpad, jingle, googleurl, sql) to the toplevel so perhaps that is your best option. (I mean, putting stuff at the toplevel seems wrong, but this at the toplevel seems no more wrong than the others.) On Thu, Jul 21, 2011 at 1:05 PM, William Chan (陈智昌) <willchan@chromium.org> wrote: > The reason I prefer content/ is because I am under the impression > we've been pushed code out to the leaves unless we need to share them. > I don't think we put code in base/ just because it's generally useful, > but only if it needs to be shared at that level. net/ feels weird to > me since it's only tangentially related to net/. But it doesn't really > matter I guess. > > On Thu, Jul 21, 2011 at 11:01 PM, <satorux@chromium.org> wrote: >> On 2011/07/21 19:55:43, satorux1 wrote: >>> >>> On 2011/07/21 19:40:00, Evan Martin wrote: >>> > On 2011/07/21 19:33:43, satorux1 wrote: >>> > > On 2011/07/21 19:24:32, Evan Martin wrote: >>> > > > I hate to say it, but content seems weird to me to. Maybe ui/? >>> > > > Really >>> the >>> > > > "app" directory was exactly for stuff like this, I don't know why we >>> deleted >>> > > it. >>> > > > :\ >>> > > >>> > > I like bikeshedding so let me continue this game. "ui" sounds awkward >>> > > as >>> D-Bus >>> > > is essentially an IPC layer that can be used for anything not only for >>> > > UI. >>> > "ipc" >>> > > directory may be an option but the directory only contains the chrome >>> > > IPC >>> > > system, so having D-Bus there would also be awkward there even in a >>> > > sub >>> > > directory. :~( >>> > >>> > We had a similar issue for our sqlite bindings. I wonder where they >>> > ended >> >> up? >>> >>> > >>> > ... hahah, they're a top-level directory now. >>> > >>> > >> >> http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre... >>> >>> > >>> > http://code.google.com/p/chromium/issues/detail?id=72317 >>> > >>> > I wonder if it's worth bringing app/ back. I see I even commented on >>> > the >>> > original thread saying that we should've kept it. :~( >> >>> Heh, I wasn't aware of the thread. I'd rather not to bring back 'app' as >>> it >>> would likely cause a larger bikeshed thread that would be no longer fun. >> >>> That said, net/dbus may still be a reasonable candidate. I'm also fine >>> with >>> content/common/dbus. >> >> Maybe we could introduce a new top-level directory named 'util' that may be >> better than 'misc' or 'app'. Don't know if it could upset someone. >> >> http://codereview.chromium.org/7467031/ >> >
On 2011/07/21 20:36:57, Evan Martin wrote: > I polled my office. They think net/ is the least bad of the options > so far proposed, but it's still bad. > > Tony points out that we've moved many similar libraries (breakpad, > jingle, googleurl, sql) to the toplevel so perhaps that is your best > option. > > (I mean, putting stuff at the toplevel seems wrong, but this at the > toplevel seems no more wrong than the others.) I'm fine with that. Eventually, we'll end up introducing a directory like 'util' on the top level to clean things up, but until then, having 'dbus' on the top-level would be okay... > > On Thu, Jul 21, 2011 at 1:05 PM, William Chan (陈智昌) > <mailto:willchan@chromium.org> wrote: > > The reason I prefer content/ is because I am under the impression > > we've been pushed code out to the leaves unless we need to share them. > > I don't think we put code in base/ just because it's generally useful, > > but only if it needs to be shared at that level. net/ feels weird to > > me since it's only tangentially related to net/. But it doesn't really > > matter I guess. > > > > On Thu, Jul 21, 2011 at 11:01 PM, mailto: <satorux@chromium.org> wrote: > >> On 2011/07/21 19:55:43, satorux1 wrote: > >>> > >>> On 2011/07/21 19:40:00, Evan Martin wrote: > >>> > On 2011/07/21 19:33:43, satorux1 wrote: > >>> > > On 2011/07/21 19:24:32, Evan Martin wrote: > >>> > > > I hate to say it, but content seems weird to me to. Maybe ui/? > >>> > > > Really > >>> the > >>> > > > "app" directory was exactly for stuff like this, I don't know why we > >>> deleted > >>> > > it. > >>> > > > :\ > >>> > > > >>> > > I like bikeshedding so let me continue this game. "ui" sounds awkward > >>> > > as > >>> D-Bus > >>> > > is essentially an IPC layer that can be used for anything not only for > >>> > > UI. > >>> > "ipc" > >>> > > directory may be an option but the directory only contains the chrome > >>> > > IPC > >>> > > system, so having D-Bus there would also be awkward there even in a > >>> > > sub > >>> > > directory. :~( > >>> > > >>> > We had a similar issue for our sqlite bindings. I wonder where they > >>> > ended > >> > >> up? > >>> > >>> > > >>> > ... hahah, they're a top-level directory now. > >>> > > >>> > > >> > >> > http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre... > >>> > >>> > > >>> > http://code.google.com/p/chromium/issues/detail?id=72317 > >>> > > >>> > I wonder if it's worth bringing app/ back. I see I even commented on > >>> > the > >>> > original thread saying that we should've kept it. :~( > >> > >>> Heh, I wasn't aware of the thread. I'd rather not to bring back 'app' as > >>> it > >>> would likely cause a larger bikeshed thread that would be no longer fun. > >> > >>> That said, net/dbus may still be a reasonable candidate. I'm also fine > >>> with > >>> content/common/dbus. > >> > >> Maybe we could introduce a new top-level directory named 'util' that may be > >> better than 'misc' or 'app'. Don't know if it could upset someone. > >> > >> http://codereview.chromium.org/7467031/ > >> > >
I'm going to send a LGTM now because I'm headed out, and I don't want to be a blocker. I'm not happy with using net/, but I also sympathize with the lack of a good location to put this code. I've notified some other net/ OWNERS so they may hop on this review if they oppose. On Thu, Jul 21, 2011 at 11:36 PM, Evan Martin <evan@chromium.org> wrote: > I polled my office. They think net/ is the least bad of the options > so far proposed, but it's still bad. > > Tony points out that we've moved many similar libraries (breakpad, > jingle, googleurl, sql) to the toplevel so perhaps that is your best > option. > > (I mean, putting stuff at the toplevel seems wrong, but this at the > toplevel seems no more wrong than the others.) > > On Thu, Jul 21, 2011 at 1:05 PM, William Chan (陈智昌) > <willchan@chromium.org> wrote: >> The reason I prefer content/ is because I am under the impression >> we've been pushed code out to the leaves unless we need to share them. >> I don't think we put code in base/ just because it's generally useful, >> but only if it needs to be shared at that level. net/ feels weird to >> me since it's only tangentially related to net/. But it doesn't really >> matter I guess. >> >> On Thu, Jul 21, 2011 at 11:01 PM, <satorux@chromium.org> wrote: >>> On 2011/07/21 19:55:43, satorux1 wrote: >>>> >>>> On 2011/07/21 19:40:00, Evan Martin wrote: >>>> > On 2011/07/21 19:33:43, satorux1 wrote: >>>> > > On 2011/07/21 19:24:32, Evan Martin wrote: >>>> > > > I hate to say it, but content seems weird to me to. Maybe ui/? >>>> > > > Really >>>> the >>>> > > > "app" directory was exactly for stuff like this, I don't know why we >>>> deleted >>>> > > it. >>>> > > > :\ >>>> > > >>>> > > I like bikeshedding so let me continue this game. "ui" sounds awkward >>>> > > as >>>> D-Bus >>>> > > is essentially an IPC layer that can be used for anything not only for >>>> > > UI. >>>> > "ipc" >>>> > > directory may be an option but the directory only contains the chrome >>>> > > IPC >>>> > > system, so having D-Bus there would also be awkward there even in a >>>> > > sub >>>> > > directory. :~( >>>> > >>>> > We had a similar issue for our sqlite bindings. I wonder where they >>>> > ended >>> >>> up? >>>> >>>> > >>>> > ... hahah, they're a top-level directory now. >>>> > >>>> > >>> >>> http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre... >>>> >>>> > >>>> > http://code.google.com/p/chromium/issues/detail?id=72317 >>>> > >>>> > I wonder if it's worth bringing app/ back. I see I even commented on >>>> > the >>>> > original thread saying that we should've kept it. :~( >>> >>>> Heh, I wasn't aware of the thread. I'd rather not to bring back 'app' as >>>> it >>>> would likely cause a larger bikeshed thread that would be no longer fun. >>> >>>> That said, net/dbus may still be a reasonable candidate. I'm also fine >>>> with >>>> content/common/dbus. >>> >>> Maybe we could introduce a new top-level directory named 'util' that may be >>> better than 'misc' or 'app'. Don't know if it could upset someone. >>> >>> http://codereview.chromium.org/7467031/ >>> >> >
On Thu, Jul 21, 2011 at 1:44 PM, <satorux@chromium.org> wrote: >> (I mean, putting stuff at the toplevel seems wrong, but this at the >> toplevel seems no more wrong than the others.) > > I'm fine with that. Eventually, we'll end up introducing a directory like > 'util' > on the top level to clean things up, but until then, having 'dbus' on the > top-level would be okay... Let's do this. I will approve it and it feels better to me than net.
src/dbus, src/base/dbus, and src/net/dbus are all fine by me. We can always move the files later.
On 2011/07/21 21:17:26, wtc wrote: > src/dbus, src/base/dbus, and src/net/dbus are all fine by me. > We can always move the files later. Thank you for all the feedback. Updated the patch and created dbus/dbus.gyp
LGTM http://codereview.chromium.org/7467031/diff/4005/build/all.gyp File build/all.gyp (right): http://codereview.chromium.org/7467031/diff/4005/build/all.gyp#newcode83 build/all.gyp:83: '../dbus/dbus.gyp:*', I think you don't need this, unless you're just adding it for testing purposes. (As soon as something uses your new dbus.gyp it will be pulled into the build as normal.)
PS: make sure that nobody's build breaks due to there not being any sources, I'm not sure if the build systems handle that corner case correctly On Thu, Jul 21, 2011 at 2:34 PM, <evan@chromium.org> wrote: > LGTM > > > http://codereview.chromium.org/7467031/diff/4005/build/all.gyp > File build/all.gyp (right): > > http://codereview.chromium.org/7467031/diff/4005/build/all.gyp#newcode83 > build/all.gyp:83: '../dbus/dbus.gyp:*', > I think you don't need this, unless you're just adding it for testing > purposes. (As soon as something uses your new dbus.gyp it will be > pulled into the build as normal.) > > http://codereview.chromium.org/7467031/ >
Running try bots to make sure any builds won't break. http://codereview.chromium.org/7467031/diff/4005/build/all.gyp File build/all.gyp (right): http://codereview.chromium.org/7467031/diff/4005/build/all.gyp#newcode83 build/all.gyp:83: '../dbus/dbus.gyp:*', On 2011/07/21 21:34:10, Evan Martin wrote: > I think you don't need this, unless you're just adding it for testing purposes. > (As soon as something uses your new dbus.gyp it will be pulled into the build as > normal.) Good point. Removed.
Change committed as 93658 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
