Add in support for internal pepper plugins into the PepperPluginRegistry and pepper::PluginModule.

Add in support for internal pepper plugins into the PepperPluginRegistry and pepper::PluginModule.

Used Chromoting's plugin as the first attempt at using this interface.

BUG=none
TEST=compiles

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

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

[awong, 2010-06-22 03:16:37]

Darin, you're listed for the pepper changes.

Gary, you're on here for the Chromoting portion.

This compiles, but I haven't had a chance to test whether or not it
runs/segfaults.  Will do tomorrow, but wanted to know if it looked like the
correct direction.

[darin (slow to review), 2010-06-22 16:02:14]

http://codereview.chromium.org/2843018/diff/2001/3003
File chrome/common/pepper_plugin_registry.cc (right):

http://codereview.chromium.org/2843018/diff/2001/3003#newcode82
chrome/common/pepper_plugin_registry.cc:82:
"pepper-application/x-chromoting-plugin");
i don't see the value in the pepper- prefix for the mime type.  the "-plugin"
suffix also seems unnecessary.  i think you should just call this
"application/x-chromoting"

http://codereview.chromium.org/2843018/diff/2001/3003#newcode110
chrome/common/pepper_plugin_registry.cc:110: plugins.clear();
why bother?

http://codereview.chromium.org/2843018/diff/2001/3003#newcode112
chrome/common/pepper_plugin_registry.cc:112: InternalPluginInfoMap
internal_plugin_info;
perhaps you should register the internal ones before the command line specified
ones?  that way the command line can be used to override an internal pepper
plugin.

http://codereview.chromium.org/2843018/diff/2001/3004
File chrome/common/pepper_plugin_registry.h (right):

http://codereview.chromium.org/2843018/diff/2001/3004#newcode15
chrome/common/pepper_plugin_registry.h:15: FilePath path;  // Internal plugs are
of the form "internal-[name]".
nit: plugs -> plugins

http://codereview.chromium.org/2843018/diff/2001/3004#newcode36
chrome/common/pepper_plugin_registry.h:36: struct PepperPluginInfoLess {
why use a map here?  seems overkill.  GetInternalPluginInfo should just return a
vector.

http://codereview.chromium.org/2843018/diff/2001/3005
File remoting/client/plugin/pepper_entrypoints.cc (right):

http://codereview.chromium.org/2843018/diff/2001/3005#newcode30
remoting/client/plugin/pepper_entrypoints.cc:30: static ChromotingModule*
g_module_singleton;
why don't you just make PPP_InitializeModule fail if g_module_singleton is
non-NULL?  i think it should be the case that your PPP_InitializeModule will
only be called once per process.  do you see something else happening?

http://codereview.chromium.org/2843018/diff/2001/3005#newcode36
remoting/client/plugin/pepper_entrypoints.cc:36: return 1;
please avoid magic numbers.

http://codereview.chromium.org/2843018/diff/2001/3005#newcode49
remoting/client/plugin/pepper_entrypoints.cc:49: delete g_module_singleton;
probably a good idea to null out g_module_singleton

http://codereview.chromium.org/2843018/diff/2001/3005#newcode65
remoting/client/plugin/pepper_entrypoints.cc:65: }
}  // namespace remoting

add a new line separating the closing brackets

http://codereview.chromium.org/2843018/diff/2001/3009
File webkit/glue/plugins/pepper_plugin_module.h (right):

http://codereview.chromium.org/2843018/diff/2001/3009#newcode28
webkit/glue/plugins/pepper_plugin_module.h:28: struct PluginModuleEntryPoints {
nit: since this struct is within the PluginModule namespace, there is no need
for the PluginModule prefix.  just name the struct EntryPoints :)

http://codereview.chromium.org/2843018/diff/2001/3009#newcode44
webkit/glue/plugins/pepper_plugin_module.h:44: PluginModuleEntryPoints
functions);
nit: perhaps the variable name should be entry_points to match the type name, or
the type name should change to Functions.

