The chrome browser target should not depend on plugins.

The chrome browser target should not depend on plugins. 

This in turn brings with it a dependency on webkit_glue which in turn brings in webkit.
We should only depend on plugins_common in the browser. 

The pepper component updater for flash is the only component in the browser which depends
on webkit\plugins\ppapi. This is to check if the interface name passed in is supported by the browser.

I added a function IsSupportedPepperInterface to the newly added ppapi_utils.cc/.h file which lives in
webkit/common/plugins/ppapi

Added a macro LEGACY_IFACE to the newly added ppapi/thunk/interfaces_legacy.h file. This macro consolidates
the list of interfaces being checked for on the browser. It is used in plugin_module.cc and ppapi_utils.cc

BUG=237249
TBR=cpu@chromium.org, jamesr@chromium.org, raymes@chromium.org, yzshen@chromium.org

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

[jamesr, 2013-05-20 23:42:30]

lgtm

https://codereview.chromium.org/15505004/diff/1/webkit/common/plugins/ppapi/p...
File webkit/common/plugins/ppapi/ppapi_utils.cc (right):

https://codereview.chromium.org/15505004/diff/1/webkit/common/plugins/ppapi/p...
webkit/common/plugins/ppapi/ppapi_utils.cc:24: if (strcmp(name,
PPB_AUDIO_TRUSTED_INTERFACE_0_6) == 0)
think you need #include <cstring> for this to compile everywhere

https://codereview.chromium.org/15505004/diff/1/webkit/plugins/webkit_plugins...
File webkit/plugins/webkit_plugins.gypi (right):

https://codereview.chromium.org/15505004/diff/1/webkit/plugins/webkit_plugins...
webkit/plugins/webkit_plugins.gypi:42: '../common/plugins/ppapi/ppapi_utils.cc',
is the plan to move the rest of the files referenced in this target into
../common/plugins/ as well?

[ananta, 2013-05-20 23:49:20]

https://codereview.chromium.org/15505004/diff/1/webkit/plugins/webkit_plugins...
File webkit/plugins/webkit_plugins.gypi (right):

https://codereview.chromium.org/15505004/diff/1/webkit/plugins/webkit_plugins...
webkit/plugins/webkit_plugins.gypi:42: '../common/plugins/ppapi/ppapi_utils.cc',
On 2013/05/20 23:42:30, jamesr wrote:
> is the plan to move the rest of the files referenced in this target into
> ../common/plugins/ as well?
Eventually yes. Would be a big change though. Will do it probably on a Friday

[raymes1, 2013-05-21 15:46:29]

https://codereview.chromium.org/15505004/diff/1/webkit/common/plugins/ppapi/p...
File webkit/common/plugins/ppapi/ppapi_utils.cc (right):

