Move the following files to content\public\plugin.

Move content_plugin_client.h to content\public\plugin.

The plugin_carbon_interpose_mac.cc which intercepts system API's in plugin processes on the Mac has been moved to

content\plugin. The plugin_carbon_interpose shared library target which lives in chrome.gyp refers to this file.

This is a short term workaround while we look into switching the interposing mechanism on the mac.

Changes as part of creating a Content API

BUG=98716
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105128

[Avi (use Gerrit), 2011-10-11 00:41:57]

Two things.

First, can we get Stuart Morgan to review moving plugin_carbon_interpose_mac? He
originally put it in browser. I'm adding him as reviewer.

Second, plugin_carbon_interpose_mac definitely belongs in content along with
plugin_interpose_util_mac. plugin_carbon_interpose_mac is in a separate file
only because of technical limitations; it is conceptually part of
plugin_interpose_util_mac.

[Dirk Pranke, 2011-10-11 00:48:56]

On 2011/10/11 00:41:57, Avi wrote:
> Two things.
> 
> First, can we get Stuart Morgan to review moving plugin_carbon_interpose_mac?
He
> originally put it in browser. I'm adding him as reviewer.
> 
> Second, plugin_carbon_interpose_mac definitely belongs in content along with
> plugin_interpose_util_mac. plugin_carbon_interpose_mac is in a separate file
> only because of technical limitations; it is conceptually part of
> plugin_interpose_util_mac.

I'm not sure that plugin_interpose_util_mac.mm should be in content/public ;
when John first started putting stuff in content/public I had objected to
putting *any* implementation there, and he thought it would be okay to put
*trivial* implementations there. I don't think this file meets my definition of
trivial, so I thought I would re-raise this point to see what others might
think?

[Avi (use Gerrit), 2011-10-11 00:53:48]

The interpose util is an internal implementation detail of plugins on the mac.
It's definitely not something to be exposed as API. It shouldn't need to be if
we move the carbon interpose library into content as well.

[jam, 2011-10-11 01:03:29]

agreed that those files don't look like a public API. If we _really_ wanted to
put it in public, then there would be an interface around it (which may be just
a bunch of static function), and the actual implementation would be in
content\plugin (similar to how we do it for WebKit API).

The only cc files we put in the public dir are things like
constructors/destructors that we would normally put in the header if it weren't
for the clang check.

plugin_carbon_interpose_mac.cc seems like it would be needed by every embedder
of content? if so it should be in content.

[jam, 2011-10-11 01:03:36]

http://codereview.chromium.org/8224021/diff/2002/content/public/plugin/conten...
File content/public/plugin/content_plugin_client.h (right):

http://codereview.chromium.org/8224021/diff/2002/content/public/plugin/conten...
content/public/plugin/content_plugin_client.h:5: #ifndef
CONTENT_PLUGIN_CONTENT_PLUGIN_CLIENT_H_
nit: update header guards

[Avi (use Gerrit), 2011-10-11 01:27:47]

On 2011/10/11 01:03:29, John Abd-El-Malek wrote:
> plugin_carbon_interpose_mac.cc seems like it would be needed by every embedder
> of content? if so it should be in content.

In fact, the library built from it is _already_ loaded by code in content
(content/plugin/plugin_main_mac.mm:29), so it needs to move, no question.

[ananta, 2011-10-11 01:39:47]

On 2011/10/11 01:27:47, Avi wrote:
> On 2011/10/11 01:03:29, John Abd-El-Malek wrote:
> > plugin_carbon_interpose_mac.cc seems like it would be needed by every
embedder
> > of content? if so it should be in content.
> 
> In fact, the library built from it is _already_ loaded by code in content
> (content/plugin/plugin_main_mac.mm:29), so it needs to move, no question.

I moved the plugin_carbon_interpose_mac.cc file to content\plugin. The
plugin_interpose_util.mac_h/.mm files remain in content\plugin. The
plugin_carbon_interpose target has been moved to content_plugin.gypi

[ananta, 2011-10-11 01:40:00]

