Remove PluginGroup

Remove PluginGroup

Depends on this CL: http://codereview.chromium.org/10917189/

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

[Bernhard Bauer, 2012-09-17 15:18:00]

http://codereview.chromium.org/10918174/diff/1/webkit/plugins/npapi/plugin_li...
File webkit/plugins/npapi/plugin_list.cc (right):

http://codereview.chromium.org/10918174/diff/1/webkit/plugins/npapi/plugin_li...
webkit/plugins/npapi/plugin_list.cc:215: dont_load_new_wmp_(false),
Wait, are we not initializing this member in the default constructor?

http://codereview.chromium.org/10918174/diff/1/webkit/plugins/npapi/plugin_li...
File webkit/plugins/npapi/plugin_list.h (right):

http://codereview.chromium.org/10918174/diff/1/webkit/plugins/npapi/plugin_li...
webkit/plugins/npapi/plugin_list.h:172: explicit PluginList(int
skip_platform_init);
Ugh. Could you use custom traits on |g_singleton| to do the platform-specific
initialization for the singleton but keep an empty protected constructor for
subclasses?

If that doesn't work, we should at least use a custom enum instead of an int.

[ibraaaa, 2012-09-17 15:44:35]

https://chromiumcodereview.appspot.com/10918174/diff/1/webkit/plugins/npapi/p...
File webkit/plugins/npapi/plugin_list.cc (right):

https://chromiumcodereview.appspot.com/10918174/diff/1/webkit/plugins/npapi/p...
webkit/plugins/npapi/plugin_list.cc:215: dont_load_new_wmp_(false),
Well, this CL only added it here:
https://chromiumcodereview.appspot.com/10833013/diff/6003/webkit/plugins/npap...

On 2012/09/17 15:18:01, Bernhard Bauer wrote:
> Wait, are we not initializing this member in the default constructor?

[Bernhard Bauer, 2012-09-17 16:00:53]

https://chromiumcodereview.appspot.com/10918174/diff/1/webkit/plugins/npapi/p...
File webkit/plugins/npapi/plugin_list.cc (right):

https://chromiumcodereview.appspot.com/10918174/diff/1/webkit/plugins/npapi/p...
webkit/plugins/npapi/plugin_list.cc:215: dont_load_new_wmp_(false),
On 2012/09/17 15:44:35, ibraaaa wrote:
> Well, this CL only added it here:
>
https://chromiumcodereview.appspot.com/10833013/diff/6003/webkit/plugins/npap...

Yup, that seems wrong. Could you also initialize it in the default constructor?

[ibraaaa, 2012-09-17 16:21:34]

http://codereview.chromium.org/10918174/diff/1/webkit/plugins/npapi/plugin_li...
File webkit/plugins/npapi/plugin_list.cc (right):

http://codereview.chromium.org/10918174/diff/1/webkit/plugins/npapi/plugin_li...
webkit/plugins/npapi/plugin_list.cc:215: dont_load_new_wmp_(false),
Sent seperate CL for this: http://codereview.chromium.org/10911338/

On 2012/09/17 16:00:54, Bernhard Bauer wrote:
> On 2012/09/17 15:44:35, ibraaaa wrote:
> > Well, this CL only added it here:
> >
>
https://chromiumcodereview.appspot.com/10833013/diff/6003/webkit/plugins/npap...
> 
> Yup, that seems wrong. Could you also initialize it in the default
constructor?

[ibraaaa, 2012-09-17 17:53:09]

http://codereview.chromium.org/10918174/diff/1/webkit/plugins/npapi/plugin_li...
File webkit/plugins/npapi/plugin_list.h (right):

http://codereview.chromium.org/10918174/diff/1/webkit/plugins/npapi/plugin_li...
webkit/plugins/npapi/plugin_list.h:172: explicit PluginList(int
skip_platform_init);
On 2012/09/17 15:18:01, Bernhard Bauer wrote:
> Ugh. Could you use custom traits on |g_singleton| to do the platform-specific
> initialization for the singleton but keep an empty protected constructor for
> subclasses?
> 
> If that doesn't work, we should at least use a custom enum instead of an int.

Done.

[Bernhard Bauer, 2012-09-17 18:51:52]

Did the other suggestion (use custom LazyInstanceTraits) not work? I'd
prefer that, as it avoids overloading the constructor.
On Sep 17, 2012 1:53 PM, <ibraaaa@google.com> wrote:

