Block NPAPI plug-ins in Metro mode instead of not loading them at all.

Block NPAPI plug-ins in Metro mode instead of not loading them at all.


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

[Bernhard Bauer, 2012-10-17 12:50:35]

Please review.

[cpu_(ooo_6.6-7.5), 2012-10-18 00:42:59]

Looks good in general.

http://codereview.chromium.org/11066043/diff/11001/chrome/common/render_messa...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/11066043/diff/11001/chrome/common/render_messa...
chrome/common/render_messages.h:62: kNpapiNotSupported,
kNPAPINotSuported

http://codereview.chromium.org/11066043/diff/11001/chrome/common/render_messa...
chrome/common/render_messages.h:490:
IPC_MESSAGE_ROUTED0(ChromeViewHostMsg_RestartInDesktopMode)
Lowly renderer telling the browser to restart? I am afraid.

http://codereview.chromium.org/11066043/diff/11001/chrome/renderer/plugins/pl...
File chrome/renderer/plugins/plugin_placeholder.cc (right):

http://codereview.chromium.org/11066043/diff/11001/chrome/renderer/plugins/pl...
chrome/renderer/plugins/plugin_placeholder.cc:564: RenderThread::Get()->Send(
we need a person from sec-team to approve this.

[cpu_(ooo_6.6-7.5), 2012-10-18 00:44:37]

we also want to delete the code I added to the missing plugin? or I guess we
could have that situation as well, but in such case we need to change the text.

[Bernhard Bauer, 2012-10-18 09:13:57]

On 2012/10/18 00:44:37, cpu wrote:
> we also want to delete the code I added to the missing plugin? or I guess we
> could have that situation as well, but in such case we need to change the
text.

Yes, I had already deleted the code that showed the Metro infobar, I'd just
forgotten to delete the infobar itself.

Now, we will still show the missing plug-in infobar and allow the user to
install the plug-in. Then, we'll show the UI to restart on the desktop once the
plug-in is installed.

http://codereview.chromium.org/11066043/diff/11001/chrome/common/render_messa...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/11066043/diff/11001/chrome/common/render_messa...
chrome/common/render_messages.h:62: kNpapiNotSupported,
On 2012/10/18 00:42:59, cpu wrote:
> kNPAPINotSuported

Done.

http://codereview.chromium.org/11066043/diff/11001/chrome/common/render_messa...
chrome/common/render_messages.h:490:
IPC_MESSAGE_ROUTED0(ChromeViewHostMsg_RestartInDesktopMode)
On 2012/10/18 00:42:59, cpu wrote:
> Lowly renderer telling the browser to restart? I am afraid.

Urgh, good point. I'm not really sure what to do here, given that this is
triggered by the user interacting with the web page.

http://codereview.chromium.org/11066043/diff/11001/chrome/renderer/plugins/pl...
File chrome/renderer/plugins/plugin_placeholder.cc (right):

http://codereview.chromium.org/11066043/diff/11001/chrome/renderer/plugins/pl...
chrome/renderer/plugins/plugin_placeholder.cc:564: RenderThread::Get()->Send(
On 2012/10/18 00:42:59, cpu wrote:
> we need a person from sec-team to approve this.

Chris, what's your opinion?

[cpu_(ooo_6.6-7.5), 2012-10-18 19:58:21]

lgtm pending security review of the IPC.

One more thing, so if a plugin is missing we could either switch to desktop and
install there but I feel that the experience would be better if we switch to
desktop first.

For example I am afraid of the installer running in desktop trying to do things
like close running browsers or maybe the installer does not trigger the switch
to desktop heuristic of windows so the installer might be running/waiting while
the user is staring at metro chrome.

What do you think?

[Bernhard Bauer, 2012-10-18 20:25:41]

On 2012/10/18 19:58:21, cpu wrote:
> lgtm pending security review of the IPC.
> 
> One more thing, so if a plugin is missing we could either switch to desktop
and
> install there but I feel that the experience would be better if we switch to
> desktop first.
> 
> For example I am afraid of the installer running in desktop trying to do
things
> like close running browsers or maybe the installer does not trigger the switch
> to desktop heuristic of windows so the installer might be running/waiting
while
> the user is staring at metro chrome.
> 
> What do you think?

Hm, given that there is only a limited number of plug-ins that we offer to
install, would you be okay with landing this and iterating on it if we actually
run into issues?

[cpu_(ooo_6.6-7.5), 2012-10-18 21:53:25]

sure, but is it not as simple as keeping my code?

[Bernhard Bauer, 2012-10-22 13:27:16]

On 2012/10/18 21:53:25, cpu wrote:
> sure, but is it not as simple as keeping my code?

OK, done.

[Bernhard Bauer, 2012-10-24 22:10:12]

After discussion per email, I updated the CL to always show an infobar. PTAL?

[Bernhard Bauer, 2012-10-25 15:36:42]

ping?

[cpu_(ooo_6.6-7.5), 2012-10-25 17:30:59]

lgtm

[Bernhard Bauer, 2012-10-25 21:10:26]

Ben, can I get an OWNERS review?

[Ben Goodger (Google), 2012-10-25 21:18:50]

lgtm

[commit-bot: I haz the power, 2012-10-26 09:39:49]

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

[commit-bot: I haz the power, 2012-10-26 09:39:56]

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

Patch:       chrome/browser/renderer_host/plugin_info_message_filter.cc
Index: chrome/browser/renderer_host/plugin_info_message_filter.cc
diff --git a/chrome/browser/renderer_host/plugin_info_message_filter.cc
b/chrome/browser/renderer_host/plugin_info_message_filter.cc
index
3054e49c8af55b7345050c55fbfc58a3a0a3a03c..7b43532f6eef4c5d4a9e34df52800fb36348990d
100644
--- a/chrome/browser/renderer_host/plugin_info_message_filter.cc
+++ b/chrome/browser/renderer_host/plugin_info_message_filter.cc
@@ -24,6 +24,10 @@
 #include "googleurl/src/gurl.h"
 #include "webkit/plugins/npapi/plugin_list.h"
 
+#if defined(OS_WIN)
+#include "base/win/metro.h"
+#endif
+
 using content::PluginService;
 using webkit::WebPluginInfo;
 
@@ -134,6 +138,14 @@ void PluginInfoMessageFilter::Context::DecidePluginStatus(
       PluginFinder::GetInstance()->GetPluginMetadata(plugin));
   *group_name = plugin_metadata->name();
   *group_identifier = plugin_metadata->identifier();
+#if defined(OS_WIN)
+  if (plugin.type == WebPluginInfo::PLUGIN_TYPE_NPAPI &&
+      base::win::IsMetroProcess()) {
+    status->value =
+        ChromeViewHostMsg_GetPluginInfo_Status::kNPAPINotSupported;
+    return;
+  }
+#endif
 
   ContentSetting plugin_setting = CONTENT_SETTING_DEFAULT;
   bool uses_default_content_setting = true;

[commit-bot: I haz the power, 2012-10-26 09:55:41]

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

[commit-bot: I haz the power, 2012-10-26 17:54:37]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-26 21:04:51]

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

[commit-bot: I haz the power, 2012-10-26 22:14:43]

Change committed as 164425