Separate plugin_metadata from plugin_installer, thread-safe plugin_finder

Separate plugin_metadata from plugin_installer, make plugin_finder thread-safe and expose its sync interface


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

[ibraaaa, 2012-09-11 09:43:34]

I have one more suggestion that we could augment in here. If you looked at
PluginInstaller and its usages after we separated the metadata in a separate
class, it seems like it doesn't need to aggregate PluginMetadata. Actually, I
guess that a dependency relation is sufficient. In other words, only a couple of
methods of PluginInstaller needs plugin_url and url_for_display which can be
supplied as input parameters to these methods.

That would eliminate all usages of PluginInstaller in PluginFinder (it will only
manipulate PluginMetadata). Also, PluginInstaller instances will be created and
released upon demand(I mean in case we need to install a plugin).

We can do this in a separate CL at the end, what do you think?

http://codereview.chromium.org/10910168/diff/2001/chrome/browser/plugin_finder.h
File chrome/browser/plugin_finder.h (right):

http://codereview.chromium.org/10910168/diff/2001/chrome/browser/plugin_finde...
chrome/browser/plugin_finder.h:50: PluginInstaller*
FindPluginWithIdentifier(const std::string& identifier);
This is only used in plugin_observer.cc because it needs a PluginInstaller
instance. This will be removed in case we decided to follow my earlier
suggestion.

http://codereview.chromium.org/10910168/diff/2001/chrome/browser/plugin_insta...
File chrome/browser/plugin_installer.h (right):

http://codereview.chromium.org/10910168/diff/2001/chrome/browser/plugin_insta...
chrome/browser/plugin_installer.h:50: const std::string& identifier() const {
return plugin_->identifier(); }
I implemented these getters so that the CL doesn't get very large, will remove
them in an upcoming CL.

[ibraaaa, 2012-09-11 12:44:10]

http://codereview.chromium.org/10910168/diff/2001/chrome/browser/plugin_insta...
File chrome/browser/plugin_installer.h (right):

http://codereview.chromium.org/10910168/diff/2001/chrome/browser/plugin_insta...
chrome/browser/plugin_installer.h:50: const std::string& identifier() const {
return plugin_->identifier(); }
Or shall we keep them, what do you think? If we are going to implement the
suggestion I mentioned earlier then they will be deleted anyway.

On 2012/09/11 09:43:34, ibraaaa wrote:
> I implemented these getters so that the CL doesn't get very large, will remove
> them in an upcoming CL.