>
> http://codereview.chromium.**org/10918174/diff/1/webkit/**
>
plugins/npapi/plugin_list.h<http://codereview.chromium.org/10918174/diff/1/webkit/plugins/npapi/plugin_list.h>
> File webkit/plugins/npapi/plugin_**list.h (right):
>
> http://codereview.chromium.**org/10918174/diff/1/webkit/**
>
plugins/npapi/plugin_list.h#**newcode172<http://codereview.chromium.org/10918174/diff/1/webkit/plugins/npapi/plugin_list.h#newcode172>
> webkit/plugins/npapi/plugin_**list.h:172: explicit PluginList(int
> skip_platform_init);
> On 2012/09/17 15:18:01, Bernhard Bauer wrote:
>
>> Ugh. Could you use custom traits on |g_singleton| to do the
>>
> platform-specific
>
>> initialization for the singleton but keep an empty protected
>>
> constructor for
>
>> subclasses?
>>
>
>  If that doesn't work, we should at least use a custom enum instead of
>>
> an int.
>
> Done.
>
>
http://codereview.chromium.**org/10918174/<http://codereview.chromium.org/109...
>

[ibraaaa, 2012-09-18 09:47:13]

I guess it works, I chose the easier way but since you have preferences. Please
take a look, thanks.

http://codereview.chromium.org/10918174/diff/14002/webkit/plugins/npapi/plugi...
File webkit/plugins/npapi/plugin_list.h (right):

http://codereview.chromium.org/10918174/diff/14002/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:276: struct CustomPluginListTraits {
Is it better to define this inside PluginList?

[Bernhard Bauer, 2012-09-18 11:01:31]

http://codereview.chromium.org/10918174/diff/14002/webkit/plugins/npapi/plugi...
File webkit/plugins/npapi/plugin_list.h (right):

http://codereview.chromium.org/10918174/diff/14002/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:274: // Custom traits for PlugList that
performs platform-dependent initialization
Nit: PluginList

http://codereview.chromium.org/10918174/diff/14002/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:276: struct CustomPluginListTraits {
On 2012/09/18 09:47:13, ibraaaa wrote:
> Is it better to define this inside PluginList?

Hm. Yeah, it's probably best to move the declaration into PluginList (and then
call it (Custom)?LazyInstanceTraits or something, because the "PluginList" is
already implicit), and move the definition to the .cc file.

http://codereview.chromium.org/10918174/diff/14002/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:287: static void Delete(PluginList* instance)
{
If you inherited from base::DefaultLazyInstanceTraits, you'd inherit the Delete
method (and the constants above).

[ibraaaa, 2012-09-18 14:49:01]

http://codereview.chromium.org/10918174/diff/14002/webkit/plugins/npapi/plugi...
File webkit/plugins/npapi/plugin_list.h (right):

http://codereview.chromium.org/10918174/diff/14002/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:274: // Custom traits for PlugList that
performs platform-dependent initialization
On 2012/09/18 11:01:31, Bernhard Bauer wrote:
> Nit: PluginList

Done.

http://codereview.chromium.org/10918174/diff/14002/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:276: struct CustomPluginListTraits {
On 2012/09/18 11:01:31, Bernhard Bauer wrote:
> On 2012/09/18 09:47:13, ibraaaa wrote:
> > Is it better to define this inside PluginList?
> 
> Hm. Yeah, it's probably best to move the declaration into PluginList (and then
> call it (Custom)?LazyInstanceTraits or something, because the "PluginList" is
> already implicit), and move the definition to the .cc file.

Done.