On 2011/10/11 01:03:36, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/8224021/diff/2002/content/public/plugin/conten...
> File content/public/plugin/content_plugin_client.h (right):
> 
>
http://codereview.chromium.org/8224021/diff/2002/content/public/plugin/conten...
> content/public/plugin/content_plugin_client.h:5: #ifndef
> CONTENT_PLUGIN_CONTENT_PLUGIN_CLIENT_H_
> nit: update header guards

Done.

[jam, 2011-10-11 02:08:39]

http://codereview.chromium.org/8224021/diff/20/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/8224021/diff/20/chrome/chrome.gyp#newcode1080
chrome/chrome.gyp:1080: # Dummy target to allow chrome to require
plugin_carbon_interpose to
would all embedders want this magic? i.e. should this block be in content?

http://codereview.chromium.org/8224021/diff/20/chrome/plugin/plugin_carbon_in...
File chrome/plugin/plugin_carbon_interpose_mac.cc (right):

http://codereview.chromium.org/8224021/diff/20/chrome/plugin/plugin_carbon_in...
chrome/plugin/plugin_carbon_interpose_mac.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
i think you forgot to delete the version in chrome

http://codereview.chromium.org/8224021/diff/20/content/content_plugin.gypi
File content/content_plugin.gypi (right):

http://codereview.chromium.org/8224021/diff/20/content/content_plugin.gypi#ne...
content/content_plugin.gypi:59: 'chrome_dll',
it's a layering violation that a module in content depends on one in chrome. why
is this needed?

http://codereview.chromium.org/8224021/diff/20/content/content_plugin.gypi#ne...
content/content_plugin.gypi:87:
'@executable_path/../Versions/<(version_full)/<(mac_product_name)
Framework.framework/<(mac_product_name) Framework',
nit: i don't know what this code is, but will this work with any embedder or is
this chrome specific?

[Avi (use Gerrit), 2011-10-11 15:58:22]

http://codereview.chromium.org/8224021/diff/20/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/8224021/diff/20/chrome/chrome.gyp#newcode1080
chrome/chrome.gyp:1080: # Dummy target to allow chrome to require
plugin_carbon_interpose to
All embedders require the carbon interpose library to achieve correct plugin
functionality.

http://codereview.chromium.org/8224021/diff/20/content/content_plugin.gypi
File content/content_plugin.gypi (right):

http://codereview.chromium.org/8224021/diff/20/content/content_plugin.gypi#ne...
content/content_plugin.gypi:59: 'chrome_dll',
As I mentioned, you should bring the carbon interpose target to content/, and
remove the dependency on chrome_dll. It only has a dependency on content anyway.

[Mark Mentovai, 2011-10-11 18:26:01]

http://codereview.chromium.org/8224021/diff/20/content/content_plugin.gypi
File content/content_plugin.gypi (right):

http://codereview.chromium.org/8224021/diff/20/content/content_plugin.gypi#ne...
content/content_plugin.gypi:59: 'chrome_dll',
Avi wrote:
> As I mentioned, you should bring the carbon interpose target to content/, and
> remove the dependency on chrome_dll. It only has a dependency on content
anyway.

While it may be true that it only depends on code that lives in content, this
target has a dependency on chrome_dll because it needs to link against the
framework. It must link against the framework (chrome_dll), not against static
libraries like content.

http://codereview.chromium.org/8224021/diff/20/content/content_plugin.gypi#ne...
content/content_plugin.gypi:87:
'@executable_path/../Versions/<(version_full)/<(mac_product_name)
Framework.framework/<(mac_product_name) Framework',
John Abd-El-Malek wrote:
> nit: i don't know what this code is, but will this work with any embedder or
is
> this chrome specific?

This is Chrome-specific. It’s just about impossible to do this in a way generic
enough for embedders to handle. This is very dependent on the packaging
structure.

[Avi (use Gerrit), 2011-10-11 18:30:26]

On 2011/10/11 18:26:01, Mark Mentovai wrote:
> This is Chrome-specific.

