PluginList: Unit tests and bugfixes

PluginList: Unit tests and bugfixes

This patch introduces unit tests for PluginList. Some refactoring/adapting was necessary to make that class testable. Also included are two bugfixes for the issues reported in bug 66505.

BUG=66505
TEST=unit tests: PluginListTest.*:PluginGroupTest.*; manual: check that issues reported and screenshotted in bug 66505 no longer occur

[Jakob Kummerow, 2010-12-14 15:45:00]

Please review. 

This is currently (as of patch set 1) based on issue 5699005 patch set 9, which
in turn is based on trunk rev 69124. I will rebase as necessary for landing.

[stuartmorgan, 2010-12-14 16:25:29]

http://codereview.chromium.org/5783005/diff/1/webkit/glue/plugins/plugin_grou...
File webkit/glue/plugins/plugin_group.cc (right):

http://codereview.chromium.org/5783005/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:121: DCHECK_EQ(0,
version_->CompareTo(*other.version_));
I'm confused; you still aren't actually copying version_, so how would this be
true?

[Jakob Kummerow, 2010-12-14 16:53:43]

http://codereview.chromium.org/5783005/diff/1/webkit/glue/plugins/plugin_grou...
File webkit/glue/plugins/plugin_group.cc (right):

http://codereview.chromium.org/5783005/diff/1/webkit/glue/plugins/plugin_grou...
webkit/glue/plugins/plugin_group.cc:121: DCHECK_EQ(0,
version_->CompareTo(*other.version_));
On 2010/12/14 16:25:29, stuartmorgan wrote:
> I'm confused; you still aren't actually copying version_, so how would this be
> true?