https://codereview.chromium.org/15505004/diff/1/webkit/common/plugins/ppapi/p...
webkit/common/plugins/ppapi/ppapi_utils.cc:21: bool
IsSupportedPepperInterface(const char* name) {
I misunderstood slightly what you meant. This won't work completely because you
still need the part which says:

  // TODO(brettw) put these in a hash map for better performance.
  #define UNPROXIED_IFACE(api_name, iface_str, iface_struct) \
      if (strcmp(name, iface_str) == 0) \
        return ::ppapi::thunk::Get##iface_struct##_Thunk();
  #define PROXIED_IFACE(api_name, iface_str, iface_struct) \
      UNPROXIED_IFACE(api_name, iface_str, iface_struct)

  #include "ppapi/thunk/interfaces_ppb_public_stable.h"
  #include "ppapi/thunk/interfaces_ppb_public_dev.h"
  #include "ppapi/thunk/interfaces_ppb_private.h"
  #include "ppapi/thunk/interfaces_ppb_private_no_permissions.h"
  #include "ppapi/thunk/interfaces_ppb_private_flash.h"

  #undef UNPROXIED_API
  #undef PROXIED_IFACE

Is there a problem including that part?

Also if we move this stuff, PluginModule::InternalGetInterface() should call
into this function to avoid code duplication.

[ananta, 2013-05-21 19:20:37]

https://codereview.chromium.org/15505004/diff/1/webkit/common/plugins/ppapi/p...
File webkit/common/plugins/ppapi/ppapi_utils.cc (right):

https://codereview.chromium.org/15505004/diff/1/webkit/common/plugins/ppapi/p...
webkit/common/plugins/ppapi/ppapi_utils.cc:21: bool
IsSupportedPepperInterface(const char* name) {
On 2013/05/21 15:46:29, raymes1 wrote:
> I misunderstood slightly what you meant. This won't work completely because
you
> still need the part which says:
> 
>   // TODO(brettw) put these in a hash map for better performance.
>   #define UNPROXIED_IFACE(api_name, iface_str, iface_struct) \
>       if (strcmp(name, iface_str) == 0) \
>         return ::ppapi::thunk::Get##iface_struct##_Thunk();
>   #define PROXIED_IFACE(api_name, iface_str, iface_struct) \
>       UNPROXIED_IFACE(api_name, iface_str, iface_struct)
> 
>   #include "ppapi/thunk/interfaces_ppb_public_stable.h"
>   #include "ppapi/thunk/interfaces_ppb_public_dev.h"
>   #include "ppapi/thunk/interfaces_ppb_private.h"
>   #include "ppapi/thunk/interfaces_ppb_private_no_permissions.h"
>   #include "ppapi/thunk/interfaces_ppb_private_flash.h"
> 
>   #undef UNPROXIED_API
>   #undef PROXIED_IFACE
> 
> Is there a problem including that part?
> 
> Also if we move this stuff, PluginModule::InternalGetInterface() should call
> into this function to avoid code duplication.

I added the required section back. We cannot share the whole code sadly because
the InternalGetInterface function refers to structures and code inside
webkit/plugins/ppapi which won't work here. Added a TODO to this effect.

https://codereview.chromium.org/15505004/diff/1/webkit/common/plugins/ppapi/p...
webkit/common/plugins/ppapi/ppapi_utils.cc:24: if (strcmp(name,
PPB_AUDIO_TRUSTED_INTERFACE_0_6) == 0)
On 2013/05/20 23:42:30, jamesr wrote:
> think you need #include <cstring> for this to compile everywhere

Done.

[yzshen1, 2013-05-21 21:42:33]

https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
File webkit/common/plugins/ppapi/ppapi_utils.cc (right):

https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:5: #include <cstring>
nit: an empty line above line 5.

https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:7: #include "base/bind.h"
You don't need some of the includes.
Please remove them.

https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:175: // If you add additional
strcmp's here please add them to the
It seems this comment belongs to plugin_module.cc

https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
File webkit/common/plugins/ppapi/ppapi_utils.h (right):

https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.h:15: }  // namespace pepper
mismatched comment.

[ananta, 2013-05-21 22:21:15]

https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
File webkit/common/plugins/ppapi/ppapi_utils.cc (right):

https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:7: #include "base/bind.h"
On 2013/05/21 21:42:33, yzshen1 wrote:
> You don't need some of the includes.
> Please remove them.

Done.

https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:175: // If you add additional
strcmp's here please add them to the
On 2013/05/21 21:42:33, yzshen1 wrote:
> It seems this comment belongs to plugin_module.cc

Done.

https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
File webkit/common/plugins/ppapi/ppapi_utils.h (right):

https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.h:15: }  // namespace pepper
On 2013/05/21 21:42:33, yzshen1 wrote:
> mismatched comment.

Done.

[ananta, 2013-05-22 00:52:54]

On 2013/05/21 22:21:15, ananta wrote:
>
https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
> File webkit/common/plugins/ppapi/ppapi_utils.cc (right):
> 
>
https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
> webkit/common/plugins/ppapi/ppapi_utils.cc:7: #include "base/bind.h"
> On 2013/05/21 21:42:33, yzshen1 wrote:
> > You don't need some of the includes.
> > Please remove them.
> 
> Done.
> 
>
https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
> webkit/common/plugins/ppapi/ppapi_utils.cc:175: // If you add additional
> strcmp's here please add them to the
> On 2013/05/21 21:42:33, yzshen1 wrote:
> > It seems this comment belongs to plugin_module.cc
> 
> Done.
> 
>
https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
> File webkit/common/plugins/ppapi/ppapi_utils.h (right):
> 
>
https://codereview.chromium.org/15505004/diff/49001/webkit/common/plugins/ppa...
> webkit/common/plugins/ppapi/ppapi_utils.h:15: }  // namespace pepper
> On 2013/05/21 21:42:33, yzshen1 wrote:
> > mismatched comment.
> 
> Done.