http://codereview.chromium.org/2843018/diff/2001/3009#newcode72
webkit/glue/plugins/pepper_plugin_module.h:72: bool InitFromFile(const FilePath&
filename);
nit: filename -> filepath or just path

http://codereview.chromium.org/2843018/diff/2001/3010
File webkit/glue/webkit_glue.gypi (right):

http://codereview.chromium.org/2843018/diff/2001/3010#newcode120
webkit/glue/webkit_glue.gypi:120:
'<(DEPTH)/third_party/ppapi/ppapi.gyp:ppapi_cpp',
why is this change needed?  it doesn't look like the changes to
pepper_plugin_module resulted in a dependency on ppapi_cpp.  the ppapi_cpp layer
is intended for use by the pepper plugin, but glue is part of the browser
implementation.

[awong, 2010-06-23 19:28:04]

PTAL

Addressed comments and did some other updates.

Biggest change this time is I stopped using ppapi's cpp interface due to the
Module singleton issues. 

Currently, I've literally forked the Module implementation into
pepper_entrypoints.cc.  I'm going to revisit how to factor this next, but wanted
to get this framework in so that other people can hack on different parts of the
functionality.

http://codereview.chromium.org/2843018/diff/2001/3003
File chrome/common/pepper_plugin_registry.cc (right):

http://codereview.chromium.org/2843018/diff/2001/3003#newcode82
chrome/common/pepper_plugin_registry.cc:82:
"pepper-application/x-chromoting-plugin");
On 2010/06/22 16:02:14, darin wrote:
> i don't see the value in the pepper- prefix for the mime type.  the "-plugin"
> suffix also seems unnecessary.  i think you should just call this
> "application/x-chromoting"

According to Gary, he tried not prefixing it with "pepper-application", and the
drawing context doesn't get initialized correctly.  I've removed the -plugin
suffix.  Let me know if I should still remove the pepper- prefix.

http://codereview.chromium.org/2843018/diff/2001/3003#newcode110
chrome/common/pepper_plugin_registry.cc:110: plugins.clear();
On 2010/06/22 16:02:14, darin wrote:
> why bother?

old code from when both shared a vector. removed.

http://codereview.chromium.org/2843018/diff/2001/3003#newcode112
chrome/common/pepper_plugin_registry.cc:112: InternalPluginInfoMap
internal_plugin_info;
On 2010/06/22 16:02:14, darin wrote:
> perhaps you should register the internal ones before the command line
specified
> ones?  that way the command line can be used to override an internal pepper
> plugin.


Good idea. Switched.

http://codereview.chromium.org/2843018/diff/2001/3004
File chrome/common/pepper_plugin_registry.h (right):

http://codereview.chromium.org/2843018/diff/2001/3004#newcode15
chrome/common/pepper_plugin_registry.h:15: FilePath path;  // Internal plugs are
of the form "internal-[name]".
On 2010/06/22 16:02:14, darin wrote:
> nit: plugs -> plugins

Done

http://codereview.chromium.org/2843018/diff/2001/3004#newcode36
chrome/common/pepper_plugin_registry.h:36: struct PepperPluginInfoLess {
On 2010/06/22 16:02:14, darin wrote:
> why use a map here?  seems overkill.  GetInternalPluginInfo should just return
a
> vector.

Done.

http://codereview.chromium.org/2843018/diff/2001/3005
File remoting/client/plugin/pepper_entrypoints.cc (right):

http://codereview.chromium.org/2843018/diff/2001/3005#newcode30
remoting/client/plugin/pepper_entrypoints.cc:30: static ChromotingModule*
g_module_singleton;
On 2010/06/22 16:02:14, darin wrote:
> why don't you just make PPP_InitializeModule fail if g_module_singleton is
> non-NULL?  i think it should be the case that your PPP_InitializeModule will
> only be called once per process.  do you see something else happening?


This code is essentially copied whole-sale from the example in ppapi.  Same with
all the magic numbers, etc., below.

http://codereview.chromium.org/2843018/diff/2001/3005#newcode49
remoting/client/plugin/pepper_entrypoints.cc:49: delete g_module_singleton;
On 2010/06/22 16:02:14, darin wrote:
> probably a good idea to null out g_module_singleton

Done.

http://codereview.chromium.org/2843018/diff/2001/3005#newcode65
remoting/client/plugin/pepper_entrypoints.cc:65: }
On 2010/06/22 16:02:14, darin wrote:
> }  // namespace remoting
> 
> add a new line separating the closing brackets

