If a user chooses to open a PDF with Reader, ask them if they want to do so a...

chrome/browser/pdf_unsupported_feature.cc

Index: chrome/browser/pdf_unsupported_feature.cc
===================================================================
--- chrome/browser/pdf_unsupported_feature.cc	(revision 72573)
+++ chrome/browser/pdf_unsupported_feature.cc	(working copy)
@@ -7,6 +7,7 @@
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "base/version.h"
+#include "chrome/browser/metrics/user_metrics.h"
 #include "chrome/browser/plugin_service.h"
 #include "chrome/browser/renderer_host/render_process_host.h"
 #include "chrome/browser/renderer_host/render_view_host.h"
@@ -14,6 +15,7 @@
 #include "chrome/browser/tab_contents/interstitial_page.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/common/jstemplate_builder.h"
+#include "chrome/common/pepper_plugin_registry.h"
 #include "grit/browser_resources.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -31,6 +33,70 @@
 
 namespace {
 
+// The info bar delegate used to ask the user if they want to use Adobe Reader
+// by default.
+class PDFEnableAdobeReaderConfirmInfoBarDelegate
+    : public ConfirmInfoBarDelegate {
+ public:
+  PDFEnableAdobeReaderConfirmInfoBarDelegate(
+      TabContents* tab_contents)
+      : ConfirmInfoBarDelegate(tab_contents) {
+    UserMetrics::RecordAction(
+        UserMetricsAction("PDF_EnableReaderInfoBarShown"));
+  }
+
+  // ConfirmInfoBarDelegate
+  virtual void InfoBarClosed() {

[Chris Evans, 2011/01/27 00:03:27]

I still think this is a different user action (user presses X on right) than
Cancel() (user presses one of the buttons) -- so maybe record a UMA here? It's
how Bernhard does it in the out-of-date-blocked-plugin infobar...

[jam, 2011/01/27 00:31:02]

ah, I thought Cancel() would get closed in that case.  turns out it doesn't.

I override InfoBarDismissed() now InfoBarClosed would get called in other
scenarios, i.e. clicking on Yes.

+    delete this;
+  }
+
+  virtual Type GetInfoBarType() const {
+    return PAGE_ACTION_TYPE;
+  }
+
+  virtual bool Accept() {
+    UserMetrics::RecordAction(
+        UserMetricsAction("PDF_EnableReaderInfoBarOK"));

[Chris Evans, 2011/01/27 00:03:27]

I'm not sure which way around the buttons are. Can you either make a change, or
confirm that the leftmost button is the one that leaves the current built-in PDF
preference intact?

[jam, 2011/01/27 00:31:02]

Yes No X
This is done per what was agreed on in the threads and spec describing this.  If
you want it changed, the best place is to reply to that thread.

+    webkit::npapi::PluginList::Singleton()->EnableGroup(
+        false, ASCIIToUTF16(PepperPluginRegistry::kPDFPluginName));
+    webkit::npapi::PluginList::Singleton()->EnableGroup(
+        true, ASCIIToUTF16(webkit::npapi::PluginGroup::kAdobeReaderGroupName));
+    return true;
+  }
+
+  virtual bool Cancel() {
+    UserMetrics::RecordAction(
+        UserMetricsAction("PDF_EnableReaderInfoBarCancel"));
+    return true;
+  }
+
+  virtual int GetButtons() const {
+    return BUTTON_OK | BUTTON_CANCEL;
+  }
+
+  virtual string16 GetButtonLabel(InfoBarButton button) const {
+    switch (button) {
+      case BUTTON_OK:
+        return l10n_util::GetStringUTF16(
+            IDS_CONFIRM_MESSAGEBOX_YES_BUTTON_LABEL);
+      case BUTTON_CANCEL:
+        return l10n_util::GetStringUTF16(
+            IDS_CONFIRM_MESSAGEBOX_NO_BUTTON_LABEL);
+      default:
+        // All buttons are labeled above.
+        NOTREACHED() << "Bad button id " << button;
+        return string16();
+    }
+  }

[Chris Evans, 2011/01/27 00:03:27]

Newline

[jam, 2011/01/27 00:31:02]

Done.

+  virtual string16 GetMessageText() const {
+    return l10n_util::GetStringUTF16(
+        IDS_PDF_INFOBAR_QUESTION_ALWAYS_USE_READER);

[Chris Evans, 2011/01/27 00:03:27]

"Always use Adobe Reader to open PDF files?" -- as per above, the default must
be no, please. Also, can we at least think about indicating:
1) How the user would undo this (about:plugins)
2) Gently recommend against it on account of either the speed or security angle?

[jam, 2011/01/27 00:31:02]

Sorry I don't think this is the place to change the UI.  You should have been on
the original thread, so best to discuss changes there.

+  }
+
+ private:
+  DISALLOW_IMPLICIT_CONSTRUCTORS(PDFEnableAdobeReaderConfirmInfoBarDelegate);
+};
+
 // Launch the url to get the latest Adbobe Reader installer.
 void OpenReaderUpdateURL(TabContents* tab) {
   tab->OpenURL(GURL(PluginGroup::kAdobeReaderUpdateURL), GURL(), CURRENT_TAB,
@@ -38,15 +104,30 @@
 }
 
 // Opens the PDF using Adobe Reader.
-void OpenUsingReader(TabContents* tab, const WebPluginInfo& reader_plugin) {
+void OpenUsingReader(TabContents* tab,
+                     const WebPluginInfo& reader_plugin,
+                     InfoBarDelegate* old_delegate) {
   PluginService::OverriddenPlugin plugin;
   plugin.render_process_id = tab->GetRenderProcessHost()->id();
   plugin.render_view_id = tab->render_view_host()->routing_id();
   plugin.url = tab->GetURL();
   plugin.plugin = reader_plugin;
+  // The plugin is disabled, so enable it to get around the renderer check.
+  // Also give it a new version so that the renderer doesn't show the blocked
+  // plugin UI if it's vulnerable, since we already went through the
+  // interstitial.
+  plugin.plugin.enabled = WebPluginInfo::USER_ENABLED;
+  plugin.plugin.version = ASCIIToUTF16("11.0.0.0");

[Chris Evans, 2011/01/27 00:03:27]

One day, that will be a vulnerable version in the 11 series. Should we use
50.0.0.0 or similar to avoid random breakage 2 years down the line?

[jam, 2011/01/27 00:31:02]

Once that happens, we'll know pretty quickly (i.e. blocked plugin will show up
and we'll see it).

 
   PluginService::GetInstance()->OverridePluginForTab(plugin);
   tab->render_view_host()->ReloadFrame();
+
+  InfoBarDelegate* bar = new PDFEnableAdobeReaderConfirmInfoBarDelegate(tab);
+  if (old_delegate) {
+    tab->ReplaceInfoBar(old_delegate, bar);
+  } else {
+    tab->AddInfoBar(bar);
+  }
 }
 
 // An interstitial to be used when the user chooses to open a PDF using Adobe
@@ -58,6 +139,7 @@
       const WebPluginInfo& reader_webplugininfo)
       : InterstitialPage(tab, false, tab->GetURL()),
         reader_webplugininfo_(reader_webplugininfo) {
+    UserMetrics::RecordAction(UserMetricsAction("PDF_ReaderInterstitialShown"));
   }
 
  protected:
@@ -92,14 +174,20 @@
 
   virtual void CommandReceived(const std::string& command) {
     if (command == "0") {
+      UserMetrics::RecordAction(
+          UserMetricsAction("PDF_ReaderInterstitialCancel"));
       DontProceed();
       return;
     }
 
     if (command == "1") {
+      UserMetrics::RecordAction(
+          UserMetricsAction("PDF_ReaderInterstitialUpdate"));
       OpenReaderUpdateURL(tab());
     } else if (command == "2") {
-      OpenUsingReader(tab(), reader_webplugininfo_);
+      UserMetrics::RecordAction(
+          UserMetricsAction("PDF_ReaderInterstitialIgnore"));
+      OpenUsingReader(tab(), reader_webplugininfo_, NULL);
     } else {
       NOTREACHED();
     }
@@ -125,6 +213,7 @@
         reader_installed_(!!reader_group),
         reader_vulnerable_(false) {
     if (reader_installed_) {
+      UserMetrics::RecordAction(UserMetricsAction("PDF_UseReaderInfoBarShown"));
       std::vector<WebPluginInfo> plugins = reader_group->web_plugin_infos();
       DCHECK_EQ(plugins.size(), 1u);
       reader_webplugininfo_ = plugins[0];
@@ -138,6 +227,9 @@
             reader_vulnerable_ = true;
         }
       }
+    } else {
+      UserMetrics::RecordAction(
+          UserMetricsAction("PDF_InstallReaderInfoBarShown"));
     }
   }
 
