Improve the UI for disabling off-store extension install.

Improve the UI for disabling off-store extension install.

Move the check later into install so that:

1) The download bar goes away.
2) There's an error message.
3) The same thing happens when double-clicking a crx, on systems that associate crx with Chrome.

Also, made themes exempt from this policy since there is no danger in installing a theme. This left themes with a dangerous download bar that said 'Installing extensions, themes, and apps can harm your computer', which isn't even true for themes. It was only there to protect against clickjacking. Removed that and replaced with standard install dialog.

Had to update a bunch of strings.

BUG=55584
TEST=Run chrome with --enable-off-store-extension-install=0, navigate to a crx file. Should see error dialog.
TBR=sky

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=139217

[Aaron Boodman, 2012-05-24 09:08:53]

https://chromiumcodereview.appspot.com/10452009/diff/1/chrome/browser/downloa...
File chrome/browser/download/chrome_download_manager_delegate.cc (left):

https://chromiumcodereview.appspot.com/10452009/diff/1/chrome/browser/downloa...
chrome/browser/download/chrome_download_manager_delegate.cc:306: if
(extensions::switch_utils::IsOffStoreInstallEnabled() ||
Removing this allows us to get further into the installation process.

https://chromiumcodereview.appspot.com/10452009/diff/1/chrome/browser/downloa...
File chrome/browser/download/download_shelf_context_menu.cc (left):

https://chromiumcodereview.appspot.com/10452009/diff/1/chrome/browser/downloa...
chrome/browser/download/download_shelf_context_menu.cc:57:
(!Extension::IsExtension(download_item_->GetTargetName()) ||
It's no longer important to prevent extensions from 'opening'... they just get
blocked later.

[asanka, 2012-05-24 16:05:37]

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/browser/down...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:303: if
(IsExtensionDownload(item)) {
Can you check if the ChromeDownloadManagerDelegate::IsExtensionDownload()
implementation is correct? The logic in DownloadShelfContextMenu is based on
Extension::IsExtension() while the logic below is based on
IsExtensionDownload(). We should use the same test in both places.

[Yoyo Zhou, 2012-05-24 17:30:35]

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/app/generate...
chrome/app/generated_resources.grd:4181: +      <message
name="IDS_EXTENSION_INSTALL_APP_PROMPT_TITLE" desc="Titlebar of the extension
installation prompt window">
s/extension/app/

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/app/generate...
chrome/app/generated_resources.grd:4184: +      <message
name="IDS_EXTENSION_INSTALL_EXTENSION_PROMPT_TITLE" desc="Titlebar of the app
installation prompt window">
s/app/extension/

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/app/generate...
chrome/app/generated_resources.grd:4418: +        Extensions, apps, and user
scripts cannot be installed from this web site.
Does this text make sense for double-clicking a local .crx file and such?

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/browser/down...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:740: // When
off-store install is disabled we skip this, since in this case, we
I realize this isn't related to this change, but the previous one to disable
off-store install, but is this correct? Even if it's nontrivial to open the
download (unlike, say, a downloaded exe file) it's still a dangerous download.

[Aaron Boodman, 2012-05-24 23:55:37]

ptal

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/app/generate...
chrome/app/generated_resources.grd:4181: +      <message
name="IDS_EXTENSION_INSTALL_APP_PROMPT_TITLE" desc="Titlebar of the extension
installation prompt window">
On 2012/05/24 17:30:35, Yoyo Zhou wrote:
> s/extension/app/

Done.

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/app/generate...
chrome/app/generated_resources.grd:4184: +      <message
name="IDS_EXTENSION_INSTALL_EXTENSION_PROMPT_TITLE" desc="Titlebar of the app
installation prompt window">
On 2012/05/24 17:30:35, Yoyo Zhou wrote:
> s/app/extension/

Heh.

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/app/generate...
chrome/app/generated_resources.grd:4418: +        Extensions, apps, and user
scripts cannot be installed from this web site.
On 2012/05/24 17:30:35, Yoyo Zhou wrote:
> Does this text make sense for double-clicking a local .crx file and such?

It doesn't really make sense, but I can't think of anything better. I'll loop
the UI team in, but I'd rather land this for now.

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/browser/down...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:303: if
(IsExtensionDownload(item)) {
On 2012/05/24 16:05:37, asanka wrote:
> Can you check if the ChromeDownloadManagerDelegate::IsExtensionDownload()
> implementation is correct? The logic in DownloadShelfContextMenu is based on
> Extension::IsExtension() while the logic below is based on
> IsExtensionDownload(). We should use the same test in both places.

Yeah, the test in the menu was incomplete. I removed it and moved the check to
download_crx_util, which made more sense.

https://chromiumcodereview.appspot.com/10452009/diff/3001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:740: // When
off-store install is disabled we skip this, since in this case, we
On 2012/05/24 17:30:35, Yoyo Zhou wrote:
> I realize this isn't related to this change, but the previous one to disable
> off-store install, but is this correct? Even if it's nontrivial to open the
> download (unlike, say, a downloaded exe file) it's still a dangerous download.

The point of the dangerous download bar in the case of extensions is to prevent
malicious pages from clickjacking the install dialog. Since we don't actually
end up installing the extension or prompting the user to do so in this case,
there's no point in showing this UI. In fact, it's unnecessary stress because
you have to decide what to do with this scary UI, and then actually it doesn't
matter what you decide.

[Yoyo Zhou, 2012-05-25 00:08:15]

LGTM. I guess links from websites will be the most common place for that error
to appear anyway.

[asanka, 2012-05-25 18:28:15]

Thanks for moving the IsExtensionDownload() method. The suggestions below are to
make the state of the context menu item match the new behavior.

http://codereview.chromium.org/10452009/diff/3002/chrome/browser/download/dow...
File chrome/browser/download/download_shelf_context_menu.cc (right):

http://codereview.chromium.org/10452009/diff/3002/chrome/browser/download/dow...
chrome/browser/download/download_shelf_context_menu.cc:56:
!download_item_->IsTemporary();
The confusingly named OPEN_WHEN_COMPLETE option means "Open when complete" while
DownloadItem::IsInProgress() and means "Open" after the download is complete.

It looks like you want the option to only be enabled if:

  download_item_->CanShowInFolder() &&
  !download_item_->IsTemporary() &&
  (!download_crx_util::IsExtensionDownload(*download_item_) ||
   download_item_->IsComplete())

(The '|| download_item->IsComplete()' part won't matter much since the download
item will disappear when the extension auto opens.)

http://codereview.chromium.org/10452009/diff/3002/chrome/browser/download/dow...
chrome/browser/download/download_shelf_context_menu.cc:76: return
download_item_->GetOpenWhenComplete();
This should ideally also return true for extensions. The menu item would then
appear checked but disabled, which represents what will happen when the download
completes.

[Aaron Boodman, 2012-05-27 07:41:17]

Good ideas, done.

[commit-bot: I haz the power, 2012-05-27 07:42:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aa@chromium.org/10452009/12002

[commit-bot: I haz the power, 2012-05-27 09:23:30]

Try job failure for 10452009-12002 (retry) (retry) (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...