The call to AddPlugin(...) updates |version_|, provided that |enabled_| isn't
set yet (which is why it didn't work before).

[Bernhard Bauer, 2010-12-14 20:18:34]

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:233: friend class
plugin_test_internal::TestablePluginList;
Couldn't you make the necessary methods protected?

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:253: virtual void
PostInitPluginGroup(PluginGroup* group) { }
Nit: Maybe name it DidInitPluginGroup?

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list_unittest.cc (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:14: class TestablePluginList :
public NPAPI::PluginList {
Nit: MockPluginList or something.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:25: void LoadPlugins(bool refresh) {
Nit: Please add a comment "NPAPI::PluginList methods:" so that it's clear you're
overriding these methods. You might also want to declare them as virtual for
consistency.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:36: group->plugin_list_ = this;
Please use a setter and add a comment that this is only for unit tests.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:165:
EXPECT_GE(groups[i].GetPlugins().size(), size_t(1));
Nit: 1u instead of size_t(1)?

[danno, 2010-12-15 10:42:12]

here you go...

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_group.cc (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.cc:107: enabled_ = false;
Please comment why there enabled_ flag is set twice, or only set once.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.cc:130: PluginGroup&
PluginGroup::operator=(const PluginGroup& other) {
use the copy/swap idiom?

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.cc:443: NPAPI::PluginList*
PluginGroup::GetPluginList() {
I think you might be better served by making this a protected method that
returns the Singleton and created a subclass that mocks an override for the
test.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_group.h (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.h:115: 
following should be const method?

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:253: virtual void
PostInitPluginGroup(PluginGroup* group) { }
How about ProcessGroupAfterInitialize
On 2010/12/14 20:18:34, Bernhard Bauer wrote:
> Nit: Maybe name it DidInitPluginGroup?

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list_unittest.cc (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:12: // This class basically clones
PluginList, but without any filesystem IO. There
How about just: 
// A PluginList for tests that avoids file system IO. There...

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:14: class TestablePluginList :
public NPAPI::PluginList {
On 2010/12/14 20:18:34, Bernhard Bauer wrote:
> Nit: MockPluginList or something.
It's a bit more than a mock. The current name is a bit curious, however, since I
really hope that the PluginList is testable in the first place :-). How about
PluginListWithoutFileIO?

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:52: bool equals(const WebPluginInfo&
a, const WebPluginInfo& b,
Equals

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:61: bool contains(const
std::vector<WebPluginInfo>& list,
Contains

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:64: for (size_t i = 0; i <
list.size(); ++i) {
Nit: how about using container iterators here?

[Jakob Kummerow, 2010-12-15 18:03:27]

Addressed your comments. Please have another look.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_group.cc (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.cc:107: enabled_ = false;
On 2010/12/15 10:42:12, danno wrote:
> Please comment why there enabled_ flag is set twice, or only set once.

Done. Cleaned up this whole method, it now performs only copying operations.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.cc:130: PluginGroup&
PluginGroup::operator=(const PluginGroup& other) {
On 2010/12/15 10:42:12, danno wrote:
> use the copy/swap idiom?

As discussed offline: would be nice to have, but is not a priority right now.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.cc:443: NPAPI::PluginList*
PluginGroup::GetPluginList() {
On 2010/12/15 10:42:12, danno wrote:
> I think you might be better served by making this a protected method that
> returns the Singleton and created a subclass that mocks an override for the
> test.

As discussed offline: Good plan, but would require major restructuring of
PluginGroup/PluginList, and will therefore have to wait.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_group.h (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_group.h:115: 
On 2010/12/15 10:42:12, danno wrote:
> following should be const method?

Done.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:233: friend class
plugin_test_internal::TestablePluginList;
On 2010/12/14 20:18:34, Bernhard Bauer wrote:
> Couldn't you make the necessary methods protected?

Done.
I have to say, though, that I find it ugly to split the non-public
methods/members into separate protected/private sections based solely on the
needs of the test class. I'm happy to revert this if you agree that having the
test class as a friend is cleaner.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:253: virtual void
PostInitPluginGroup(PluginGroup* group) { }
On 2010/12/15 10:42:12, danno wrote:
> How about ProcessGroupAfterInitialize
> On 2010/12/14 20:18:34, Bernhard Bauer wrote:
> > Nit: Maybe name it DidInitPluginGroup?
> 

Done.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list_unittest.cc (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:12: // This class basically clones
PluginList, but without any filesystem IO. There
On 2010/12/15 10:42:12, danno wrote:
> How about just: 
> // A PluginList for tests that avoids file system IO. There...

Done.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:14: class TestablePluginList :
public NPAPI::PluginList {
On 2010/12/15 10:42:12, danno wrote:
> On 2010/12/14 20:18:34, Bernhard Bauer wrote:
> > Nit: MockPluginList or something.
> It's a bit more than a mock. The current name is a bit curious, however, since
I
> really hope that the PluginList is testable in the first place :-). How about
> PluginListWithoutFileIO?

Done.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:25: void LoadPlugins(bool refresh) {
On 2010/12/14 20:18:34, Bernhard Bauer wrote:
> Nit: Please add a comment "NPAPI::PluginList methods:" so that it's clear
you're
> overriding these methods. You might also want to declare them as virtual for
> consistency.

Done.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:36: group->plugin_list_ = this;
On 2010/12/14 20:18:34, Bernhard Bauer wrote:
> Please use a setter and add a comment that this is only for unit tests.

Done.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:52: bool equals(const WebPluginInfo&
a, const WebPluginInfo& b,
On 2010/12/15 10:42:12, danno wrote:
> Equals

Done.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:61: bool contains(const
std::vector<WebPluginInfo>& list,
On 2010/12/15 10:42:12, danno wrote:
> Contains

Done.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:64: for (size_t i = 0; i <
list.size(); ++i) {
On 2010/12/15 10:42:12, danno wrote:
> Nit: how about using container iterators here? 

Done.

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:165:
EXPECT_GE(groups[i].GetPlugins().size(), size_t(1));
On 2010/12/14 20:18:34, Bernhard Bauer wrote:
> Nit: 1u instead of size_t(1)?

Done.

[Bernhard Bauer, 2010-12-15 18:33:12]

On Wed, Dec 15, 2010 at 18:03,  <jkummerow@chromium.org> wrote:
> Addressed your comments. Please have another look.
>
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> File webkit/glue/plugins/plugin_group.cc (right):
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_group.cc:107: enabled_ = false;
> On 2010/12/15 10:42:12, danno wrote:
>>
>> Please comment why there enabled_ flag is set twice, or only set once.
>
> Done. Cleaned up this whole method, it now performs only copying
> operations.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_group.cc:130: PluginGroup&
> PluginGroup::operator=(const PluginGroup& other) {
> On 2010/12/15 10:42:12, danno wrote:
>>
>> use the copy/swap idiom?
>
> As discussed offline: would be nice to have, but is not a priority right
> now.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_group.cc:443: NPAPI::PluginList*
> PluginGroup::GetPluginList() {
> On 2010/12/15 10:42:12, danno wrote:
>>
>> I think you might be better served by making this a protected method
>
> that
>>
>> returns the Singleton and created a subclass that mocks an override
>
> for the
>>
>> test.
>
> As discussed offline: Good plan, but would require major restructuring
> of PluginGroup/PluginList, and will therefore have to wait.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> File webkit/glue/plugins/plugin_group.h (right):
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_group.h:115:
> On 2010/12/15 10:42:12, danno wrote:
>>
>> following should be const method?
>
> Done.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> File webkit/glue/plugins/plugin_list.h (right):
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_list.h:233: friend class
> plugin_test_internal::TestablePluginList;
> On 2010/12/14 20:18:34, Bernhard Bauer wrote:
>>
>> Couldn't you make the necessary methods protected?
>
> Done.
> I have to say, though, that I find it ugly to split the non-public
> methods/members into separate protected/private sections based solely on
> the needs of the test class. I'm happy to revert this if you agree that
> having the test class as a friend is cleaner.

It does add a bit more complexity to the header, but the added
complexity is in a part that's non-public anyway, and I'd say that
declaring the test class as a friend is basically equivalent to
exposing all private methods to it, whereas this only exposes some
methods, so this way the class is still a bit more encapsulated.

>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_list.h:253: virtual void
> PostInitPluginGroup(PluginGroup* group) { }
> On 2010/12/15 10:42:12, danno wrote:
>>
>> How about ProcessGroupAfterInitialize
>> On 2010/12/14 20:18:34, Bernhard Bauer wrote:
>> > Nit: Maybe name it DidInitPluginGroup?
>
>
> Done.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> File webkit/glue/plugins/plugin_list_unittest.cc (right):
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_list_unittest.cc:12: // This class basically
> clones PluginList, but without any filesystem IO. There
> On 2010/12/15 10:42:12, danno wrote:
>>
>> How about just:
>> // A PluginList for tests that avoids file system IO. There...
>
> Done.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_list_unittest.cc:14: class TestablePluginList
> : public NPAPI::PluginList {
> On 2010/12/15 10:42:12, danno wrote:
>>
>> On 2010/12/14 20:18:34, Bernhard Bauer wrote:
>> > Nit: MockPluginList or something.
>> It's a bit more than a mock. The current name is a bit curious,
>
> however, since I
>>
>> really hope that the PluginList is testable in the first place :-).
>
> How about
>>
>> PluginListWithoutFileIO?
>
> Done.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_list_unittest.cc:25: void LoadPlugins(bool
> refresh) {
> On 2010/12/14 20:18:34, Bernhard Bauer wrote:
>>
>> Nit: Please add a comment "NPAPI::PluginList methods:" so that it's
>
> clear you're
>>
>> overriding these methods. You might also want to declare them as
>
> virtual for
>>
>> consistency.
>
> Done.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_list_unittest.cc:36: group->plugin_list_ =
> this;
> On 2010/12/14 20:18:34, Bernhard Bauer wrote:
>>
>> Please use a setter and add a comment that this is only for unit
>
> tests.
>
> Done.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_list_unittest.cc:52: bool equals(const
> WebPluginInfo& a, const WebPluginInfo& b,
> On 2010/12/15 10:42:12, danno wrote:
>>
>> Equals
>
> Done.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_list_unittest.cc:61: bool contains(const
> std::vector<WebPluginInfo>& list,
> On 2010/12/15 10:42:12, danno wrote:
>>
>> Contains
>
> Done.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_list_unittest.cc:64: for (size_t i = 0; i <
> list.size(); ++i) {
> On 2010/12/15 10:42:12, danno wrote:
>>
>> Nit: how about using container iterators here?
>
> Done.
>
>
http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
> webkit/glue/plugins/plugin_list_unittest.cc:165:
> EXPECT_GE(groups[i].GetPlugins().size(), size_t(1));
> On 2010/12/14 20:18:34, Bernhard Bauer wrote:
>>
>> Nit: 1u instead of size_t(1)?
>
> Done.
>
> http://codereview.chromium.org/5783005/
>

[jam, 2010-12-15 19:41:57]

http://codereview.chromium.org/5783005/diff/19001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/5783005/diff/19001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:234: // Constructors are private for
singletons.
nit: this comment doesn't make sense now

http://codereview.chromium.org/5783005/diff/19001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:254: std::vector<WebPluginInfo*> plugins_;
i actually don't like moving member variables to be protected so that test
classes can derive from this object and access them.  this isn't a pattern that
i've seen in our tests, and breaks encapsulation since now non-test classes can
just derive from pluginlist and modify the private members.  you can just have
protected getters for these objects if you want to inspect them.

[jam, 2010-12-15 19:43:39]

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list.h:233: friend class
plugin_test_internal::TestablePluginList;
On 2010/12/15 18:03:27, jkummerow wrote:
> On 2010/12/14 20:18:34, Bernhard Bauer wrote:
> > Couldn't you make the necessary methods protected?
> 
> Done.
> I have to say, though, that I find it ugly to split the non-public
> methods/members into separate protected/private sections based solely on the
> needs of the test class. I'm happy to revert this if you agree that having the
> test class as a friend is cleaner.

I agree that it's not as nice.  We shouldn't break encapsulation just for tests,
since those newly exposed members can now in the future be misused in non-test
code.  Having the test class be a friend ensures that the members aren't touched
by non-test code.

[Jakob Kummerow, 2010-12-16 13:00:58]

FYI, I have moved the bug fixes into a separate CL (5918003) in order to be able
to land them in a timely manner. 
I'll resume working on the PluginList unit tests once the implementation details
of CL 5699005 are reasonably stable.

[Bernhard Bauer, 2010-12-17 18:54:17]

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
File webkit/glue/plugins/plugin_list_unittest.cc (right):

http://codereview.chromium.org/5783005/diff/10001/webkit/glue/plugins/plugin_...
webkit/glue/plugins/plugin_list_unittest.cc:14: class TestablePluginList :
public NPAPI::PluginList {
On 2010/12/15 10:42:12, danno wrote:
> On 2010/12/14 20:18:34, Bernhard Bauer wrote:
> > Nit: MockPluginList or something.
> It's a bit more than a mock. The current name is a bit curious, however, since
I
> really hope that the PluginList is testable in the first place :-). How about
> PluginListWithoutFileIO?

My suggestions for the color of the bikeshed: {Dummy,Fake}PluginList, and blue.

[Jakob Kummerow, 2011-01-25 08:21:25]

The remaining part of this CL, that is the unit tests for PluginList, have now
been (adapted and) landed as part of CL 5699005 by pastarmovj. I'm therefore
closing this issue.