[Bernhard Bauer, 2012-09-17 15:34:34]

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_finde...
File chrome/browser/plugin_finder.cc (right):

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_finde...
chrome/browser/plugin_finder.cc:80: void PluginFinder::Init() {
Why don't we do this in the constructor?

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_finde...
chrome/browser/plugin_finder.cc:183: PluginMetadata*
PluginFinder::FindPluginMetadataWithIdentifier(
Nit: I would prefer "FindPlugin..." here.

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_insta...
File chrome/browser/plugin_installer.h (right):

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_insta...
chrome/browser/plugin_installer.h:83: // It can't be NULL. It is owned by
PluginFinder.
"It" seems more confusing here than simply leaving it out (or explicitly
mentioning |plugin_|).

[ibraaaa, 2012-09-17 16:12:56]

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_finde...
File chrome/browser/plugin_finder.cc (right):

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_finde...
chrome/browser/plugin_finder.cc:80: void PluginFinder::Init() {
well, I remember that the style guide does not recommend doing work in Ctors,
here is a link:
http://www.corp.google.com/eng/doc/cppguide.xml#Doing_Work_in_Constructors

I personally like doing work in the Ctor :), do I move the logic there?

On 2012/09/17 15:34:34, Bernhard Bauer wrote:
> Why don't we do this in the constructor?

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_finde...
chrome/browser/plugin_finder.cc:183: PluginMetadata*
PluginFinder::FindPluginMetadataWithIdentifier(
Well, there is already "FindPluginWithIdentifier" above. Probably, it is better
to name this one FindPluginWithIdentifier and the one above
FindPluginInstallerWithIdentifier?

However, this naming switch will make this CL much bigger. I suggest we make
this switch in a separate CL?

On 2012/09/17 15:34:34, Bernhard Bauer wrote:
> Nit: I would prefer "FindPlugin..." here.

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_insta...
File chrome/browser/plugin_installer.h (right):

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_insta...
chrome/browser/plugin_installer.h:83: // It can't be NULL. It is owned by
PluginFinder.
On 2012/09/17 15:34:34, Bernhard Bauer wrote:
> "It" seems more confusing here than simply leaving it out (or explicitly
> mentioning |plugin_|).

Done.

[ibraaaa, 2012-09-20 16:55:14]

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

[Bernhard Bauer, 2012-09-20 17:05:10]

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_finde...
File chrome/browser/plugin_finder.cc (right):

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_finde...
chrome/browser/plugin_finder.cc:183: PluginMetadata*
PluginFinder::FindPluginMetadataWithIdentifier(
On 2012/09/17 16:12:56, ibraaaa wrote:
> Well, there is already "FindPluginWithIdentifier" above. Probably, it is
better
> to name this one FindPluginWithIdentifier and the one above
> FindPluginInstallerWithIdentifier?
> 
> However, this naming switch will make this CL much bigger. I suggest we make
> this switch in a separate CL?

Sure.

> 
> On 2012/09/17 15:34:34, Bernhard Bauer wrote:
> > Nit: I would prefer "FindPlugin..." here.
>

[Bernhard Bauer, 2012-09-20 17:13:02]

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_finde...
File chrome/browser/plugin_finder.cc (right):

http://codereview.chromium.org/10910168/diff/5002/chrome/browser/plugin_finde...
chrome/browser/plugin_finder.cc:80: void PluginFinder::Init() {
On 2012/09/17 16:12:56, ibraaaa wrote:
> well, I remember that the style guide does not recommend doing work in Ctors,
> here is a link:
> http://www.corp.google.com/eng/doc/cppguide.xml#Doing_Work_in_Constructors
> 
> I personally like doing work in the Ctor :), do I move the logic there?

I think doing work in the constructor is a lesser evil than having to initialize
a singleton in a completely different place. If we want to avoid both (at the
expense of more code), we could add special SingletonTraits that call the Init()
method. 

> On 2012/09/17 15:34:34, Bernhard Bauer wrote:
> > Why don't we do this in the constructor?
>

[ibraaaa, 2012-09-21 14:11:46]

https://chromiumcodereview.appspot.com/10910168/diff/5002/chrome/browser/plug...
File chrome/browser/plugin_finder.cc (right):

https://chromiumcodereview.appspot.com/10910168/diff/5002/chrome/browser/plug...
chrome/browser/plugin_finder.cc:80: void PluginFinder::Init() {
On 2012/09/20 17:13:02, Bernhard Bauer wrote:
> On 2012/09/17 16:12:56, ibraaaa wrote:
> > well, I remember that the style guide does not recommend doing work in
Ctors,
> > here is a link:
> > http://www.corp.google.com/eng/doc/cppguide.xml#Doing_Work_in_Constructors
> > 
> > I personally like doing work in the Ctor :), do I move the logic there?
> 
> I think doing work in the constructor is a lesser evil than having to
initialize
> a singleton in a completely different place. If we want to avoid both (at the
> expense of more code), we could add special SingletonTraits that call the
Init()
> method. 
> 
> > On 2012/09/17 15:34:34, Bernhard Bauer wrote:
> > > Why don't we do this in the constructor?
> > 
> 

Done.

[Bernhard Bauer, 2012-09-24 09:14:25]

LGTM w/ nits:

https://chromiumcodereview.appspot.com/10910168/diff/17002/chrome/browser/bro...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/10910168/diff/17002/chrome/browser/bro...
chrome/browser/browser_process_impl.cc:796: // Triggers initialization of the
singleton instance on UI thread.
Oh, I forgot that we need to make sure PluginFinder is initialized on the right
thread. In that case we don't really gain anything from using custom Singleton
traits, do we? Also, at some point we're probably going to need to initialize
PluginFinder with Local State anyway. I'll leave it to you if want to go back to
explicit initialization or keep the custom Singleton Traits.

https://chromiumcodereview.appspot.com/10910168/diff/17002/chrome/browser/plu...
File chrome/browser/plugins/plugin_finder.h (right):

https://chromiumcodereview.appspot.com/10910168/diff/17002/chrome/browser/plu...
chrome/browser/plugins/plugin_finder.h:31: // |GetInstance()| and |Init()| on
the UI thread.
This comment isn't correct anymore.

https://chromiumcodereview.appspot.com/10910168/diff/17002/chrome/renderer/pl...
File chrome/renderer/plugins/plugin_placeholder.cc (left):

https://chromiumcodereview.appspot.com/10910168/diff/17002/chrome/renderer/pl...
chrome/renderer/plugins/plugin_placeholder.cc:441: 
Nit: This change is unnecessary now.

[ibraaaa, 2012-09-24 11:01:07]

Bernhard, PTAL. I removed callers of FindPluginMetadataWithIdentifier. I am
going to update it in this CL: http://codereview.chromium.org/10917189/ to be
FindPluginNameWithIdentifier.

http://codereview.chromium.org/10910168/diff/17002/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/10910168/diff/17002/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:796: // Triggers initialization of the
singleton instance on UI thread.
Now I understood your point in using custom traits here :)

On 2012/09/24 09:14:25, Bernhard Bauer wrote:
> Oh, I forgot that we need to make sure PluginFinder is initialized on the
right
> thread. In that case we don't really gain anything from using custom Singleton
> traits, do we? Also, at some point we're probably going to need to initialize
> PluginFinder with Local State anyway. I'll leave it to you if want to go back
to
> explicit initialization or keep the custom Singleton Traits.

http://codereview.chromium.org/10910168/diff/17002/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_finder.h (right):

http://codereview.chromium.org/10910168/diff/17002/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_finder.h:31: // |GetInstance()| and |Init()| on
the UI thread.
reverted the custom traits.
On 2012/09/24 09:14:25, Bernhard Bauer wrote:
> This comment isn't correct anymore.

http://codereview.chromium.org/10910168/diff/17002/chrome/renderer/plugins/pl...
File chrome/renderer/plugins/plugin_placeholder.cc (left):

http://codereview.chromium.org/10910168/diff/17002/chrome/renderer/plugins/pl...
chrome/renderer/plugins/plugin_placeholder.cc:441: 
On 2012/09/24 09:14:25, Bernhard Bauer wrote:
> Nit: This change is unnecessary now.

Done.

[Bernhard Bauer, 2012-09-24 11:10:59]

still lgtm

[ibraaaa, 2012-09-24 11:16:48]

Hi John,

Can I have an OWNERS review for this CL.

Everything except chrome/browser/plugins/

Thanks!

[jam, 2012-09-24 15:29:45]

lgtm

[commit-bot: I haz the power, 2012-09-24 16:03:52]

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

[commit-bot: I haz the power, 2012-09-24 16:41:28]

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

[commit-bot: I haz the power, 2012-09-24 16:56:35]

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

[ibraaaa, 2012-09-24 17:57:09]

Bernhard, all browser policy tests for plugins were failing because our update
to PluginPrefs#EnablePlugin no more posts a task via MessageLoop if there is a
policy on the plugin (simply because we no more call PluginFinder::Get).
Accordingly the method SetPluginEnabled in browser_policytests.cc will fail
because in the above case we will be quittin a message loop without running it.

Please take a look at policy_browsertests.cc, Thanks!

[Bernhard Bauer, 2012-09-24 18:06:04]

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/policy/poli...
File chrome/browser/policy/policy_browsertest.cc (left):

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:285:
plugin_prefs->EnablePlugin(enabled, plugin->path,
Wait, I don't understand. We call the callback synchronously (instead of posting
it), and that's why the tests were failing? Couldn't we just remove the
RunMessageLoop() call in that case? To depend on whether we really end up
posting a task to the message loop or not seems kinda clunky.

[ibraaaa, 2012-09-24 18:18:36]

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/policy/poli...
File chrome/browser/policy/policy_browsertest.cc (left):

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:285:
plugin_prefs->EnablePlugin(enabled, plugin->path,
Yes, that is the reason we are failing.

Do you mean updating SetPluginEnabled to run the message loop only when
PluginPrefs#EnablePlugin posts a task?

If yes, then we can but I didn't do it because we will need to update the test
code below to pass in the expected value to SetPluginEnabled + create a special
CopyValueAndQuit to be used for plugin policy tests only that quits the message
loop based on the result, is this ok for you?

On 2012/09/24 18:06:04, Bernhard Bauer wrote:
> Wait, I don't understand. We call the callback synchronously (instead of
posting
> it), and that's why the tests were failing? Couldn't we just remove the
> RunMessageLoop() call in that case? To depend on whether we really end up
> posting a task to the message loop or not seems kinda clunky.

[Bernhard Bauer, 2012-09-24 18:27:31]

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/policy/poli...
File chrome/browser/policy/policy_browsertest.cc (left):

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:285:
plugin_prefs->EnablePlugin(enabled, plugin->path,
On 2012/09/24 18:18:36, ibraaaa wrote:
> Yes, that is the reason we are failing.
> 
> Do you mean updating SetPluginEnabled to run the message loop only when
> PluginPrefs#EnablePlugin posts a task?
> 
> If yes, then we can but I didn't do it because we will need to update the test
> code below to pass in the expected value to SetPluginEnabled + create a
special
> CopyValueAndQuit to be used for plugin policy tests only that quits the
message
> loop based on the result, is this ok for you?

Hm, what I meant was that whether PluginPrefs::EnablePlugin() posts a task or
not is an implementation detail, and the test should not depend on it.

OTOH, maybe we should simplify the behavior of EnablePlugin() and just make sure
that we always post the callback, even if we could do it synchronously. WDYT?

> On 2012/09/24 18:06:04, Bernhard Bauer wrote:
> > Wait, I don't understand. We call the callback synchronously (instead of
> posting
> > it), and that's why the tests were failing? Couldn't we just remove the
> > RunMessageLoop() call in that case? To depend on whether we really end up
> > posting a task to the message loop or not seems kinda clunky.
>

[ibraaaa, 2012-09-24 18:31:26]

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/policy/poli...
File chrome/browser/policy/policy_browsertest.cc (left):

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:285:
plugin_prefs->EnablePlugin(enabled, plugin->path,
Well, I am fine but why do we add a needless callback to the browser code if we
can adapt the tests:), WDYT?

On 2012/09/24 18:27:31, Bernhard Bauer wrote:
> On 2012/09/24 18:18:36, ibraaaa wrote:
> > Yes, that is the reason we are failing.
> > 
> > Do you mean updating SetPluginEnabled to run the message loop only when
> > PluginPrefs#EnablePlugin posts a task?
> > 
> > If yes, then we can but I didn't do it because we will need to update the
test
> > code below to pass in the expected value to SetPluginEnabled + create a
> special
> > CopyValueAndQuit to be used for plugin policy tests only that quits the
> message
> > loop based on the result, is this ok for you?
> 
> Hm, what I meant was that whether PluginPrefs::EnablePlugin() posts a task or
> not is an implementation detail, and the test should not depend on it.
> 
> OTOH, maybe we should simplify the behavior of EnablePlugin() and just make
sure
> that we always post the callback, even if we could do it synchronously. WDYT?
> 
> > On 2012/09/24 18:06:04, Bernhard Bauer wrote:
> > > Wait, I don't understand. We call the callback synchronously (instead of
> > posting
> > > it), and that's why the tests were failing? Couldn't we just remove the
> > > RunMessageLoop() call in that case? To depend on whether we really end up
> > > posting a task to the message loop or not seems kinda clunky.
> > 
>

[ibraaaa, 2012-09-24 19:10:21]

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_prefs.cc (right):

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_prefs.cc:164: callback.Run(false);
Do you mean calling this callback by MessageLoop::Current()->PostTask instead of
calling it synchronously?

[Bernhard Bauer, 2012-09-24 19:34:28]

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_prefs.cc (right):

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_prefs.cc:164: callback.Run(false);
On 2012/09/24 19:10:21, ibraaaa wrote:
> Do you mean calling this callback by MessageLoop::Current()->PostTask instead
of
> calling it synchronously?

Yes, exactly. 

(Sorry, forgot to submit my comments) The advantage is that this way the
contract is simpler: We always return asynchronously, regardless of success or
failure (which a non-unit-test caller would not even know in advance).

[ibraaaa, 2012-09-24 20:07:13]

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/plugins/plu...
File chrome/browser/plugins/plugin_prefs.cc (right):

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/plugins/plu...
chrome/browser/plugins/plugin_prefs.cc:164: callback.Run(false);
On 2012/09/24 19:34:28, Bernhard Bauer wrote:
> On 2012/09/24 19:10:21, ibraaaa wrote:
> > Do you mean calling this callback by MessageLoop::Current()->PostTask
instead
> of
> > calling it synchronously?
> 
> Yes, exactly. 
> 
> (Sorry, forgot to submit my comments) The advantage is that this way the
> contract is simpler: We always return asynchronously, regardless of success or
> failure (which a non-unit-test caller would not even know in advance). 

Done.

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/policy/poli...
File chrome/browser/policy/policy_browsertest.cc (left):

http://codereview.chromium.org/10910168/diff/30012/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:285:
plugin_prefs->EnablePlugin(enabled, plugin->path,
On 2012/09/24 18:27:31, Bernhard Bauer wrote:
> On 2012/09/24 18:18:36, ibraaaa wrote:
> > Yes, that is the reason we are failing.
> > 
> > Do you mean updating SetPluginEnabled to run the message loop only when
> > PluginPrefs#EnablePlugin posts a task?
> > 
> > If yes, then we can but I didn't do it because we will need to update the
test
> > code below to pass in the expected value to SetPluginEnabled + create a
> special
> > CopyValueAndQuit to be used for plugin policy tests only that quits the
> message
> > loop based on the result, is this ok for you?
> 
> Hm, what I meant was that whether PluginPrefs::EnablePlugin() posts a task or
> not is an implementation detail, and the test should not depend on it.
> 
> OTOH, maybe we should simplify the behavior of EnablePlugin() and just make
sure
> that we always post the callback, even if we could do it synchronously. WDYT?
> 
> > On 2012/09/24 18:06:04, Bernhard Bauer wrote:
> > > Wait, I don't understand. We call the callback synchronously (instead of
> > posting
> > > it), and that's why the tests were failing? Couldn't we just remove the
> > > RunMessageLoop() call in that case? To depend on whether we really end up
> > > posting a task to the message loop or not seems kinda clunky.
> > 
> 

Done.

[Bernhard Bauer, 2012-09-24 20:09:32]

LGTM

[commit-bot: I haz the power, 2012-09-24 20:12:29]

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

[commit-bot: I haz the power, 2012-09-24 20:12:38]

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

[commit-bot: I haz the power, 2012-09-24 20:32:19]

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

[commit-bot: I haz the power, 2012-09-24 20:38:09]

Retried try job too often for step(s) compile

[commit-bot: I haz the power, 2012-09-24 20:46:30]

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

[commit-bot: I haz the power, 2012-09-24 20:54:17]

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

[commit-bot: I haz the power, 2012-09-24 22:07:47]

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

[commit-bot: I haz the power, 2012-09-24 22:59:54]

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

[commit-bot: I haz the power, 2012-09-25 17:29:04]

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

[commit-bot: I haz the power, 2012-09-25 19:56:03]

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

[commit-bot: I haz the power, 2012-09-25 20:03:44]

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

[commit-bot: I haz the power, 2012-09-25 21:24:00]

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

[commit-bot: I haz the power, 2012-09-25 23:06:04]

Change committed as 158679