How? I'm not talking about the linking issues. The interception of window and
cursor calls is required for Carbon plugins. They won't function properly
without it. Are we willing to say that plugins hosted by content code simply
don't support Carbon?

(At least one argument for mach_override, as bad as it is.)

[Mark Mentovai, 2011-10-11 18:31:18]

On 2011/10/11 18:30:26, Avi wrote:
> On 2011/10/11 18:26:01, Mark Mentovai wrote:
> > This is Chrome-specific.
> 
> How? I'm not talking about the linking issues. The interception of window and
> cursor calls is required for Carbon plugins. They won't function properly
> without it. Are we willing to say that plugins hosted by content code simply
> don't support Carbon?

I was talking specifically about the packaging stuff, responding to John’s
comment on the line with a pathname.

[jam, 2011-10-11 20:47:24]

On 2011/10/11 18:31:18, Mark Mentovai wrote:
> On 2011/10/11 18:30:26, Avi wrote:
> > On 2011/10/11 18:26:01, Mark Mentovai wrote:
> > > This is Chrome-specific.
> > 
> > How? I'm not talking about the linking issues. The interception of window
and
> > cursor calls is required for Carbon plugins. They won't function properly
> > without it. Are we willing to say that plugins hosted by content code simply
> > don't support Carbon?
> 
> I was talking specifically about the packaging stuff, responding to John’s
> comment on the line with a pathname.

so would it make sense to leave the code in content, but the gyp target around
it in chrome?

[Dirk Pranke, 2011-10-11 23:05:18]

On Tue, Oct 11, 2011 at 1:47 PM,  <jam@chromium.org> wrote:
> On 2011/10/11 18:31:18, Mark Mentovai wrote:
>>
>> On 2011/10/11 18:30:26, Avi wrote:
>> > On 2011/10/11 18:26:01, Mark Mentovai wrote:
>> > > This is Chrome-specific.
>> >
>> > How? I'm not talking about the linking issues. The interception of
>> > window
>
> and
>>
>> > cursor calls is required for Carbon plugins. They won't function
>> > properly
>> > without it. Are we willing to say that plugins hosted by content code
>> > simply
>> > don't support Carbon?
>
>> I was talking specifically about the packaging stuff, responding to John’s
>> comment on the line with a pathname.
>
> so would it make sense to leave the code in content, but the gyp target
> around
> it in chrome?
>

I really don't like having to do that.

-- Dirk

> http://codereview.chromium.org/8224021/
>

[jam, 2011-10-11 23:12:46]

On Tue, Oct 11, 2011 at 4:04 PM, Dirk Pranke <dpranke@chromium.org> wrote:

> On Tue, Oct 11, 2011 at 1:47 PM,  <jam@chromium.org> wrote:
> > On 2011/10/11 18:31:18, Mark Mentovai wrote:
> >>
> >> On 2011/10/11 18:30:26, Avi wrote:
> >> > On 2011/10/11 18:26:01, Mark Mentovai wrote:
> >> > > This is Chrome-specific.
> >> >
> >> > How? I'm not talking about the linking issues. The interception of
> >> > window
> >
> > and
> >>
> >> > cursor calls is required for Carbon plugins. They won't function
> >> > properly
> >> > without it. Are we willing to say that plugins hosted by content code
> >> > simply
> >> > don't support Carbon?
> >
> >> I was talking specifically about the packaging stuff, responding to
> John’s
> >> comment on the line with a pathname.
> >
> > so would it make sense to leave the code in content, but the gyp target
> > around
> > it in chrome?
> >
>
> I really don't like having to do that.
>

Suggestions?

The code that got moved is something that'll need to be done by every
embedder, so I think it doesn't belong in chrome.

Avi has some ideas about using a different type of patching to do this so
that an external binary isn't needed.


>
> -- Dirk
>
> > http://codereview.chromium.org/8224021/
> >
>

[Dirk Pranke, 2011-10-11 23:25:54]

