Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2240793003:
net_watcher: Stop linking against glib/gconf/gio. (Closed)
|
Created:
4 years, 4 months ago by Raphael Kubo da Costa (rakuco) Modified:
4 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionnet_watcher: Stop linking against glib/gconf/gio.
The dependency on glib is only present because we need to call
g_type_init(), but commit 9d47fa1658 ("Add calls to g_type_init")
has moved the call to ProxyConfigServiceLinux, which is what
actually uses it.
R=pauljensen@chromium.org,mmenke@chromium.org,xunjieli@chromium.org
Committed: https://crrev.com/938b74344dd6fb4de351fa3507b25e37434ab5a7
Cr-Commit-Position: refs/heads/master@{#412503}
Patch Set 1 #
Messages
Total messages: 22 (5 generated)
PTAL.
The commit description says this in unneeded because it's obsolete, but it seems like it's unneeded because of https://codereview.chromium.org/2015243002/diff/40001/net/proxy/proxy_config_...
On 2016/08/12 11:49:33, pauljensen wrote: > The commit description says this in unneeded because it's obsolete, but it seems > like it's unneeded because of > https://codereview.chromium.org/2015243002/diff/40001/net/proxy/proxy_config_... Hmm, I honestly believed it was unneeded because it's obsolete; the commit message of the CL you mentioned shows errors related to glib 2.32.4, which is the version shipped by Debian Wheezy (oldstable). Even though the current Chromium sysroot uses a Wheezy chroot, I've always been under the impression that Chrome/Chromium were never expected to run on it. Would you like me to mention both the fact that calling g_type_init() is obsolete as well as pointing to that CL as another reason why the code is not needed in net_watcher?
Hrm...Does anyone actually use this tool? Just wondering if it makes more sense to remove it. Suppose we're removing net-internals from Chrome, so won't be able to view live events there any more.
No idea, but I can gladly write a CL doing that if that's what the net team prefers.
On 2016/08/12 15:55:41, Raphael Kubo da Costa (rakuco) wrote: > No idea, but I can gladly write a CL doing that if that's what the net team > prefers. Let's wait to hear from pauljensen on that - he's most likely to know if someone is using it.
I use net_watcher occasionally. It's very useful when I need it, but I must admit I probably haven't used it in the last two years as I don't believe it works on Android and I've been fairly Android focused for the past few years. I'd prefer we keep it working if possible.
On 2016/08/12 12:09:12, Raphael Kubo da Costa (rakuco) wrote: > On 2016/08/12 11:49:33, pauljensen wrote: > > The commit description says this in unneeded because it's obsolete, but it > seems > > like it's unneeded because of > > > https://codereview.chromium.org/2015243002/diff/40001/net/proxy/proxy_config_... > > Hmm, I honestly believed it was unneeded because it's obsolete; the commit > message of the CL you mentioned shows errors related to glib 2.32.4, which is > the version shipped by Debian Wheezy (oldstable). Even though the current > Chromium sysroot uses a Wheezy chroot, I've always been under the impression > that Chrome/Chromium were never expected to run on it. > > Would you like me to mention both the fact that calling g_type_init() is > obsolete as well as pointing to that CL as another reason why the code is not > needed in net_watcher? If net_watcher works on older versions of linux because of the change I linked to (which I think it does, and it is expected to), I think we should mention that, rather than getting into a discussion of whether the call itself is obsolete.
Description was changed from ========== net_watcher: Stop linking against glib/gconf/gio. The dependency on glib is only present because we need to call g_type_init() when using glib < 2.35.0. That version is pretty old and all distributions officially supported by Chrome on Linux (Debian 8+, Fedora 21+, openSUSE 13.1+ and Ubuntu 14.04+) have more recent versions that do not need this call. R=pauljensen@chromium.org,mmenke@chromium.org,xunjieli@chromium.org ========== to ========== net_watcher: Stop linking against glib/gconf/gio. The dependency on glib is only present because we need to call g_type_init(), but commit 9d47fa1658395080390c4bbc01545958f209ce30 ("Add calls to g_type_init") has moved the call to ProxyConfigServiceLinux, which is what actually uses it. R=pauljensen@chromium.org,mmenke@chromium.org,xunjieli@chromium.org ==========
Description was changed from
==========
net_watcher: Stop linking against glib/gconf/gio.
The dependency on glib is only present because we need to call
g_type_init(), but commit 9d47fa1658395080390c4bbc01545958f209ce30 ("Add calls
to g_type_init") has moved the call to ProxyConfigServiceLinux, which is what
actually uses it.
R=pauljensen@chromium.org,mmenke@chromium.org,xunjieli@chromium.org
==========
to
==========
net_watcher: Stop linking against glib/gconf/gio.
The dependency on glib is only present because we need to call
g_type_init(), but commit 9d47fa1658 ("Add calls to g_type_init")
has moved the call to ProxyConfigServiceLinux, which is what
actually uses it.
R=pauljensen@chromium.org,mmenke@chromium.org,xunjieli@chromium.org
==========
I've updated the CL description to mention the one you linked to; please take another look and see if it's OK.
On 2016/08/12 16:16:31, pauljensen wrote: > On 2016/08/12 12:09:12, Raphael Kubo da Costa (rakuco) wrote: > > On 2016/08/12 11:49:33, pauljensen wrote: > > > The commit description says this in unneeded because it's obsolete, but it > > seems > > > like it's unneeded because of > > > > > > https://codereview.chromium.org/2015243002/diff/40001/net/proxy/proxy_config_... > > > > Hmm, I honestly believed it was unneeded because it's obsolete; the commit > > message of the CL you mentioned shows errors related to glib 2.32.4, which is > > the version shipped by Debian Wheezy (oldstable). Even though the current > > Chromium sysroot uses a Wheezy chroot, I've always been under the impression > > that Chrome/Chromium were never expected to run on it. > > > > Would you like me to mention both the fact that calling g_type_init() is > > obsolete as well as pointing to that CL as another reason why the code is not > > needed in net_watcher? > > If net_watcher works on older versions of linux because of the change I linked > to (which I think it does, and it is expected to), I think we should mention > that, rather than getting into a discussion of whether the call itself is > obsolete. If we're going to keep it, should we add docs for it somewhere? Not in this CL, of course. Nowhere that I can see is it documented. I didn't even know it existed.
On 2016/08/12 17:09:47, mmenke wrote: > On 2016/08/12 16:16:31, pauljensen wrote: > > On 2016/08/12 12:09:12, Raphael Kubo da Costa (rakuco) wrote: > > > On 2016/08/12 11:49:33, pauljensen wrote: > > > > The commit description says this in unneeded because it's obsolete, but it > > > seems > > > > like it's unneeded because of > > > > > > > > > > https://codereview.chromium.org/2015243002/diff/40001/net/proxy/proxy_config_... > > > > > > Hmm, I honestly believed it was unneeded because it's obsolete; the commit > > > message of the CL you mentioned shows errors related to glib 2.32.4, which > is > > > the version shipped by Debian Wheezy (oldstable). Even though the current > > > Chromium sysroot uses a Wheezy chroot, I've always been under the impression > > > that Chrome/Chromium were never expected to run on it. > > > > > > Would you like me to mention both the fact that calling g_type_init() is > > > obsolete as well as pointing to that CL as another reason why the code is > not > > > needed in net_watcher? > > > > If net_watcher works on older versions of linux because of the change I linked > > to (which I think it does, and it is expected to), I think we should mention > > that, rather than getting into a discussion of whether the call itself is > > obsolete. > > If we're going to keep it, should we add docs for it somewhere? Not in this CL, > of course. Nowhere that I can see is it documented. I didn't even know it > existed. I didn't write it but I can take a stab at documenting it. The one-liner at the top of net_watcher.cc about sums it up but I can make it more verbose.
On 2016/08/12 16:33:24, Raphael Kubo da Costa (rakuco) wrote: > I've updated the CL description to mention the one you linked to; please take > another look and see if it's OK. Did you try building and running on an old version of linux where calling g_type_init() is required?
On 2016/08/16 16:55:14, pauljensen wrote:
> On 2016/08/12 16:33:24, Raphael Kubo da Costa (rakuco) wrote:
> > I've updated the CL description to mention the one you linked to; please
take
> > another look and see if it's OK.
>
> Did you try building and running on an old version of linux where calling
> g_type_init() is required?
I did now: I launched an Ubuntu Precise (12.04, with glib 2.32.4) Live CD and
copied a net_watcher binary built on my actual machine (Fedora 24) using
Chromium's Wheezy sysroot (which has libglib2.0-0_2.33.12+really2.32.4). My GN
flags were "is_debug = true" and "is_component_build = false". Upon launching
it, the only messages I got were:
[0817/083520:INFO:net_watcher.cc(187)] Initial connection type:
CONNECTION_UNKNOWN
[0817/083520:INFO:net_watcher.cc(195)] Initial proxy config:
{"source":"GSETTINGS"}, CONFIG_VALID
[0817/083520:INFO:net_watcher.cc(200)] Watching for network events...
lgtm
The CQ bit was checked by raphael.kubo.da.costa@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
net_watcher: Stop linking against glib/gconf/gio.
The dependency on glib is only present because we need to call
g_type_init(), but commit 9d47fa1658 ("Add calls to g_type_init")
has moved the call to ProxyConfigServiceLinux, which is what
actually uses it.
R=pauljensen@chromium.org,mmenke@chromium.org,xunjieli@chromium.org
==========
to
==========
net_watcher: Stop linking against glib/gconf/gio.
The dependency on glib is only present because we need to call
g_type_init(), but commit 9d47fa1658 ("Add calls to g_type_init")
has moved the call to ProxyConfigServiceLinux, which is what
actually uses it.
R=pauljensen@chromium.org,mmenke@chromium.org,xunjieli@chromium.org
==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from
==========
net_watcher: Stop linking against glib/gconf/gio.
The dependency on glib is only present because we need to call
g_type_init(), but commit 9d47fa1658 ("Add calls to g_type_init")
has moved the call to ProxyConfigServiceLinux, which is what
actually uses it.
R=pauljensen@chromium.org,mmenke@chromium.org,xunjieli@chromium.org
==========
to
==========
net_watcher: Stop linking against glib/gconf/gio.
The dependency on glib is only present because we need to call
g_type_init(), but commit 9d47fa1658 ("Add calls to g_type_init")
has moved the call to ProxyConfigServiceLinux, which is what
actually uses it.
R=pauljensen@chromium.org,mmenke@chromium.org,xunjieli@chromium.org
Committed: https://crrev.com/938b74344dd6fb4de351fa3507b25e37434ab5a7
Cr-Commit-Position: refs/heads/master@{#412503}
==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/938b74344dd6fb4de351fa3507b25e37434ab5a7 Cr-Commit-Position: refs/heads/master@{#412503} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
