Move libxml_utils from chrome/common to third_party/libxml/chromium.

Move libxml_utils from chrome/common to third_party/libxml/chromium.

Why here? Discussion at:
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/fafe7c6bb54c4ed5/


BUG=chromium:119817
TEST=Chrome builds on all platforms and trybots succeed.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137247

[Paweł Hajdan Jr., 2012-04-28 07:10:47]

Unless that's part of libxml (doesn't seem so, there are Chromium license
headers), putting that in src/libxml will break Linux distros that _remove_
contents of third_party directories to ensure the bundled libraries are not
used.

Please find a better place. Each modification of upstream sources, even if it's
"just" dropping our files there without need (and there is no need because it
worked without the files), adds to the maintenance effort of updating the
library.

[dtu, 2012-05-03 23:53:55]

+stevenjb, need OWNERS for chrome/browser/chromeos/dbus
+jam, do I need a LICENSE file in third_party/libxml/chromium?

[stevenjb, 2012-05-04 00:25:16]

owner lgtm for chrome/browser/chromeos/dbus/introspect_util.cc

[jam, 2012-05-04 06:36:35]

I don't know about the license part. I'd follow what we did in the other
third_party dirs that Brett mentioned

http://codereview.chromium.org/10251004/diff/53008/third_party/libxml/chromiu...
File third_party/libxml/chromium/include/libxml/libxml_utils.h (right):

http://codereview.chromium.org/10251004/diff/53008/third_party/libxml/chromiu...
third_party/libxml/chromium/include/libxml/libxml_utils.h:5: #ifndef
__XML_LIBXML_UTILS_H__
nit: this should be THIRD_PARTY_LIBXML_CHROMIUM_INCLUDE_LIBXML_LIBXML_UTILS_H_

[Paweł Hajdan Jr., 2012-05-04 07:37:15]

http://codereview.chromium.org/10251004/diff/53008/third_party/libxml/libxml.gyp
File third_party/libxml/libxml.gyp (right):

http://codereview.chromium.org/10251004/diff/53008/third_party/libxml/libxml....
third_party/libxml/libxml.gyp:43: 'chromium/include/libxml/libxml_utils.h',
This breaks when -Duse_system_libxml=1 is used. Can those two files be put in a
separate target?

[jam, 2012-05-06 06:47:03]

On 2012/05/04 07:37:15, Paweł Hajdan Jr. wrote:
>
http://codereview.chromium.org/10251004/diff/53008/third_party/libxml/libxml.gyp
> File third_party/libxml/libxml.gyp (right):
> 
>
http://codereview.chromium.org/10251004/diff/53008/third_party/libxml/libxml....
> third_party/libxml/libxml.gyp:43: 'chromium/include/libxml/libxml_utils.h',
> This breaks when -Duse_system_libxml=1 is used. Can those two files be put in
a
> separate target?

note that extra targets are not cheap, i.e. they make loading/unloading
solutions in VS/XCode slower.

is it possible to make -Duse_system_libxml=1 bring in the file to an existing
target?

[Paweł Hajdan Jr., 2012-05-07 06:24:38]

On 2012/05/06 06:47:03, John Abd-El-Malek wrote:
> note that extra targets are not cheap, i.e. they make loading/unloading
> solutions in VS/XCode slower.
> 
> is it possible to make -Duse_system_libxml=1 bring in the file to an existing
> target?

It should be. Please make sure to test it, and I'd like to see it before it gets
committed.

[dtu, 2012-05-07 22:59:11]

PTAL. Added the needed dependencies and includes under the use_sytem_libxml
condition and made sure it builds with -Duse_system_libxml=1.

http://codereview.chromium.org/10251004/diff/53008/third_party/libxml/chromiu...
File third_party/libxml/chromium/include/libxml/libxml_utils.h (right):

http://codereview.chromium.org/10251004/diff/53008/third_party/libxml/chromiu...
third_party/libxml/chromium/include/libxml/libxml_utils.h:5: #ifndef
__XML_LIBXML_UTILS_H__
On 2012/05/04 06:36:35, John Abd-El-Malek wrote:
> nit: this should be THIRD_PARTY_LIBXML_CHROMIUM_INCLUDE_LIBXML_LIBXML_UTILS_H_

Done.

http://codereview.chromium.org/10251004/diff/53008/third_party/libxml/libxml.gyp
File third_party/libxml/libxml.gyp (right):

http://codereview.chromium.org/10251004/diff/53008/third_party/libxml/libxml....
third_party/libxml/libxml.gyp:43: 'chromium/include/libxml/libxml_utils.h',
On 2012/05/04 07:37:25, Paweł Hajdan Jr. wrote:
> This breaks when -Duse_system_libxml=1 is used. Can those two files be put in
a
> separate target?