On Tue, Oct 11, 2011 at 4:12 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>
>
> On Tue, Oct 11, 2011 at 4:04 PM, Dirk Pranke <dpranke@chromium.org> wrote:
>>
>> On Tue, Oct 11, 2011 at 1:47 PM,  <jam@chromium.org> wrote:
>> > On 2011/10/11 18:31:18, Mark Mentovai wrote:
>> >>
>> >> On 2011/10/11 18:30:26, Avi wrote:
>> >> > On 2011/10/11 18:26:01, Mark Mentovai wrote:
>> >> > > This is Chrome-specific.
>> >> >
>> >> > How? I'm not talking about the linking issues. The interception of
>> >> > window
>> >
>> > and
>> >>
>> >> > cursor calls is required for Carbon plugins. They won't function
>> >> > properly
>> >> > without it. Are we willing to say that plugins hosted by content code
>> >> > simply
>> >> > don't support Carbon?
>> >
>> >> I was talking specifically about the packaging stuff, responding to
>> >> John’s
>> >> comment on the line with a pathname.
>> >
>> > so would it make sense to leave the code in content, but the gyp target
>> > around
>> > it in chrome?
>> >
>>
>> I really don't like having to do that.
>
> Suggestions?
> The code that got moved is something that'll need to be done by every
> embedder, so I think it doesn't belong in chrome.
> Avi has some ideas about using a different type of patching to do this so
> that an external binary isn't needed.
>

Sorry, I realize my comment wasn't that helpful. I don't fully
understand what's going on, so it's hard for me to offer much, but
there's no way this could be addressed through a callback that patches
things properly or provides the right strings? I'm sure you've
probably already thought of these ideas, though ...