Done.

http://codereview.chromium.org/2843018/diff/2001/3009
File webkit/glue/plugins/pepper_plugin_module.h (right):

http://codereview.chromium.org/2843018/diff/2001/3009#newcode28
webkit/glue/plugins/pepper_plugin_module.h:28: struct PluginModuleEntryPoints {
On 2010/06/22 16:02:14, darin wrote:
> nit: since this struct is within the PluginModule namespace, there is no need
> for the PluginModule prefix.  just name the struct EntryPoints :)

Done.

http://codereview.chromium.org/2843018/diff/2001/3009#newcode44
webkit/glue/plugins/pepper_plugin_module.h:44: PluginModuleEntryPoints
functions);
On 2010/06/22 16:02:14, darin wrote:
> nit: perhaps the variable name should be entry_points to match the type name,
or
> the type name should change to Functions.

Done.

http://codereview.chromium.org/2843018/diff/2001/3009#newcode72
webkit/glue/plugins/pepper_plugin_module.h:72: bool InitFromFile(const FilePath&
filename);
On 2010/06/22 16:02:14, darin wrote:
> nit: filename -> filepath or just path

Done.

http://codereview.chromium.org/2843018/diff/2001/3010
File webkit/glue/webkit_glue.gypi (right):

http://codereview.chromium.org/2843018/diff/2001/3010#newcode120
webkit/glue/webkit_glue.gypi:120:
'<(DEPTH)/third_party/ppapi/ppapi.gyp:ppapi_cpp',
On 2010/06/22 16:02:14, darin wrote:
> why is this change needed?  it doesn't look like the changes to
> pepper_plugin_module resulted in a dependency on ppapi_cpp.  the ppapi_cpp
layer
> is intended for use by the pepper plugin, but glue is part of the browser
> implementation.

I see. I got confused.  It's removed.

[garykac, 2010-06-23 20:41:17]

LGTM

http://codereview.chromium.org/2843018/diff/24001/25005
File remoting/client/plugin/chromoting_plugin.cc (right):

http://codereview.chromium.org/2843018/diff/24001/25005#newcode26
remoting/client/plugin/chromoting_plugin.cc:26: const char*
ChromotingPlugin::kMimeType = "application/chromoting::Chromoting";
pepper-application/x-chromoting

http://codereview.chromium.org/2843018/diff/24001/25006
File remoting/client/plugin/chromoting_plugin.h (right):

http://codereview.chromium.org/2843018/diff/24001/25006#newcode15
remoting/client/plugin/chromoting_plugin.h:15: #include
"third_party/ppapi/c/ppb_instance.h"
How anal-retentive are we supposed to be WRT alphabetical order?  It seems like
this should be at the end.

http://codereview.chromium.org/2843018/diff/24001/25008
File remoting/client/plugin/pepper_entrypoints.h (right):

http://codereview.chromium.org/2843018/diff/24001/25008#newcode9
remoting/client/plugin/pepper_entrypoints.h:9: #include
"third_party/ppapi/c/pp_module.h"
order

[awong, 2010-06-23 22:59:24]

Gary's comments fixed.

Also fixed a pretty big bug in the conditional (forget a !).

Darin, any final comments, or should I checkin?

-Albert

[darin (slow to review), 2010-06-23 23:16:04]

LGTM

