Add PluginInstaller to encapsulate information about a missing plug-in.

Add PluginInstaller to encapsulate information about a missing plug-in, and add a separate help URL for each plug-in.


TBR=arv@chromium.org
BUG=62079, 102987
TEST=none


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

[Bernhard Bauer, 2011-11-29 14:33:36]

Please review.

[battre, 2011-11-30 14:10:06]

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_finder.cc
File chrome/browser/plugin_finder.cc (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_finder...
chrome/browser/plugin_finder.cc:103: installers_[identifier] = installer;
do you need to free the installers_ in the destructor or are singletons never
properly destroyed?

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_finder...
chrome/browser/plugin_finder.cc:107: base::Bind(found_callback, installer));
once you free the installers_ int he destructor you might have a dangling
pointer here.

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_instal...
File chrome/browser/plugin_installer.h (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_instal...
chrome/browser/plugin_installer.h:23: std::string identifier() { return
identifier_; }
const std::string& identifier() const { return identifier_; }?

at least the function should be const

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_instal...
chrome/browser/plugin_installer.h:26: string16 name() { return name_; }
same here

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_instal...
chrome/browser/plugin_installer.h:32: bool url_for_display() { return
url_for_display_; }
const, also below

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_observ...
File chrome/browser/plugin_observer.cc (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_observ...
chrome/browser/plugin_observer.cc:343: weak_ptr_factory_.GetWeakPtr(),
installer)));
life-time issue

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_observ...
File chrome/browser/plugin_observer.h (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_observ...
chrome/browser/plugin_observer.h:31: PluginInstaller* installer);
const PluginInstaller*?

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_observ...
chrome/browser/plugin_observer.h:34: void InstallMissingPlugin(PluginInstaller*
installer);
same here?

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/resources/plu...
File chrome/browser/resources/plugins_mac.json (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/resources/plu...
chrome/browser/resources/plugins_mac.json:178: "displayurl": false
any reason why the "displayurl" element is not listed in plugins_win.json?

[Bernhard Bauer, 2011-11-30 14:29:23]

Thanks for the review! I'll address the other comments in the next patch set.

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_finder.cc
File chrome/browser/plugin_finder.cc (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_finder...
chrome/browser/plugin_finder.cc:103: installers_[identifier] = installer;
On 2011/11/30 14:10:06, battre wrote:
> do you need to free the installers_ in the destructor or are singletons never
> properly destroyed?

True, there might be circumstances where we destroy singletons in unit tests.

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_finder...
chrome/browser/plugin_finder.cc:107: base::Bind(found_callback, installer));
On 2011/11/30 14:10:06, battre wrote:
> once you free the installers_ int he destructor you might have a dangling
> pointer here.

Hm, during normal browser runs I think it's reasonable to assume the
PluginFinder will continue to be around. Would you be okay with me just adding a
comment to the header that clients need to make sure their callbacks are
appropriately scoped (using WeakPtr or so) so that they won't outlive the
PluginFinder?

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_observ...
File chrome/browser/plugin_observer.cc (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_observ...
chrome/browser/plugin_observer.cc:343: weak_ptr_factory_.GetWeakPtr(),
installer)));
On 2011/11/30 14:10:06, battre wrote:
> life-time issue

The PluginObserver is bound to the lifetime of the TabContents, so it should not
outlive the PluginInstaller.

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_observ...
File chrome/browser/plugin_observer.h (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_observ...
chrome/browser/plugin_observer.h:34: void InstallMissingPlugin(PluginInstaller*
installer);
On 2011/11/30 14:10:06, battre wrote:
> same here?

Hm, I'm not sure if that's worth it, because once we have state in the
PluginInstaller, installing a plug-in will change it, so I'd need to make it
non-const again.

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/resources/plu...
File chrome/browser/resources/plugins_mac.json (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/resources/plu...
chrome/browser/resources/plugins_mac.json:178: "displayurl": false
On 2011/11/30 14:10:06, battre wrote:
> any reason why the "displayurl" element is not listed in plugins_win.json?

Oops, this line shouldn't be in here. For now, all the Mac plugins use
displayurl, until the plugin installation functionality is ported (and we have
direct download URLs). For Windows, the URLs are for direct download, so we
don't use displayurl there.

[Bernhard Bauer, 2011-12-01 09:39:25]

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_finder.cc
File chrome/browser/plugin_finder.cc (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_finder...
chrome/browser/plugin_finder.cc:103: installers_[identifier] = installer;
On 2011/11/30 14:10:06, battre wrote:
> do you need to free the installers_ in the destructor or are singletons never
> properly destroyed?

Done.

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_instal...
File chrome/browser/plugin_installer.h (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/plugin_instal...
chrome/browser/plugin_installer.h:23: std::string identifier() { return
identifier_; }
On 2011/11/30 14:10:06, battre wrote:
> const std::string& identifier() const { return identifier_; }?
> 
> at least the function should be const

done (also in other places in this file).

[Bernhard Bauer, 2011-12-01 10:20:56]

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/resources/plu...
File chrome/browser/resources/plugins_mac.json (right):

http://codereview.chromium.org/8664027/diff/3010/chrome/browser/resources/plu...
chrome/browser/resources/plugins_mac.json:178: "displayurl": false
On 2011/11/30 14:29:23, Bernhard Bauer wrote:
> On 2011/11/30 14:10:06, battre wrote:
> > any reason why the "displayurl" element is not listed in plugins_win.json?
> 
> Oops, this line shouldn't be in here. For now, all the Mac plugins use
> displayurl, until the plugin installation functionality is ported (and we have
> direct download URLs). For Windows, the URLs are for direct download, so we
> don't use displayurl there.

Done.

[battre, 2011-12-01 10:23:56]

lgtm

[commit-bot: I haz the power, 2011-12-01 17:30:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/8664027/14001

[commit-bot: I haz the power, 2011-12-01 20:04:20]

Try job failure for 8664027-14001 (retry) on win_rel for step "ui_tests"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-01 22:56:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/8664027/14001

[commit-bot: I haz the power, 2011-12-02 02:28:14]

Change committed as 112610