@@ -145,16 +237,49 @@
   virtual void InfoBarClosed() {
     delete this;
   }
-  virtual Type GetInfoBarType() {
+
+  virtual Type GetInfoBarType() const {
     return PAGE_ACTION_TYPE;
   }
+
   virtual bool Accept() {

[Chris Evans, 2011/01/27 00:03:27]

As per the other infobar, please can we make sure that the leftmost button is
effectively "no"? For the majority of PDFs where this fires, the user is likely
seeing a well-rendered PDF that gives them what they want already, with no
particular need to bounce to Reader.

[jam, 2011/01/27 00:31:02]

ditto

-    LaunchReader();
+    if (!reader_installed_) {
+      UserMetrics::RecordAction(
+          UserMetricsAction("PDF_InstallReaderInfoBarOK"));
+      OpenReaderUpdateURL(tab_contents_);
+      return true;
+    }
+
+    UserMetrics::RecordAction(
+        UserMetricsAction("PDF_UseReaderInfoBarOK"));
+
+    if (reader_vulnerable_) {
+      PDFUnsupportedFeatureInterstitial* interstitial = new
+          PDFUnsupportedFeatureInterstitial(
+              tab_contents_, reader_webplugininfo_);
+      interstitial->Show();
+      return true;
+    }
+
+    OpenUsingReader(tab_contents_, reader_webplugininfo_, this);
+    return false;
+  }
+
+  virtual bool Cancel() {
+    if (reader_installed_) {
+      UserMetrics::RecordAction(
+          UserMetricsAction("PDF_UseReaderInfoBarCancel"));
+    } else {
+      UserMetrics::RecordAction(
+          UserMetricsAction("PDF_InstallReaderInfoBarCancel"));
+    }
     return true;
   }
+
   virtual int GetButtons() const {
     return BUTTON_OK | BUTTON_CANCEL;
   }
+
   virtual string16 GetButtonLabel(InfoBarButton button) const {
     switch (button) {
       case BUTTON_OK:
@@ -169,6 +294,7 @@
         return string16();
     }
   }
+
   virtual string16 GetMessageText() const {
     return l10n_util::GetStringUTF16(reader_installed_ ?
         IDS_PDF_INFOBAR_QUESTION_READER_INSTALLED :
@@ -176,23 +302,6 @@
   }
 
  private:
-  void LaunchReader() {
-    if (!reader_installed_) {
-      OpenReaderUpdateURL(tab_contents_);
-      return;
-    }
-
-    if (reader_vulnerable_) {
-      PDFUnsupportedFeatureInterstitial* interstitial = new
-          PDFUnsupportedFeatureInterstitial(
-              tab_contents_, reader_webplugininfo_);
-      interstitial->Show();
-      return;
-    }
-
-    OpenUsingReader(tab_contents_, reader_webplugininfo_);
-  }
-
   TabContents* tab_contents_;
   bool reader_installed_;
   bool reader_vulnerable_;
@@ -209,12 +318,16 @@
   // externally since Adobe Reader doesn't work inside Chrome.
   return;
 #endif
+  string16 reader_group_name(ASCIIToUTF16(PluginGroup::kAdobeReaderGroupName));
 
+  // If the Reader plugin is disabled by policy, don't prompt them.
+  if (PluginGroup::IsPluginNameDisabledByPolicy(reader_group_name))
+    return;

[Chris Evans, 2011/01/27 00:03:27]

I guess what I was hoping for is some command line, i.e.
--no-offer-adobe-reader-for-pdf or something equivalently easy?

[jam, 2011/01/27 00:31:02]

I guess I don't see the scenario where a company will want to block people from
being offered to use Reader, but will still allow them to manually set it in
about:plugins.  If a company is that paranoid that an edge case (unsupported
feature) can allow a user to enable Reader, they'll most likely have already
disabled it via group policy.

+
   PluginGroup* reader_group = NULL;
   std::vector<PluginGroup> plugin_groups;
   PluginList::Singleton()->GetPluginGroups(
       false, &plugin_groups);
-  string16 reader_group_name(UTF8ToUTF16(PluginGroup::kAdobeReaderGroupName));
   for (size_t i = 0; i < plugin_groups.size(); ++i) {
     if (plugin_groups[i].GetGroupName() == reader_group_name) {
       reader_group = &plugin_groups[i];
@@ -222,10 +335,6 @@
     }
   }
 
-  // If the plugin is disabled by policy or by the user, don't prompt them.
-  if (reader_group && !reader_group->Enabled())
-    return;
-
   tab->AddInfoBar(new PDFUnsupportedFeatureConfirmInfoBarDelegate(
       tab, reader_group));
 }