Clean up PluginDataRemover.

Clean up PluginDataRemover.

This CL fixes some threading issues in PluginDataRemover. It also adds a static method to find out if removing plugin data is supported by an installed plugin, and a method to set the completion task while it is in the process of removing data.

BUG=58235
TEST=none

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

[Bernhard Bauer, 2010-12-09 13:48:18]

Please review. These are some additions to PluginDataRemover necessary to make
it actually work ;-)

Danno: FYI.

[jam, 2010-12-13 22:41:26]

I'm curious, why do you have to introduce an internal class?  Why not just make
PluginDataRemover be refcounted?  This pattern is useful when we don't have
control of our object's lifetime, say because it inherits from an interface
that's not refcounted.  but here we have control of the caller's code

http://codereview.chromium.org/5715001/diff/5001/chrome/browser/plugin_data_r...
File chrome/browser/plugin_data_remover.cc (right):

http://codereview.chromium.org/5715001/diff/5001/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.cc:27: const int64 timeout_ms = 10000;
can you prefix globals with "g_"

[Bernhard Bauer, 2010-12-13 22:53:02]

On Mon, Dec 13, 2010 at 23:41,  <jam@chromium.org> wrote:
> I'm curious, why do you have to introduce an internal class?  Why not just
> make
> PluginDataRemover be refcounted?  This pattern is useful when we don't have
> control of our object's lifetime, say because it inherits from an interface
> that's not refcounted.  but here we have control of the caller's code

Right, that could work. I'm going to try it out.

>
http://codereview.chromium.org/5715001/diff/5001/chrome/browser/plugin_data_r...
> File chrome/browser/plugin_data_remover.cc (right):
>
>
http://codereview.chromium.org/5715001/diff/5001/chrome/browser/plugin_data_r...
> chrome/browser/plugin_data_remover.cc:27: const int64 timeout_ms =
> 10000;
> can you prefix globals with "g_"
>
> http://codereview.chromium.org/5715001/
>

[Bernhard Bauer, 2010-12-14 01:41:28]

On 2010/12/13 22:41:26, John Abd-El-Malek wrote:
> I'm curious, why do you have to introduce an internal class?  Why not just
make
> PluginDataRemover be refcounted?  This pattern is useful when we don't have
> control of our object's lifetime, say because it inherits from an interface
> that's not refcounted.  but here we have control of the caller's code

done.

>
http://codereview.chromium.org/5715001/diff/5001/chrome/browser/plugin_data_r...
> File chrome/browser/plugin_data_remover.cc (right):
> 
>
http://codereview.chromium.org/5715001/diff/5001/chrome/browser/plugin_data_r...
> chrome/browser/plugin_data_remover.cc:27: const int64 timeout_ms = 10000;
> can you prefix globals with "g_"

done.

[jam, 2010-12-14 17:34:19]

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
File chrome/browser/plugin_data_remover.cc (right):

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.cc:13: #include "ipc/ipc_channel.h"
nit: i dont think this include is needed

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.cc:49: AddRef();
I don't understand why you keep a self reference between this function and
OnChannelOpened?  i.e. why not keep it until the process is done?  and also, if
there's a timeout, Release() is never called (i.e. what if the process never
launched or connected).

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.cc:165: && min_version->CompareTo(*version)
== -1;
nit: style is to have the && at the end of hte previous line

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
File chrome/browser/plugin_data_remover.h (right):

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.h:13: #include "ipc/ipc_message.h"
nit: don't need this include

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.h:26: ~PluginDataRemover();
nit: ref counted objects should have a private constructor, to avoid someone
deleting the object accidentally.

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.h:37: bool is_removing() { return
is_removing_; }
nit: const

[Bernhard Bauer, 2010-12-14 20:38:37]

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
File chrome/browser/plugin_data_remover.cc (right):

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.cc:13: #include "ipc/ipc_channel.h"
On 2010/12/14 17:34:19, John Abd-El-Malek wrote:
> nit: i dont think this include is needed

Done.

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.cc:49: AddRef();
On 2010/12/14 17:34:19, John Abd-El-Malek wrote:
> I don't understand why you keep a self reference between this function and
> OnChannelOpened?  i.e. why not keep it until the process is done?  and also,
if
> there's a timeout, Release() is never called (i.e. what if the process never
> launched or connected).

Even if the process never launches, eventually OnError is called (when the
PluginProcessHost is deleted). For that reason, I need to make sure the client
is not deleted until either OnChannelOpened or OnError is called, and calling
Release() in each of these methods is the easiest way to make sure that happens.

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.cc:165: && min_version->CompareTo(*version)
== -1;
On 2010/12/14 17:34:19, John Abd-El-Malek wrote:
> nit: style is to have the && at the end of hte previous line

Done.

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
File chrome/browser/plugin_data_remover.h (right):

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.h:13: #include "ipc/ipc_message.h"
On 2010/12/14 17:34:19, John Abd-El-Malek wrote:
> nit: don't need this include

Done (included ipc_channel instead of ipc_channel_proxy as well).

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.h:26: ~PluginDataRemover();
On 2010/12/14 17:34:19, John Abd-El-Malek wrote:
> nit: ref counted objects should have a private constructor, to avoid someone
> deleting the object accidentally.

Right, done.

http://codereview.chromium.org/5715001/diff/3003/chrome/browser/plugin_data_r...
chrome/browser/plugin_data_remover.h:37: bool is_removing() { return
is_removing_; }
On 2010/12/14 17:34:19, John Abd-El-Malek wrote:
> nit: const

Done.

[jam, 2010-12-14 21:41:33]

lgtm