http://codereview.chromium.org/10918174/diff/14002/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:287: static void Delete(PluginList* instance)
{
On 2012/09/18 11:01:31, Bernhard Bauer wrote:
> If you inherited from base::DefaultLazyInstanceTraits, you'd inherit the
Delete
> method (and the constants above).

Done.

[Bernhard Bauer, 2012-09-20 17:03:00]

http://codereview.chromium.org/10918174/diff/21004/webkit/plugins/npapi/plugi...
File webkit/plugins/npapi/plugin_list.h (right):

http://codereview.chromium.org/10918174/diff/21004/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:53: struct CustomLazyInstanceTraits
I think you could forward-declare the struct, move it to the private section,
and move the class definition to the .cc file as well?

[ibraaaa, 2012-09-21 09:24:56]

http://codereview.chromium.org/10918174/diff/21004/webkit/plugins/npapi/plugi...
File webkit/plugins/npapi/plugin_list.h (right):

http://codereview.chromium.org/10918174/diff/21004/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:53: struct CustomLazyInstanceTraits
If I understand correctly, then you mean:

.h file:

private:
 struct CustomLazyInstanceTraits;

.cc file

struct PluginList::CustomLazyInstanceTraits {
.....
}

if this is right, then I guess CustomLazyInstanceTraits will not be visible
anymore? 

On 2012/09/20 17:03:00, Bernhard Bauer wrote:
> I think you could forward-declare the struct, move it to the private section,
> and move the class definition to the .cc file as well?

[Bernhard Bauer, 2012-09-21 09:33:52]

http://codereview.chromium.org/10918174/diff/21004/webkit/plugins/npapi/plugi...
File webkit/plugins/npapi/plugin_list.h (right):

http://codereview.chromium.org/10918174/diff/21004/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:53: struct CustomLazyInstanceTraits
On 2012/09/21 09:24:57, ibraaaa wrote:
> If I understand correctly, then you mean:
> 
> .h file:
> 
> private:
>  struct CustomLazyInstanceTraits;
> 
> .cc file
> 
> struct PluginList::CustomLazyInstanceTraits {
> .....
> }
> 
> if this is right, then I guess CustomLazyInstanceTraits will not be visible
> anymore? 

Yes, it will only be forward-declared for other classes, but that's okay,
because it's only being used by the LazyInstance in the .cc file, which isn't
externally visible either, right?

> On 2012/09/20 17:03:00, Bernhard Bauer wrote:
> > I think you could forward-declare the struct, move it to the private
section,
> > and move the class definition to the .cc file as well?
>

[ibraaaa, 2012-09-21 09:37:55]

http://codereview.chromium.org/10918174/diff/21004/webkit/plugins/npapi/plugi...
File webkit/plugins/npapi/plugin_list.h (right):

http://codereview.chromium.org/10918174/diff/21004/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:53: struct CustomLazyInstanceTraits
yes but the LazyInstance in the .cc file is declared in an anonymous namespace
and it needs as part of its declaration 
LazyInstance<PluginList, PluginList::CustomLazyInstanceTraits> g_singleton =...;

if CustomLazyInstanceTraits is not visible then this won't work?

On 2012/09/21 09:33:52, Bernhard Bauer wrote:
> On 2012/09/21 09:24:57, ibraaaa wrote:
> > If I understand correctly, then you mean:
> > 
> > .h file:
> > 
> > private:
> >  struct CustomLazyInstanceTraits;
> > 
> > .cc file
> > 
> > struct PluginList::CustomLazyInstanceTraits {
> > .....
> > }
> > 
> > if this is right, then I guess CustomLazyInstanceTraits will not be visible
> > anymore? 
> 
> Yes, it will only be forward-declared for other classes, but that's okay,
> because it's only being used by the LazyInstance in the .cc file, which isn't
> externally visible either, right?
> 
> > On 2012/09/20 17:03:00, Bernhard Bauer wrote:
> > > I think you could forward-declare the struct, move it to the private
> section,
> > > and move the class definition to the .cc file as well?
> > 
>

[Bernhard Bauer, 2012-09-21 09:53:28]

http://codereview.chromium.org/10918174/diff/21004/webkit/plugins/npapi/plugi...
File webkit/plugins/npapi/plugin_list.h (right):

http://codereview.chromium.org/10918174/diff/21004/webkit/plugins/npapi/plugi...
webkit/plugins/npapi/plugin_list.h:53: struct CustomLazyInstanceTraits
On 2012/09/21 09:37:56, ibraaaa wrote:
> yes but the LazyInstance in the .cc file is declared in an anonymous namespace
> and it needs as part of its declaration 
> LazyInstance<PluginList, PluginList::CustomLazyInstanceTraits> g_singleton
=...;
> 
> if CustomLazyInstanceTraits is not visible then this won't work?

Ah, you meant visible in the 'public'/'private' sense? Right, we might need to
keep the declaration in the public section.

> On 2012/09/21 09:33:52, Bernhard Bauer wrote:
> > On 2012/09/21 09:24:57, ibraaaa wrote:
> > > If I understand correctly, then you mean:
> > > 
> > > .h file:
> > > 
> > > private:
> > >  struct CustomLazyInstanceTraits;
> > > 
> > > .cc file
> > > 
> > > struct PluginList::CustomLazyInstanceTraits {
> > > .....
> > > }
> > > 
> > > if this is right, then I guess CustomLazyInstanceTraits will not be
visible
> > > anymore? 
> > 
> > Yes, it will only be forward-declared for other classes, but that's okay,
> > because it's only being used by the LazyInstance in the .cc file, which
isn't
> > externally visible either, right?
> > 
> > > On 2012/09/20 17:03:00, Bernhard Bauer wrote:
> > > > I think you could forward-declare the struct, move it to the private
> > section,
> > > > and move the class definition to the .cc file as well?
> > > 
> > 
>

[Bernhard Bauer, 2012-09-21 14:02:40]

LGTM!

[ibraaaa, 2012-09-21 15:56:54]

Hi John & James,

Can I have an OWNERS review for this CL?

Specifically,
-John for content/
-James for chrome/

[jam, 2012-09-21 16:04:15]

content lgtm

[ibraaaa, 2012-09-25 17:56:30]

Hi James,

Can I have an OWNERS review for this CL?

Specifically for chrome/browser (except chrome/browser/plugins/)

Thanks!

[James Hawkins, 2012-09-25 18:00:20]

chrome/ LGTM

[commit-bot: I haz the power, 2012-09-26 15:09:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10918174/44001

[commit-bot: I haz the power, 2012-09-26 15:09:43]

Failed to apply patch for chrome/browser/plugins/plugin_infobar_delegates.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/plugins/plugin_infobar_delegates.cc
Hunk #1 FAILED at 9.
Hunk #2 succeeded at 20 (offset -1 lines).
Hunk #3 succeeded at 83 (offset -1 lines).
Hunk #4 succeeded at 183 (offset -2 lines).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/plugins/plugin_infobar_delegates.cc.rej

[commit-bot: I haz the power, 2012-09-26 16:57:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10918174/51001

[commit-bot: I haz the power, 2012-09-26 16:57:39]

Presubmit check for 10918174-51001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit

Presubmit checks took 2.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[ibraaaa, 2012-09-26 17:10:15]

Hi James,

Can I have an OWNERS review for the file webkit/glue/webkit_glue.gypi ?

Thanks!

[James Hawkins, 2012-09-26 17:11:19]

I already LGed, but that doesn't matter for this file since I'm not an owner of
webkit/.

[James Hawkins, 2012-09-26 17:11:34]

On 2012/09/26 17:11:19, James Hawkins wrote:
> I already LGed, but that doesn't matter for this file since I'm not an owner
of
> webkit/.

And now I see that you're probably talking to jamesr.

[ibraaaa, 2012-09-26 17:13:14]

Yes, James Robinson :)

On Wed, Sep 26, 2012 at 7:11 PM, <jhawkins@chromium.org> wrote:

> On 2012/09/26 17:11:19, James Hawkins wrote:
>
>> I already LGed, but that doesn't matter for this file since I'm not an
>> owner
>>
> of
>
>> webkit/.
>>
>
> And now I see that you're probably talking to jamesr.
>
>
http://codereview.chromium.**org/10918174/<http://codereview.chromium.org/109...
>

[jamesr, 2012-09-26 17:42:10]

webkit/ lgtm

[commit-bot: I haz the power, 2012-09-26 17:53:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10918174/51001

[commit-bot: I haz the power, 2012-09-26 18:11:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-26 18:18:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10918174/51003

[commit-bot: I haz the power, 2012-09-26 19:51:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10918174/51003

[commit-bot: I haz the power, 2012-09-26 20:14:23]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-27 08:06:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10918174/54007

[commit-bot: I haz the power, 2012-09-27 13:10:13]

Failed to apply patch for chrome/browser/chrome_plugin_service_filter.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/chrome_plugin_service_filter.cc
Hunk #3 FAILED at 85.
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chrome_plugin_service_filter.cc.rej

[commit-bot: I haz the power, 2012-09-27 13:23:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ibraaaa@google.com/10918174/47007

[commit-bot: I haz the power, 2012-09-27 16:11:55]

Change committed as 159037