It's just that if in order to use content you have to link against
targets defined in chrome, then content isn't really a standalone
component (obviously). To my mind, defining targets in chrome/* gyp
files isn't much better than having the source code live under
chrome/* .

-- Dirk

[jam, 2011-10-11 23:32:18]

On Tue, Oct 11, 2011 at 4:25 PM, Dirk Pranke <dpranke@chromium.org> wrote:

> On Tue, Oct 11, 2011 at 4:12 PM, John Abd-El-Malek <jam@chromium.org>
> wrote:
> >
> >
> > On Tue, Oct 11, 2011 at 4:04 PM, Dirk Pranke <dpranke@chromium.org>
> wrote:
> >>
> >> On Tue, Oct 11, 2011 at 1:47 PM,  <jam@chromium.org> wrote:
> >> > On 2011/10/11 18:31:18, Mark Mentovai wrote:
> >> >>
> >> >> On 2011/10/11 18:30:26, Avi wrote:
> >> >> > On 2011/10/11 18:26:01, Mark Mentovai wrote:
> >> >> > > This is Chrome-specific.
> >> >> >
> >> >> > How? I'm not talking about the linking issues. The interception of
> >> >> > window
> >> >
> >> > and
> >> >>
> >> >> > cursor calls is required for Carbon plugins. They won't function
> >> >> > properly
> >> >> > without it. Are we willing to say that plugins hosted by content
> code
> >> >> > simply
> >> >> > don't support Carbon?
> >> >
> >> >> I was talking specifically about the packaging stuff, responding to
> >> >> John’s
> >> >> comment on the line with a pathname.
> >> >
> >> > so would it make sense to leave the code in content, but the gyp
> target
> >> > around
> >> > it in chrome?
> >> >
> >>
> >> I really don't like having to do that.
> >
> > Suggestions?
> > The code that got moved is something that'll need to be done by every
> > embedder, so I think it doesn't belong in chrome.
> > Avi has some ideas about using a different type of patching to do this so
> > that an external binary isn't needed.
> >
>
> Sorry, I realize my comment wasn't that helpful. I don't fully
> understand what's going on, so it's hard for me to offer much, but
> there's no way this could be addressed through a callback that patches
> things properly or provides the right strings? I'm sure you've
> probably already thought of these ideas, though ...
>
> It's just that if in order to use content you have to link against
> targets defined in chrome, then content isn't really a standalone
> component (obviously). To my mind, defining targets in chrome/* gyp
> files isn't much better than having the source code live under
> chrome/* .
>

I'm not that familiar with Mac, but from what I understand from Avi, , the
exported functions that are used by this small helper come from content. The
small helper needs to depend on the binary that has those exported symbols,
which by definition will have to be chrome.

That's why it seems that Avi's suggestion of using the different way of
patching is better in terms of the content/chrome split.

>
> -- Dirk
>

[ananta, 2011-10-11 23:36:08]

On 2011/10/11 23:32:18, John Abd-El-Malek wrote:
> On Tue, Oct 11, 2011 at 4:25 PM, Dirk Pranke <mailto:dpranke@chromium.org>
wrote:
> 
> > On Tue, Oct 11, 2011 at 4:12 PM, John Abd-El-Malek <mailto:jam@chromium.org>
> > wrote:
> > >
> > >
> > > On Tue, Oct 11, 2011 at 4:04 PM, Dirk Pranke <mailto:dpranke@chromium.org>
> > wrote:
> > >>
> > >> On Tue, Oct 11, 2011 at 1:47 PM,  <mailto:jam@chromium.org> wrote:
> > >> > On 2011/10/11 18:31:18, Mark Mentovai wrote:
> > >> >>
> > >> >> On 2011/10/11 18:30:26, Avi wrote:
> > >> >> > On 2011/10/11 18:26:01, Mark Mentovai wrote:
> > >> >> > > This is Chrome-specific.
> > >> >> >
> > >> >> > How? I'm not talking about the linking issues. The interception of
> > >> >> > window
> > >> >
> > >> > and
> > >> >>
> > >> >> > cursor calls is required for Carbon plugins. They won't function
> > >> >> > properly
> > >> >> > without it. Are we willing to say that plugins hosted by content
> > code
> > >> >> > simply
> > >> >> > don't support Carbon?
> > >> >
> > >> >> I was talking specifically about the packaging stuff, responding to
> > >> >> John’s
> > >> >> comment on the line with a pathname.
> > >> >
> > >> > so would it make sense to leave the code in content, but the gyp
> > target
> > >> > around
> > >> > it in chrome?
> > >> >
> > >>
> > >> I really don't like having to do that.
> > >
> > > Suggestions?
> > > The code that got moved is something that'll need to be done by every
> > > embedder, so I think it doesn't belong in chrome.
> > > Avi has some ideas about using a different type of patching to do this so
> > > that an external binary isn't needed.
> > >
> >
> > Sorry, I realize my comment wasn't that helpful. I don't fully
> > understand what's going on, so it's hard for me to offer much, but
> > there's no way this could be addressed through a callback that patches
> > things properly or provides the right strings? I'm sure you've
> > probably already thought of these ideas, though ...
> >
> > It's just that if in order to use content you have to link against
> > targets defined in chrome, then content isn't really a standalone
> > component (obviously). To my mind, defining targets in chrome/* gyp
> > files isn't much better than having the source code live under
> > chrome/* .
> >
> 
> I'm not that familiar with Mac, but from what I understand from Avi, , the
> exported functions that are used by this small helper come from content. The
> small helper needs to depend on the binary that has those exported symbols,
> which by definition will have to be chrome.
> 
> That's why it seems that Avi's suggestion of using the different way of
> patching is better in terms of the content/chrome split.
> 
> >
> > -- Dirk
> >

Getting rid of the shim which patches the system API's seems like a good idea. I
dunno much about what the other style of patching entails and whether it has
limitations on the same lines as IAT patching on Windows, etc.

-Ananta

[jam, 2011-10-11 23:37:59]

On Tue, Oct 11, 2011 at 4:32 PM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Tue, Oct 11, 2011 at 4:25 PM, Dirk Pranke <dpranke@chromium.org> wrote:
>
>> On Tue, Oct 11, 2011 at 4:12 PM, John Abd-El-Malek <jam@chromium.org>
>> wrote:
>> >
>> >
>> > On Tue, Oct 11, 2011 at 4:04 PM, Dirk Pranke <dpranke@chromium.org>
>> wrote:
>> >>
>> >> On Tue, Oct 11, 2011 at 1:47 PM,  <jam@chromium.org> wrote:
>> >> > On 2011/10/11 18:31:18, Mark Mentovai wrote:
>> >> >>
>> >> >> On 2011/10/11 18:30:26, Avi wrote:
>> >> >> > On 2011/10/11 18:26:01, Mark Mentovai wrote:
>> >> >> > > This is Chrome-specific.
>> >> >> >
>> >> >> > How? I'm not talking about the linking issues. The interception of
>> >> >> > window
>> >> >
>> >> > and
>> >> >>
>> >> >> > cursor calls is required for Carbon plugins. They won't function
>> >> >> > properly
>> >> >> > without it. Are we willing to say that plugins hosted by content
>> code
>> >> >> > simply
>> >> >> > don't support Carbon?
>> >> >
>> >> >> I was talking specifically about the packaging stuff, responding to
>> >> >> John’s
>> >> >> comment on the line with a pathname.
>> >> >
>> >> > so would it make sense to leave the code in content, but the gyp
>> target
>> >> > around
>> >> > it in chrome?
>> >> >
>> >>
>> >> I really don't like having to do that.
>> >
>> > Suggestions?
>> > The code that got moved is something that'll need to be done by every
>> > embedder, so I think it doesn't belong in chrome.
>> > Avi has some ideas about using a different type of patching to do this
>> so
>> > that an external binary isn't needed.
>> >
>>
>> Sorry, I realize my comment wasn't that helpful. I don't fully
>> understand what's going on, so it's hard for me to offer much, but
>> there's no way this could be addressed through a callback that patches
>> things properly or provides the right strings? I'm sure you've
>> probably already thought of these ideas, though ...
>>
>> It's just that if in order to use content you have to link against
>> targets defined in chrome, then content isn't really a standalone
>> component (obviously). To my mind, defining targets in chrome/* gyp
>> files isn't much better than having the source code live under
>> chrome/* .
>>
>
> I'm not that familiar with Mac, but from what I understand from Avi, , the
> exported functions that are used by this small helper come from content. The
> small helper needs to depend on the binary that has those exported symbols,
> which by definition will have to be chrome.
>
> That's why it seems that Avi's suggestion of using the different way of
> patching is better in terms of the content/chrome split.
>

btw we do this in the Windows code in a number of places (some of them are
in plugin code, and sound very similar to this Mac code).


>> -- Dirk
>>
>
>

[Avi (use Gerrit), 2011-10-11 23:41:18]

Right now you'll have to convince Mark that the upside of using mach_override is
greater than the downside of using an unsupported mechanism.

Discussion at crbug.com/99879.

[ananta, 2011-10-12 00:46:51]

On 2011/10/11 23:41:18, Avi wrote:
> Right now you'll have to convince Mark that the upside of using mach_override
is
> greater than the downside of using an unsupported mechanism.
> 
> Discussion at crbug.com/99879.

As per John's suggestion moving the plugin_carbon_interpose_mac.cc file to
content\plugin and referring to it from the plugin_carbon_interpose target in
chrome.gyp.

Hopefully this is short term as we may switch the interception mechanism on the
mac.

PTAL.

[stuartmorgan, 2011-10-12 10:05:00]

I'm not sure I can actually provide any useful review here, since I'm not at all
in the loop as to the details of the content/ project. There's no question that
this code is conceptually an internal implementation detail of content though,
and that having API for it would be really undesireable.

I defer to Mark on the question of mach_override vs DYLD_INSERT_LIBRARIES.

What's the timeframe where we need to get the weird build dependency here
resolved? In light of this thread, I'm discussing EOL of Carbon plugin support
with Mark again right now, and if we were to make a product decision to do that
in the near future this whole problem would go away (possibly as early as EOQ4).

[jam, 2011-10-12 18:07:18]

lgtm