http://codereview.chromium.org/2843018/diff/29002/32003
File chrome/common/pepper_plugin_registry.cc (right):

http://codereview.chromium.org/2843018/diff/29002/32003#newcode66
chrome/common/pepper_plugin_registry.cc:66: // Currently, to centralize the
internal plugin registration
style-nit: make the comment block expand to fill the allotted 80 chars.  it'll
look a bit nicer.

http://codereview.chromium.org/2843018/diff/29002/32004
File chrome/common/pepper_plugin_registry.h (right):

http://codereview.chromium.org/2843018/diff/29002/32004#newcode34
chrome/common/pepper_plugin_registry.h:34: typedef
std::vector<std::pair<PepperPluginInfo,
nit: use a named struct instead of std::pair to make things more readable:

struct InternalPluginInfo : PepperPluginInfo {
  pepper::PluginModule::EntryPoints entry_points;
};
typedef std::vector<InternalPluginInfo> InternalPluginInfoList;

http://codereview.chromium.org/2843018/diff/29002/32005
File remoting/client/plugin/chromoting_plugin.cc (right):

http://codereview.chromium.org/2843018/diff/29002/32005#newcode118
remoting/client/plugin/chromoting_plugin.cc:118: /*
should this commented block of code be deleted?  or add a TODO(ajwong)?

[awong, 2010-06-23 23:32:22]

Fixed, and committing.

Thanks!

http://codereview.chromium.org/2843018/diff/29002/32003
File chrome/common/pepper_plugin_registry.cc (right):

http://codereview.chromium.org/2843018/diff/29002/32003#newcode66
chrome/common/pepper_plugin_registry.cc:66: // Currently, to centralize the
internal plugin registration
On 2010/06/23 23:16:04, darin wrote:
> style-nit: make the comment block expand to fill the allotted 80 chars.  it'll
> look a bit nicer.

Done.

http://codereview.chromium.org/2843018/diff/29002/32004
File chrome/common/pepper_plugin_registry.h (right):

http://codereview.chromium.org/2843018/diff/29002/32004#newcode34
chrome/common/pepper_plugin_registry.h:34: typedef
std::vector<std::pair<PepperPluginInfo,
On 2010/06/23 23:16:04, darin wrote:
> nit: use a named struct instead of std::pair to make things more readable:
> 
> struct InternalPluginInfo : PepperPluginInfo {
>   pepper::PluginModule::EntryPoints entry_points;
> };
> typedef std::vector<InternalPluginInfo> InternalPluginInfoList;

Done.

http://codereview.chromium.org/2843018/diff/29002/32005
File remoting/client/plugin/chromoting_plugin.cc (right):

http://codereview.chromium.org/2843018/diff/29002/32005#newcode118
remoting/client/plugin/chromoting_plugin.cc:118: /*
On 2010/06/23 23:16:04, darin wrote:
> should this commented block of code be deleted?  or add a TODO(ajwong)?

TODO added.  We want it...but for right now, I just want the module loaded and
dealing with switching to the c-api for this is just annoying.

[jam, 2010-06-23 23:44:31]

http://codereview.chromium.org/2843018/diff/37001/25014
File chrome/common/pepper_plugin_registry.cc (right):

http://codereview.chromium.org/2843018/diff/37001/25014#newcode85
chrome/common/pepper_plugin_registry.cc:85: info.entry_points.shutdown_module =
remoting::PPP_ShutdownModule;
are these internal plugins shown in about:plugins?  If not, they should be :) 
If they are, then how are we getting the plugin name and description?

[darin (slow to review), 2010-06-24 00:16:58]

On Wed, Jun 23, 2010 at 4:44 PM, <jam@chromium.org> wrote:

>
> http://codereview.chromium.org/2843018/diff/37001/25014
>
> File chrome/common/pepper_plugin_registry.cc (right):
>
> http://codereview.chromium.org/2843018/diff/37001/25014#newcode85
> chrome/common/pepper_plugin_registry.cc:85:
> info.entry_points.shutdown_module = remoting::PPP_ShutdownModule;
> are these internal plugins shown in about:plugins?  If not, they should
> be :)  If they are, then how are we getting the plugin name and
> description?
>
>
Yeah, I hooked up about:plugins support previously.  There is not a great
amount of data shown in about:plugins.

See PluginService::RegisterPepperPlugins() in plugin_service.cc

-Darin





>
> http://codereview.chromium.org/2843018/show
>

[awong, 2010-06-24 18:11:27]

Re-opening.

Adding a dependency from chrome/common -> remoting causes a circular dependency
in gyp which breaks mac.

I'm going to try and track down why we're depending on chrome/common from
remoting.  But distasteful as it might be, maybe it'd be better to just
duplicate the registration code inside of both BrowserMain and RendererMain.  Or
go with the statically initialized linked list of internal plugins mentioned
earlier...

-Albert


On 2010/06/24 00:16:58, darin wrote:
> On Wed, Jun 23, 2010 at 4:44 PM, <mailto:jam@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.org/2843018/diff/37001/25014
> >
> > File chrome/common/pepper_plugin_registry.cc (right):
> >
> > http://codereview.chromium.org/2843018/diff/37001/25014#newcode85
> > chrome/common/pepper_plugin_registry.cc:85:
> > info.entry_points.shutdown_module = remoting::PPP_ShutdownModule;
> > are these internal plugins shown in about:plugins?  If not, they should
> > be :)  If they are, then how are we getting the plugin name and
> > description?
> >
> >
> Yeah, I hooked up about:plugins support previously.  There is not a great
> amount of data shown in about:plugins.
> 
> See PluginService::RegisterPepperPlugins() in plugin_service.cc
> 
> -Darin
> 
> 
> 
> 
> 
> >
> > http://codereview.chromium.org/2843018/show
> >
>

[darin (slow to review), 2010-06-24 18:13:10]

It seems incorrect for remoting/ to depend on chrome/common/ assuming
that chrome/ depends on remoting/.

-Darin

On Thu, Jun 24, 2010 at 11:11 AM, <ajwong@chromium.org> wrote:

> Re-opening.
>
> Adding a dependency from chrome/common -> remoting causes a circular
> dependency
> in gyp which breaks mac.
>
> I'm going to try and track down why we're depending on chrome/common from
> remoting.  But distasteful as it might be, maybe it'd be better to just
> duplicate the registration code inside of both BrowserMain and
> RendererMain.  Or
> go with the statically initialized linked list of internal plugins
> mentioned
> earlier...
>
> -Albert
>
>
> On 2010/06/24 00:16:58, darin wrote:
>
>  On Wed, Jun 23, 2010 at 4:44 PM, <mailto:jam@chromium.org> wrote:
>>
>
>  >
>> > http://codereview.chromium.org/2843018/diff/37001/25014
>> >
>> > File chrome/common/pepper_plugin_registry.cc (right):
>> >
>> > http://codereview.chromium.org/2843018/diff/37001/25014#newcode85
>> > chrome/common/pepper_plugin_registry.cc:85:
>> > info.entry_points.shutdown_module = remoting::PPP_ShutdownModule;
>> > are these internal plugins shown in about:plugins?  If not, they should
>> > be :)  If they are, then how are we getting the plugin name and
>> > description?
>> >
>> >
>> Yeah, I hooked up about:plugins support previously.  There is not a great
>> amount of data shown in about:plugins.
>>
>
>  See PluginService::RegisterPepperPlugins() in plugin_service.cc
>>
>
>  -Darin
>>
>
>
>
>
>
>  >
>> > http://codereview.chromium.org/2843018/show
>> >
>>
>
>
>
>
> http://codereview.chromium.org/2843018/show
>

[Sergey Ulanov, 2010-06-24 18:29:52]

We use JsonPrefStore from src/chrome/common. It needs to moved to src/base,
and then we can remove chrome/common dependency.

On Thu, Jun 24, 2010 at 11:12 AM, Darin Fisher <darin@chromium.org> wrote:

> It seems incorrect for remoting/ to depend on chrome/common/ assuming
> that chrome/ depends on remoting/.
>
> -Darin
>
>
> On Thu, Jun 24, 2010 at 11:11 AM, <ajwong@chromium.org> wrote:
>
>> Re-opening.
>>
>> Adding a dependency from chrome/common -> remoting causes a circular
>> dependency
>> in gyp which breaks mac.
>>
>> I'm going to try and track down why we're depending on chrome/common from
>> remoting.  But distasteful as it might be, maybe it'd be better to just
>> duplicate the registration code inside of both BrowserMain and
>> RendererMain.  Or
>> go with the statically initialized linked list of internal plugins
>> mentioned
>> earlier...
>>
>> -Albert
>>
>>
>> On 2010/06/24 00:16:58, darin wrote:
>>
>>  On Wed, Jun 23, 2010 at 4:44 PM, <mailto:jam@chromium.org> wrote:
>>>
>>
>>  >
>>> > http://codereview.chromium.org/2843018/diff/37001/25014
>>> >
>>> > File chrome/common/pepper_plugin_registry.cc (right):
>>> >
>>> > http://codereview.chromium.org/2843018/diff/37001/25014#newcode85
>>> > chrome/common/pepper_plugin_registry.cc:85:
>>> > info.entry_points.shutdown_module = remoting::PPP_ShutdownModule;
>>> > are these internal plugins shown in about:plugins?  If not, they should
>>> > be :)  If they are, then how are we getting the plugin name and
>>> > description?
>>> >
>>> >
>>> Yeah, I hooked up about:plugins support previously.  There is not a great
>>> amount of data shown in about:plugins.
>>>
>>
>>  See PluginService::RegisterPepperPlugins() in plugin_service.cc
>>>
>>
>>  -Darin
>>>
>>
>>
>>
>>
>>
>>  >
>>> > http://codereview.chromium.org/2843018/show
>>> >
>>>
>>
>>
>>
>>
>> http://codereview.chromium.org/2843018/show
>>
>
>

[darin (slow to review), 2010-06-24 18:38:31]

Please also add an include_rules entry to your DEPS file so that check_deps
will complain if anyone adds these bad dependencies again.

-Darin


On Thu, Jun 24, 2010 at 11:29 AM, Sergey Ulanov <sergeyu@chromium.org>wrote:

> We use JsonPrefStore from src/chrome/common. It needs to moved to src/base,
> and then we can remove chrome/common dependency.
>
>
> On Thu, Jun 24, 2010 at 11:12 AM, Darin Fisher <darin@chromium.org> wrote:
>
>> It seems incorrect for remoting/ to depend on chrome/common/ assuming
>> that chrome/ depends on remoting/.
>>
>> -Darin
>>
>>
>> On Thu, Jun 24, 2010 at 11:11 AM, <ajwong@chromium.org> wrote:
>>
>>> Re-opening.
>>>
>>> Adding a dependency from chrome/common -> remoting causes a circular
>>> dependency
>>> in gyp which breaks mac.
>>>
>>> I'm going to try and track down why we're depending on chrome/common from
>>> remoting.  But distasteful as it might be, maybe it'd be better to just
>>> duplicate the registration code inside of both BrowserMain and
>>> RendererMain.  Or
>>> go with the statically initialized linked list of internal plugins
>>> mentioned
>>> earlier...
>>>
>>> -Albert
>>>
>>>
>>> On 2010/06/24 00:16:58, darin wrote:
>>>
>>>  On Wed, Jun 23, 2010 at 4:44 PM, <mailto:jam@chromium.org> wrote:
>>>>
>>>
>>>  >
>>>> > http://codereview.chromium.org/2843018/diff/37001/25014
>>>> >
>>>> > File chrome/common/pepper_plugin_registry.cc (right):
>>>> >
>>>> > http://codereview.chromium.org/2843018/diff/37001/25014#newcode85
>>>> > chrome/common/pepper_plugin_registry.cc:85:
>>>> > info.entry_points.shutdown_module = remoting::PPP_ShutdownModule;
>>>> > are these internal plugins shown in about:plugins?  If not, they
>>>> should
>>>> > be :)  If they are, then how are we getting the plugin name and
>>>> > description?
>>>> >
>>>> >
>>>> Yeah, I hooked up about:plugins support previously.  There is not a
>>>> great
>>>> amount of data shown in about:plugins.
>>>>
>>>
>>>  See PluginService::RegisterPepperPlugins() in plugin_service.cc
>>>>
>>>
>>>  -Darin
>>>>
>>>
>>>
>>>
>>>
>>>
>>>  >
>>>> > http://codereview.chromium.org/2843018/show
>>>> >
>>>>
>>>
>>>
>>>
>>>
>>> http://codereview.chromium.org/2843018/show
>>>
>>
>>
>

[darin (slow to review), 2010-06-24 18:40:30]

JsonPrefStore seems to depend on quite a few other things in chrome/common/.
 I haven't studied those enough to determine if they should really be moved
into base/.  This would probably make a good post to chromium-dev as there
may be others with strong opinions about where this code should live.

-Darin


On Thu, Jun 24, 2010 at 11:38 AM, Darin Fisher <darin@chromium.org> wrote:

> Please also add an include_rules entry to your DEPS file so that check_deps
> will complain if anyone adds these bad dependencies again.
>
> -Darin
>
>
> On Thu, Jun 24, 2010 at 11:29 AM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>
>> We use JsonPrefStore from src/chrome/common. It needs to moved to
>> src/base, and then we can remove chrome/common dependency.
>>
>>
>> On Thu, Jun 24, 2010 at 11:12 AM, Darin Fisher <darin@chromium.org>wrote:
>>
>>> It seems incorrect for remoting/ to depend on chrome/common/ assuming
>>> that chrome/ depends on remoting/.
>>>
>>> -Darin
>>>
>>>
>>> On Thu, Jun 24, 2010 at 11:11 AM, <ajwong@chromium.org> wrote:
>>>
>>>> Re-opening.
>>>>
>>>> Adding a dependency from chrome/common -> remoting causes a circular
>>>> dependency
>>>> in gyp which breaks mac.
>>>>
>>>> I'm going to try and track down why we're depending on chrome/common
>>>> from
>>>> remoting.  But distasteful as it might be, maybe it'd be better to just
>>>> duplicate the registration code inside of both BrowserMain and
>>>> RendererMain.  Or
>>>> go with the statically initialized linked list of internal plugins
>>>> mentioned
>>>> earlier...
>>>>
>>>> -Albert
>>>>
>>>>
>>>> On 2010/06/24 00:16:58, darin wrote:
>>>>
>>>>  On Wed, Jun 23, 2010 at 4:44 PM, <mailto:jam@chromium.org> wrote:
>>>>>
>>>>
>>>>  >
>>>>> > http://codereview.chromium.org/2843018/diff/37001/25014
>>>>> >
>>>>> > File chrome/common/pepper_plugin_registry.cc (right):
>>>>> >
>>>>> > http://codereview.chromium.org/2843018/diff/37001/25014#newcode85
>>>>> > chrome/common/pepper_plugin_registry.cc:85:
>>>>> > info.entry_points.shutdown_module = remoting::PPP_ShutdownModule;
>>>>> > are these internal plugins shown in about:plugins?  If not, they
>>>>> should
>>>>> > be :)  If they are, then how are we getting the plugin name and
>>>>> > description?
>>>>> >
>>>>> >
>>>>> Yeah, I hooked up about:plugins support previously.  There is not a
>>>>> great
>>>>> amount of data shown in about:plugins.
>>>>>
>>>>
>>>>  See PluginService::RegisterPepperPlugins() in plugin_service.cc
>>>>>
>>>>
>>>>  -Darin
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>  >
>>>>> > http://codereview.chromium.org/2843018/show
>>>>> >
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>> http://codereview.chromium.org/2843018/show
>>>>
>>>
>>>
>>
>

[Sergey Ulanov, 2010-06-25 00:47:00]

I fixed remoting code so that it doesn't depend on JsonPrefStore and uses
base/json instead: http://codereview.chromium.org/2819026/show

On Thu, Jun 24, 2010 at 11:40 AM, Darin Fisher <darin@chromium.org> wrote:

> JsonPrefStore seems to depend on quite a few other things in
> chrome/common/.  I haven't studied those enough to determine if they should
> really be moved into base/.  This would probably make a good post to
> chromium-dev as there may be others with strong opinions about where this
> code should live.
>
> -Darin
>
>
> On Thu, Jun 24, 2010 at 11:38 AM, Darin Fisher <darin@chromium.org> wrote:
>
>> Please also add an include_rules entry to your DEPS file so that
>> check_deps will complain if anyone adds these bad dependencies again.
>>
>> -Darin
>>
>>
>> On Thu, Jun 24, 2010 at 11:29 AM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>>
>>> We use JsonPrefStore from src/chrome/common. It needs to moved to
>>> src/base, and then we can remove chrome/common dependency.
>>>
>>>
>>> On Thu, Jun 24, 2010 at 11:12 AM, Darin Fisher <darin@chromium.org>wrote:
>>>
>>>> It seems incorrect for remoting/ to depend on chrome/common/ assuming
>>>> that chrome/ depends on remoting/.
>>>>
>>>> -Darin
>>>>
>>>>
>>>> On Thu, Jun 24, 2010 at 11:11 AM, <ajwong@chromium.org> wrote:
>>>>
>>>>> Re-opening.
>>>>>
>>>>> Adding a dependency from chrome/common -> remoting causes a circular
>>>>> dependency
>>>>> in gyp which breaks mac.
>>>>>
>>>>> I'm going to try and track down why we're depending on chrome/common
>>>>> from
>>>>> remoting.  But distasteful as it might be, maybe it'd be better to just
>>>>> duplicate the registration code inside of both BrowserMain and
>>>>> RendererMain.  Or
>>>>> go with the statically initialized linked list of internal plugins
>>>>> mentioned
>>>>> earlier...
>>>>>
>>>>> -Albert
>>>>>
>>>>>
>>>>> On 2010/06/24 00:16:58, darin wrote:
>>>>>
>>>>>  On Wed, Jun 23, 2010 at 4:44 PM, <mailto:jam@chromium.org> wrote:
>>>>>>
>>>>>
>>>>>  >
>>>>>> > http://codereview.chromium.org/2843018/diff/37001/25014
>>>>>> >
>>>>>> > File chrome/common/pepper_plugin_registry.cc (right):
>>>>>> >
>>>>>> > http://codereview.chromium.org/2843018/diff/37001/25014#newcode85
>>>>>> > chrome/common/pepper_plugin_registry.cc:85:
>>>>>> > info.entry_points.shutdown_module = remoting::PPP_ShutdownModule;
>>>>>> > are these internal plugins shown in about:plugins?  If not, they
>>>>>> should
>>>>>> > be :)  If they are, then how are we getting the plugin name and
>>>>>> > description?
>>>>>> >
>>>>>> >
>>>>>> Yeah, I hooked up about:plugins support previously.  There is not a
>>>>>> great
>>>>>> amount of data shown in about:plugins.
>>>>>>
>>>>>
>>>>>  See PluginService::RegisterPepperPlugins() in plugin_service.cc
>>>>>>
>>>>>
>>>>>  -Darin
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>  >
>>>>>> > http://codereview.chromium.org/2843018/show
>>>>>> >
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> http://codereview.chromium.org/2843018/show
>>>>>
>>>>
>>>>
>>>
>>
>