Add the --disable-off-store-extension-install switch.

Add the --disable-off-store-extension-install switch.

Subsequent changes will add a first-class UI in chrome://extensions/
to meet this use case.

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

[Randy Smith (Not in Mondays), 2012-04-30 18:52:39]

Ben, could you take this one?  (More because I'm trying to shard out reviews
than because I think it's in an area you have a lot of familiarity with.)  Pull
me back in if you have any questions I can be useful with.

[benjhayden, 2012-04-30 19:26:12]

Can I have a little more information about the use-case and the new UX?

https://chromiumcodereview.appspot.com/10266015/diff/11/chrome/browser/downlo...
File chrome/browser/download/download_crx_util.cc (right):

https://chromiumcodereview.appspot.com/10266015/diff/11/chrome/browser/downlo...
chrome/browser/download/download_crx_util.cc:55: if
(extensions::switch_utils::IsOffStoreInstallEnabled() &&
Can you just return this condition directly?

https://chromiumcodereview.appspot.com/10266015/diff/11/chrome/browser/downlo...
chrome/browser/download/download_crx_util.cc:55: if
(extensions::switch_utils::IsOffStoreInstallEnabled() &&
Do you anticipate needing to call IsOffStoreInstallEnabled() any other places?
If not, why not just inline it here? That seems to be a more common practice
than creating a new file-pair to wrap a single tiny static bool().

https://chromiumcodereview.appspot.com/10266015/diff/11/chrome/common/chrome_...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10266015/diff/11/chrome/common/chrome_...
chrome/common/chrome_switches.cc:338: // Enables a new off-store extension
installation UX.
Did you mean "Disables"? If not, can you either rename the flag or add a little
more explanation? A flag named "disable" that actually enables something is
confusing.

[Aaron Boodman, 2012-04-30 20:23:41]

https://chromiumcodereview.appspot.com/10266015/diff/11/chrome/browser/downlo...
File chrome/browser/download/download_crx_util.cc (right):

https://chromiumcodereview.appspot.com/10266015/diff/11/chrome/browser/downlo...
chrome/browser/download/download_crx_util.cc:55: if
(extensions::switch_utils::IsOffStoreInstallEnabled() &&
On 2012/04/30 19:26:12, benjhayden_chromium wrote:
> Can you just return this condition directly?

I think it reads more clearly to use a traditional if statement when the line
wraps. I'll change it if you feel strongly since this is downloads/.

https://chromiumcodereview.appspot.com/10266015/diff/11/chrome/browser/downlo...
chrome/browser/download/download_crx_util.cc:55: if
(extensions::switch_utils::IsOffStoreInstallEnabled() &&
On 2012/04/30 19:26:12, benjhayden_chromium wrote:
> Do you anticipate needing to call IsOffStoreInstallEnabled() any other places?

Yes.

> If not, why not just inline it here? That seems to be a more common practice
> than creating a new file-pair to wrap a single tiny static bool().

I'm trying something new here.

Guidance is that we should be able to easily kill new features if they get
delayed during launch review. It's not easy to do that with the traditional
switch approach because you have to negate the check in many places. The idea is
that in order to switch the feature from default-on to default-off, I just need
to change switches file and switch_utils.*.

[Aaron Boodman, 2012-04-30 20:24:22]

Re: use-case and ux: did you see the associated bugs?

[Aaron Boodman, 2012-04-30 20:24:28]

https://chromiumcodereview.appspot.com/10266015/diff/11/chrome/common/chrome_...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10266015/diff/11/chrome/common/chrome_...
chrome/common/chrome_switches.cc:338: // Enables a new off-store extension
installation UX.
On 2012/04/30 19:26:12, benjhayden_chromium wrote:
> Did you mean "Disables"? If not, can you either rename the flag or add a
little
> more explanation? A flag named "disable" that actually enables something is
> confusing.

Done.

[Aaron Boodman, 2012-04-30 20:50:17]

Ben, this fails some browser tests. Let me fix it up -- I'll let you know when
it's ready for review again.

[Aaron Boodman, 2012-04-30 21:49:01]

ok works now

[benjhayden, 2012-05-01 13:28:20]

Want to kick the win_rel bot again?

content_browser.lib(tcmalloc_internals_request_job.obj) : warning LNK4217:
locally defined symbol ?instance@MallocExtension@@SAPAV1@XZ (public: static
class MallocExtension * __cdecl MallocExtension::instance(void)) imported in
function "void __cdecl content::AboutTcmalloc(class
std::basic_string<char,struct std::char_traits<char>,class std::allocator<char>
> *)"
(?AboutTcmalloc@content@@YAXPAV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Z)
content_common.lib(child_thread.obj) : warning LNK4049: locally defined symbol
?instance@MallocExtension@@SAPAV1@XZ (public: static class MallocExtension *
__cdecl MallocExtension::instance(void)) imported
browser.lib(download_crx_util.obj) :error LNK2019: unresolved external symbol
"bool __cdecl extensions::switch_utils::IsOffStoreInstallEnabled(void)"
(?IsOffStoreInstallEnabled@switch_utils@extensions@@YA_NXZ) referenced in
function "bool __cdecl download_crx_util::ShouldOpenExtensionDownload(class
content::DownloadItem const &)"
(?ShouldOpenExtensionDownload@download_crx_util@@YA_NABVDownloadItem@content@@@Z)
..\build\Release\chrome.dll : fatalerror LNK1120: 1 unresolved externals

[Aaron Boodman, 2012-05-01 16:05:44]

Done, but you can review in the meantime. Anything that comes up which is
windows specific on this change is likely to be minor compiler differences.

[benjhayden, 2012-05-01 16:22:04]

LGTM