Added a new header file ppapi/thunk/interfaces_legacy.h which contains the
LEGACY_IFACE macro along with the list of interfaces being checked for. This is
used from ppapi_utils.cc and plugin_module.cc as per your  suggestion.

[yzshen1, 2013-05-22 05:15:37]

LGTM with some nits.

Thanks for making this change!

https://codereview.chromium.org/15505004/diff/76001/ppapi/thunk/interfaces_le...
File ppapi/thunk/interfaces_legacy.h (right):

https://codereview.chromium.org/15505004/diff/76001/ppapi/thunk/interfaces_le...
ppapi/thunk/interfaces_legacy.h:4: #ifndef PPAPI_THUNK_INTERFACES_LEGACY_H_
nit: we don't need this #ifndef guard.

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
File webkit/common/plugins/ppapi/ppapi_utils.cc (right):

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:5: #include <cstring>
nit: please add an empty line above this C header file include.

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:96: #include
"ppapi/shared_impl/ppapi_globals.h"
I think this is not needed.

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:97: #include
"ppapi/shared_impl/ppb_opengles2_shared.h"
I think you could just include the corresponding C interface file (ppapi/c/*),
right?

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:117: #undef UNPROXIED_API
This one seems incorrect.
(I know it is not introduced by this CL. But please change it and the one in
plugin_module.cc now that you are here.)

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
File webkit/common/plugins/ppapi/ppapi_utils.h (right):

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.h:17: const void*
GetThunkForInterface(const char* name);
This declaration is not needed anymore.

[raymes, 2013-05-22 14:49:06]

lgtm with yzshen's comments.

Thanks for doing this.

[cpu_(ooo_6.6-7.5), 2013-05-22 17:43:16]

lgtm, please see if james has any further comments.

[ananta, 2013-05-22 17:56:43]

https://codereview.chromium.org/15505004/diff/76001/ppapi/thunk/interfaces_le...
File ppapi/thunk/interfaces_legacy.h (right):

https://codereview.chromium.org/15505004/diff/76001/ppapi/thunk/interfaces_le...
ppapi/thunk/interfaces_legacy.h:4: #ifndef PPAPI_THUNK_INTERFACES_LEGACY_H_
On 2013/05/22 05:15:37, yzshen1 wrote:
> nit: we don't need this #ifndef guard.

Done.

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
File webkit/common/plugins/ppapi/ppapi_utils.cc (right):

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:5: #include <cstring>
On 2013/05/22 05:15:37, yzshen1 wrote:
> nit: please add an empty line above this C header file include.

Done.

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:96: #include
"ppapi/shared_impl/ppapi_globals.h"
On 2013/05/22 05:15:37, yzshen1 wrote:
> I think this is not needed.

Done.

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:97: #include
"ppapi/shared_impl/ppb_opengles2_shared.h"
On 2013/05/22 05:15:37, yzshen1 wrote:
> I think you could just include the corresponding C interface file (ppapi/c/*),
> right?

Done.

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.cc:117: #undef UNPROXIED_API
On 2013/05/22 05:15:37, yzshen1 wrote:
> This one seems incorrect.
> (I know it is not introduced by this CL. But please change it and the one in
> plugin_module.cc now that you are here.)

Done.

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
File webkit/common/plugins/ppapi/ppapi_utils.h (right):

https://codereview.chromium.org/15505004/diff/76001/webkit/common/plugins/ppa...
webkit/common/plugins/ppapi/ppapi_utils.h:17: const void*
GetThunkForInterface(const char* name);
On 2013/05/22 05:15:37, yzshen1 wrote:
> This declaration is not needed anymore.

Done.

[jamesr, 2013-05-22 18:11:33]

lgtm

[ananta, 2013-05-22 21:46:02]

TBR'ing sky for chrome\browser owners

[ananta, 2013-05-22 22:38:41]

Committed patchset #9 manually as r201623.