Moving libxml_utils to third_party/libxml

[Dave Tu]

Hi,

I'm moving libxml_utils.cc and libxml_utils.h from chrome/common into third_party/libxml. This also adds +third_party/libxml into chrome/test/DEPS. Let me know if there are any objections.

https://chromiumcodereview.appspot.com/10251004/

[Paweł Hajdan, Jr.]

See my reply in code review. third_party/libxml is not really a good idea, because putting additional files there that are not part of upstream sources is going to increase maintenance effort of updating the library, and also break Linux distros that remove entire contents of third_party directories to make sure no bundled libs are used.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Evan Martin]

The reason for third_party's existence is to track code that falls
under a license separate from the main code. We shouldn't put any
code that isn't part of upstream's libxml there.

I'm not familiar with the last three rounds of renamings, but at one
point the purpose of chrome/common was for common Chrome code, which
seems a natural place for a dependency of chrome/test.

[Dave Tu]

I want to parse XML files for GPU blacklisting on Windows, see http://crbug.com/122838. This code is in content/gpu, and chrome/common is inaccessible from there. jam suggested base as the proper place for files that are required by both chrome and content, but brettw said no, we would rather not add a dependency on libxml into base if it's not already there.

[Alexei Svitkine]

On Tue, May 1, 2012 at 6:40 PM, Dave Tu <d...@google.com> wrote:
> I want to parse XML files for GPU blacklisting on Windows,
> see http://crbug.com/122838. This code is in content/gpu, and chrome/common
> is inaccessible from there. jam suggested base as the proper place for files
> that are required by both chrome and content, but brettw said no, we would
> rather not add a dependency on libxml into base if it's not already there.

Is there a reason you need to use XML and not, for example, protocol
buffers, which would be much more efficient?

-Alexei

[Alexei Svitkine]

On Tue, May 1, 2012 at 6:44 PM, Alexei Svitkine <asvi...@chromium.org> wrote:
> On Tue, May 1, 2012 at 6:40 PM, Dave Tu <d...@google.com> wrote:
>> I want to parse XML files for GPU blacklisting on Windows,
>> see http://crbug.com/122838. This code is in content/gpu, and chrome/common
>> is inaccessible from there. jam suggested base as the proper place for files
>> that are required by both chrome and content, but brettw said no, we would
>> rather not add a dependency on libxml into base if it's not already there.
>
> Is there a reason you need to use XML and not, for example, protocol
> buffers, which would be much more efficient?

Nevermind, looking at the bug, I see you want to parse XML files from
the system. Ignore my comment.

-Alexei

[Ryan Sleevi]

On Tue, May 1, 2012 at 3:44 PM, Alexei Svitkine <asvi...@chromium.org> wrote:

On Tue, May 1, 2012 at 6:40 PM, Dave Tu <d...@google.com> wrote:
> I want to parse XML files for GPU blacklisting on Windows,
> see http://crbug.com/122838. This code is in content/gpu, and chrome/common
> is inaccessible from there. jam suggested base as the proper place for files
> that are required by both chrome and content, but brettw said no, we would
> rather not add a dependency on libxml into base if it's not already there.

Is there a reason you need to use XML and not, for example, protocol
buffers, which would be much more efficient?

-Alexei

See linked bug. It's a Windows-provided XML file. The public APIs wrapping the reads of this file lead to a significant jump in crashes, including a nasty Heisenbug - http://googlechromereleases.blogspot.com/2012/04/stable-channel-update.html 

[Tom Wiltzius]

While I understand there are concerns with various suggested approaches, we do need a path forward... can one of John, Brett, Pawel, or Evan suggest where this should go? We'd appreciate being able to fix the bug this is blocking, as its probably causing some user pain on low-performance computers. Thanks,

Tom

[Ilya Sherman]

Why can't this go into content/common?

[John Abd-El-Malek]

This file is used by chrome/, and chrome/ can only include files from content/public. We only put interfaces there and not utility files like this.

[Ilya Sherman]

Sorry, I still don't understand why this can't be structured as 

content/public/common/libxml_utils.h

content/common/libxml_utils_impl.cc

Is the problem that the utility functions are not core content/ functionality?  If so, it sounds like you're suggesting creating yet another target to house this and similar utility files that need to be shared between content/ and chrome/, but not exported as part of the "public content API".  Is that accurate, or would you recommend something else instead?

[John Abd-El-Malek]

On Wed, May 2, 2012 at 11:25 PM, Ilya Sherman <ishe...@chromium.org> wrote:

Sorry, I still don't understand why this can't be structured as 

content/public/common/libxml_utils.h

content/common/libxml_utils_impl.cc

Is the problem that the utility functions are not core content/ functionality?

The Content API is similar to the WebKit API, in the public directory there are just interfaces/structs that are used by embedders. By design we don't put utility functions there. Usually, for things shared between content and other modules, these have gone into base.

 If so, it sounds like you're suggesting creating yet another target to house this and similar utility files that need to be shared between content/ and chrome/, but not exported as part of the "public content API".  Is that accurate, or would you recommend something else instead?

No I didn't suggest suggest that. I just think that whatever solution we come up with will have to avoid putting helper files in content/public.

It's still not clear to me why this can't be in third_party/libxml, especially under a different directory (/third_party/libxml/google ?). That directory can have a different license file. We do have other directories under third_party like patches which include our changes.

[Evan Martin]

On Wed, May 2, 2012 at 6:33 PM, Tom Wiltzius <wilt...@chromium.org> wrote:

> While I understand there are concerns with various suggested approaches, we
> do need a path forward... can one of John, Brett, Pawel, or Evan suggest
> where this should go? We'd appreciate being able to fix the bug this is
> blocking, as its probably causing some user pain on low-performance
> computers.

Non-third-party code that is itself contained should just be a
top-level directory. You could name it just "xml".

(Precedent: see also src/dbus, src/courgette, src/breakpad, etc.)

[John Abd-El-Malek]

I think it would be excessive to add a top level directory of two files.

[Zhenyao Mo]

Is there any reason we can't put this under base/?

It looks like a good place to me.

[John Abd-El-Malek]

The reason is we wouldn't want base to depend on xml which is large (and goes against base being a small library).

Another alternative would be to have a base_xml target that depends on libxml.

[Brett Wilson]

On Thu, May 3, 2012 at 2:54 PM, Zhenyao Mo <z...@google.com> wrote:

> Is there any reason we can't put this under base/?
>
> It looks like a good place to me.

This will bring in libxml into base which we don't want.

I talked with Will about this and we previously decided
third_party/foo is the only place for files related to "foo" when we
don't want to have base depending on all of third_party. We already
have a bunch of google code in various third party directories.
third_party/libxml/google is clear enough and doesn't increase
maintenance burden.

Brett

[Zhenyao Mo]

third_party/libxml/google sounds good to me.

[thecapta...@gmail.com]

I'm trying to enable WebGL on my Raspberry Pi Chrome, but it says;

• WebGL: Unavailable

And when I tried to find out why, I get this:

• Accelerated video decode is unavailable on Linux: 137247
  Disabled Features: accelerated_video_decode

How is this even possible? I have the version Chrome/51.0.2704.91. Can someone help me?