Fixed the use_system_libxml condition.

[Paweł Hajdan Jr., 2012-05-08 06:41:42]

LGTM with a comment

I'd really prefer a separate target though, so that we can avoid duplication.
Now the code is more fragile because the sources are duplicated, and some
direct_dependent_settings are also duplicated.

http://codereview.chromium.org/10251004/diff/52011/third_party/libxml/libxml.gyp
File third_party/libxml/libxml.gyp (right):

http://codereview.chromium.org/10251004/diff/52011/third_party/libxml/libxml....
third_party/libxml/libxml.gyp:38: 'defines': [
I think you should also have USE_SYSTEM_LIBXML defined for chromium/ files.

[dtu, 2012-05-10 00:06:03]

jam?

http://codereview.chromium.org/10251004/diff/52011/third_party/libxml/libxml.gyp
File third_party/libxml/libxml.gyp (right):

http://codereview.chromium.org/10251004/diff/52011/third_party/libxml/libxml....
third_party/libxml/libxml.gyp:38: 'defines': [
On 2012/05/08 06:41:42, Paweł Hajdan Jr. wrote:
> I think you should also have USE_SYSTEM_LIBXML defined for chromium/ files.

Done.

[jam, 2012-05-14 06:21:13]

(was ooo, in the future feel free to send to other reviewers instead of waiting)

http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
File chrome/browser/chromeos/dbus/introspect_util.cc (right):

http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
chrome/browser/chromeos/dbus/introspect_util.cc:7: #include
"libxml/libxml_utils.h"
nit: here and elsewhere, this should be third_party/libxml per the style guide

[dtu, 2012-05-14 21:14:43]

http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
File chrome/browser/chromeos/dbus/introspect_util.cc (right):

http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
chrome/browser/chromeos/dbus/introspect_util.cc:7: #include
"libxml/libxml_utils.h"
On 2012/05/14 06:21:14, John Abd-El-Malek wrote:
> nit: here and elsewhere, this should be third_party/libxml per the style guide

Which style guide? Does this only apply to ChromeOS? A number of the files in
this CL won't compile with #include "third_party/libxml/libxml_utils.h"

[jam, 2012-05-15 00:02:12]

On 2012/05/14 21:14:43, Dave Tu wrote:
>
http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
> File chrome/browser/chromeos/dbus/introspect_util.cc (right):
> 
>
http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
> chrome/browser/chromeos/dbus/introspect_util.cc:7: #include
> "libxml/libxml_utils.h"
> On 2012/05/14 06:21:14, John Abd-El-Malek wrote:
> > nit: here and elsewhere, this should be third_party/libxml per the style
guide
> 
> Which style guide?

chrome/google (not sure which, I would presume google). as you can see from all
of our includes, we always list them from src/. this doesn't apply to code in
third_party of course.

> Does this only apply to ChromeOS?

all of code in src/ other than in src/third_party

> A number of the files in
> this CL won't compile with #include "third_party/libxml/libxml_utils.h"

which ones?

[dtu, 2012-05-15 00:13:43]

On 2012/05/15 00:02:12, John Abd-El-Malek wrote:
> On 2012/05/14 21:14:43, Dave Tu wrote:
> >
>
http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
> > File chrome/browser/chromeos/dbus/introspect_util.cc (right):
> > 
> >
>
http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
> > chrome/browser/chromeos/dbus/introspect_util.cc:7: #include
> > "libxml/libxml_utils.h"
> > On 2012/05/14 06:21:14, John Abd-El-Malek wrote:
> > > nit: here and elsewhere, this should be third_party/libxml per the style
> guide
> > 
> > Which style guide?
> 
> chrome/google (not sure which, I would presume google). as you can see from
all
> of our includes, we always list them from src/. this doesn't apply to code in
> third_party of course.
> 
> > Does this only apply to ChromeOS?
> 
> all of code in src/ other than in src/third_party
> 
> > A number of the files in
> > this CL won't compile with #include "third_party/libxml/libxml_utils.h"
> 
> which ones?

Uhm, I think all of them. Here's an example try result with just the files under
browser/chromeos changed (patch set 7):
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[dtu, 2012-05-15 00:18:16]

On 2012/05/15 00:13:43, Dave Tu wrote:
> On 2012/05/15 00:02:12, John Abd-El-Malek wrote:
> > On 2012/05/14 21:14:43, Dave Tu wrote:
> > >
> >
>
http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
> > > File chrome/browser/chromeos/dbus/introspect_util.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
> > > chrome/browser/chromeos/dbus/introspect_util.cc:7: #include
> > > "libxml/libxml_utils.h"
> > > On 2012/05/14 06:21:14, John Abd-El-Malek wrote:
> > > > nit: here and elsewhere, this should be third_party/libxml per the style
> > guide
> > > 
> > > Which style guide?
> > 
> > chrome/google (not sure which, I would presume google). as you can see from
> all
> > of our includes, we always list them from src/. this doesn't apply to code
in
> > third_party of course.
> > 
> > > Does this only apply to ChromeOS?
> > 
> > all of code in src/ other than in src/third_party
> > 
> > > A number of the files in
> > > this CL won't compile with #include "third_party/libxml/libxml_utils.h"
> > 
> > which ones?
> 
> Uhm, I think all of them. Here's an example try result with just the files
under
> browser/chromeos changed (patch set 7):
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

And all of the files that use libxml do it by just "libxml/":
http://code.google.com/searchframe#search/&exact_package=chromium&q=%22%23inc...

[dtu, 2012-05-15 00:28:23]

On 2012/05/15 00:18:16, Dave Tu wrote:
> On 2012/05/15 00:13:43, Dave Tu wrote:
> > On 2012/05/15 00:02:12, John Abd-El-Malek wrote:
> > > On 2012/05/14 21:14:43, Dave Tu wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
> > > > File chrome/browser/chromeos/dbus/introspect_util.cc (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/10251004/diff/46008/chrome/browser/chromeos/db...
> > > > chrome/browser/chromeos/dbus/introspect_util.cc:7: #include
> > > > "libxml/libxml_utils.h"
> > > > On 2012/05/14 06:21:14, John Abd-El-Malek wrote:
> > > > > nit: here and elsewhere, this should be third_party/libxml per the
style
> > > guide
> > > > 
> > > > Which style guide?
> > > 
> > > chrome/google (not sure which, I would presume google). as you can see
from
> > all
> > > of our includes, we always list them from src/. this doesn't apply to code
> in
> > > third_party of course.
> > > 
> > > > Does this only apply to ChromeOS?
> > > 
> > > all of code in src/ other than in src/third_party
> > > 
> > > > A number of the files in
> > > > this CL won't compile with #include "third_party/libxml/libxml_utils.h"
> > > 
> > > which ones?
> > 
> > Uhm, I think all of them. Here's an example try result with just the files
> under
> > browser/chromeos changed (patch set 7):
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
> 
> And all of the files that use libxml do it by just "libxml/":
>
http://code.google.com/searchframe#search/&exact_package=chromium&q=%2522%252...

Sorry, made a mistake; it should be "#include
"third_party/libxml/chromium/include/libxml/libxml_utils.h""

[jam, 2012-05-15 16:23:57]

http://codereview.chromium.org/10251004/diff/59031/chrome/browser/DEPS
File chrome/browser/DEPS (right):

http://codereview.chromium.org/10251004/diff/59031/chrome/browser/DEPS#newcode55
chrome/browser/DEPS:55: "+third_party/libxml",  # For search engine definition
parsing.
nit: just put this in chrome/DEPS instead of the 3 directories

http://codereview.chromium.org/10251004/diff/59031/third_party/libxml/chromiu...
File third_party/libxml/chromium/include/libxml/libxml_utils.h (right):

http://codereview.chromium.org/10251004/diff/59031/third_party/libxml/chromiu...
third_party/libxml/chromium/include/libxml/libxml_utils.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
why isn't this just third_party/libxml/chromium/libxml_utils.h? i.e. why the
"include/libxml" subdirectories?

[dtu, 2012-05-15 20:50:59]

http://codereview.chromium.org/10251004/diff/59031/chrome/browser/DEPS
File chrome/browser/DEPS (right):

http://codereview.chromium.org/10251004/diff/59031/chrome/browser/DEPS#newcode55
chrome/browser/DEPS:55: "+third_party/libxml",  # For search engine definition
parsing.
On 2012/05/15 16:23:58, John Abd-El-Malek wrote:
> nit: just put this in chrome/DEPS instead of the 3 directories

Done.

http://codereview.chromium.org/10251004/diff/59031/third_party/libxml/chromiu...
File third_party/libxml/chromium/include/libxml/libxml_utils.h (right):

http://codereview.chromium.org/10251004/diff/59031/third_party/libxml/chromiu...
third_party/libxml/chromium/include/libxml/libxml_utils.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2012/05/15 16:23:58, John Abd-El-Malek wrote:
> why isn't this just third_party/libxml/chromium/libxml_utils.h? i.e. why the
> "include/libxml" subdirectories?

Done.

[jam, 2012-05-15 20:58:36]

lgtm