Disabled external_extensions.json and chrome://extensions in Guest mode.

Disabled external_extensions.json in Guest mode.
Added browser_defaults::extensions_enabled to control this behavior.

BUG=chromiumos:4420
TEST=Go to Guest mode and verify that no extensions are loaded by going to chrome://extensions.

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

[whywhat, 2010-09-27 14:30:46]

PTAL!

[Erik does not do reviews, 2010-09-27 14:54:48]

What problem are you trying to solve here?  Why is it important to hide
chrome://extensions?  I understand why you don't want to load any of the
preloaded extensions, but I don't understand the point of the rest.

Erik


On Mon, Sep 27, 2010 at 7:30 AM, <avayvod@chromium.org> wrote:

> Reviewers: Erik Kay, Dmitry Polukhin,
>
> Message:
> PTAL!
>
> Description:
> Disabled external_extensions.json and chrome://extensions in Guest mode.
> Added browser_defaults::extensions_enabled to control this behavior.
>
> BUG=chromiumos:4420
> TEST=Go to Guest mode and verify that chrome://extensions is not working
> and
> no extensions are loaded.
>
> Please review this at http://codereview.chromium.org/3462016/show
>
> SVN Base: http://src.chromium.org/git/chromium.git
>
> Affected files:
>  M chrome/app/chrome_dll_main.cc
>  M chrome/browser/defaults.h
>  M chrome/browser/defaults.cc
>  M chrome/browser/dom_ui/dom_ui_factory.cc
>  M chrome/browser/extensions/extensions_service.cc
>
>
> Index: chrome/app/chrome_dll_main.cc
> diff --git a/chrome/app/chrome_dll_main.cc b/chrome/app/chrome_dll_main.cc
> index
>
70dacf79cafac10699b0c8cba9171bbfd7f887eb..82ed86c070cab398a5c8437206695cb58105e6e8
> 100644
> --- a/chrome/app/chrome_dll_main.cc
> +++ b/chrome/app/chrome_dll_main.cc
> @@ -598,6 +598,7 @@ int ChromeMain(int argc, char** argv) {
>     singleton_command_line->AppendSwitch(switches::kDisableSync);
>     singleton_command_line->AppendSwitch(switches::kDisableExtensions);
>     browser_defaults::bookmarks_enabled = false;
> +    browser_defaults::extensions_enabled = false;
>   }
>  #endif
>
> Index: chrome/browser/defaults.cc
> diff --git a/chrome/browser/defaults.cc b/chrome/browser/defaults.cc
> index
>
f985c5f621fd4127b0db3d568aa7051827ed85cb..17961a2e9208947e17f5c6f4f743ba00d189dc83
> 100644
> --- a/chrome/browser/defaults.cc
> +++ b/chrome/browser/defaults.cc
> @@ -83,5 +83,6 @@ const bool kBrowserAliveWithNoWindows = false;
>  const bool kPhantomTabsEnabled = false;
>
>  bool bookmarks_enabled = true;
> +bool extensions_enabled = true;
>
>  }  // namespace browser_defaults
> Index: chrome/browser/defaults.h
> diff --git a/chrome/browser/defaults.h b/chrome/browser/defaults.h
> index
>
cd635b0689c6237937fb2ba2ab6e1d28a0f73fcd..d0fa0433a6e724b56ce064d4f79e5cb89c78cf21
> 100644
> --- a/chrome/browser/defaults.h
> +++ b/chrome/browser/defaults.h
> @@ -81,6 +81,9 @@ extern const bool kPhantomTabsEnabled;
>  // Are bookmark enabled? True by default.
>  extern bool bookmarks_enabled;
>
> +// Are extensions enabled? True by default.
> +extern bool extensions_enabled;
> +
>  }  // namespace browser_defaults
>
>  #endif  // CHROME_BROWSER_DEFAULTS_H_
> Index: chrome/browser/dom_ui/dom_ui_factory.cc
> diff --git a/chrome/browser/dom_ui/dom_ui_factory.cc
> b/chrome/browser/dom_ui/dom_ui_factory.cc
> index
>
ac3c4624665ac56e671f9328f6119dfe9f25b4d2..40fa77f13f308f434e5958a0cbc4d7a68d55be57
> 100644
> --- a/chrome/browser/dom_ui/dom_ui_factory.cc
> +++ b/chrome/browser/dom_ui/dom_ui_factory.cc
> @@ -6,6 +6,7 @@
>
>  #include "base/command_line.h"
>  #include "chrome/browser/chrome_thread.h"
> +#include "chrome/browser/defaults.h"
>  #include "chrome/browser/dom_ui/bookmarks_ui.h"
>  #include "chrome/browser/dom_ui/bug_report_ui.h"
>  #include "chrome/browser/dom_ui/downloads_ui.h"
> @@ -118,7 +119,8 @@ static DOMUIFactoryFunction
> GetDOMUIFactoryFunction(Profile* profile,
>     return &NewDOMUI<DevToolsUI>;
>   if (url.host() == chrome::kChromeUIDownloadsHost)
>     return &NewDOMUI<DownloadsUI>;
> -  if (url.host() == chrome::kChromeUIExtensionsHost)
> +  if (url.host() == chrome::kChromeUIExtensionsHost &&
> +      browser_defaults::extensions_enabled)
>     return &NewDOMUI<ExtensionsUI>;
>   if (url.host() == chrome::kChromeUIHistoryHost)
>     return &NewDOMUI<HistoryUI>;
> Index: chrome/browser/extensions/extensions_service.cc
> diff --git a/chrome/browser/extensions/extensions_service.cc
> b/chrome/browser/extensions/extensions_service.cc
> index
>
e87e744b70437c47d7e490b3620c9f953d99ead1..a354bbb3778f1becac48203e68e133c9bc6df43c
> 100644
> --- a/chrome/browser/extensions/extensions_service.cc
> +++ b/chrome/browser/extensions/extensions_service.cc
> @@ -22,6 +22,7 @@
>  #include "chrome/browser/browser_process.h"
>  #include "chrome/browser/chrome_thread.h"
>  #include "chrome/browser/debugger/devtools_manager.h"
> +#include "chrome/browser/defaults.h"
>  #include "chrome/browser/extensions/crx_installer.h"
>  #include "chrome/browser/extensions/extension_accessibility_api.h"
>  #include "chrome/browser/extensions/extension_bookmarks_module.h"
> @@ -290,6 +291,9 @@ ExtensionsServiceBackend::ExtensionsServiceBackend(
>           install_directory_(install_directory),
>           alert_on_error_(false),
>           external_extension_added_(false) {
> +  if (!browser_defaults::extensions_enabled)
> +    return;
> +
>   // TODO(aa): This ends up doing blocking IO on the UI thread because it
> reads
>   // pref data in the ctor and that is called on the UI thread. Would be
> better
>   // to re-read data each time we list external extensions, anyway.
>
>
>

[whywhat, 2010-09-27 15:10:19]

I reverted disabling of chrome://extensions.
I only need to not install default external extensions.

On 2010/09/27 14:54:48, Erik Kay wrote:
> What problem are you trying to solve here?  Why is it important to hide
> chrome://extensions?  I understand why you don't want to load any of the
> preloaded extensions, but I don't understand the point of the rest.
> 
> Erik
> 
> 
> On Mon, Sep 27, 2010 at 7:30 AM, <mailto:avayvod@chromium.org> wrote:
> 
> > Reviewers: Erik Kay, Dmitry Polukhin,
> >
> > Message:
> > PTAL!
> >
> > Description:
> > Disabled external_extensions.json and chrome://extensions in Guest mode.
> > Added browser_defaults::extensions_enabled to control this behavior.
> >
> > BUG=chromiumos:4420
> > TEST=Go to Guest mode and verify that chrome://extensions is not working
> > and
> > no extensions are loaded.
> >
> > Please review this at http://codereview.chromium.org/3462016/show
> >
> > SVN Base: http://src.chromium.org/git/chromium.git
> >
> > Affected files:
> >  M chrome/app/chrome_dll_main.cc
> >  M chrome/browser/defaults.h
> >  M chrome/browser/defaults.cc
> >  M chrome/browser/dom_ui/dom_ui_factory.cc
> >  M chrome/browser/extensions/extensions_service.cc
> >
> >
> > Index: chrome/app/chrome_dll_main.cc
> > diff --git a/chrome/app/chrome_dll_main.cc b/chrome/app/chrome_dll_main.cc
> > index
> >
>
70dacf79cafac10699b0c8cba9171bbfd7f887eb..82ed86c070cab398a5c8437206695cb58105e6e8
> > 100644
> > --- a/chrome/app/chrome_dll_main.cc
> > +++ b/chrome/app/chrome_dll_main.cc
> > @@ -598,6 +598,7 @@ int ChromeMain(int argc, char** argv) {
> >     singleton_command_line->AppendSwitch(switches::kDisableSync);
> >     singleton_command_line->AppendSwitch(switches::kDisableExtensions);
> >     browser_defaults::bookmarks_enabled = false;
> > +    browser_defaults::extensions_enabled = false;
> >   }
> >  #endif
> >
> > Index: chrome/browser/defaults.cc
> > diff --git a/chrome/browser/defaults.cc b/chrome/browser/defaults.cc
> > index
> >
>
f985c5f621fd4127b0db3d568aa7051827ed85cb..17961a2e9208947e17f5c6f4f743ba00d189dc83
> > 100644
> > --- a/chrome/browser/defaults.cc
> > +++ b/chrome/browser/defaults.cc
> > @@ -83,5 +83,6 @@ const bool kBrowserAliveWithNoWindows = false;
> >  const bool kPhantomTabsEnabled = false;
> >
> >  bool bookmarks_enabled = true;
> > +bool extensions_enabled = true;
> >
> >  }  // namespace browser_defaults
> > Index: chrome/browser/defaults.h
> > diff --git a/chrome/browser/defaults.h b/chrome/browser/defaults.h
> > index
> >
>
cd635b0689c6237937fb2ba2ab6e1d28a0f73fcd..d0fa0433a6e724b56ce064d4f79e5cb89c78cf21
> > 100644
> > --- a/chrome/browser/defaults.h
> > +++ b/chrome/browser/defaults.h
> > @@ -81,6 +81,9 @@ extern const bool kPhantomTabsEnabled;
> >  // Are bookmark enabled? True by default.
> >  extern bool bookmarks_enabled;
> >
> > +// Are extensions enabled? True by default.
> > +extern bool extensions_enabled;
> > +
> >  }  // namespace browser_defaults
> >
> >  #endif  // CHROME_BROWSER_DEFAULTS_H_
> > Index: chrome/browser/dom_ui/dom_ui_factory.cc
> > diff --git a/chrome/browser/dom_ui/dom_ui_factory.cc
> > b/chrome/browser/dom_ui/dom_ui_factory.cc
> > index
> >
>
ac3c4624665ac56e671f9328f6119dfe9f25b4d2..40fa77f13f308f434e5958a0cbc4d7a68d55be57
> > 100644
> > --- a/chrome/browser/dom_ui/dom_ui_factory.cc
> > +++ b/chrome/browser/dom_ui/dom_ui_factory.cc
> > @@ -6,6 +6,7 @@
> >
> >  #include "base/command_line.h"
> >  #include "chrome/browser/chrome_thread.h"
> > +#include "chrome/browser/defaults.h"
> >  #include "chrome/browser/dom_ui/bookmarks_ui.h"
> >  #include "chrome/browser/dom_ui/bug_report_ui.h"
> >  #include "chrome/browser/dom_ui/downloads_ui.h"
> > @@ -118,7 +119,8 @@ static DOMUIFactoryFunction
> > GetDOMUIFactoryFunction(Profile* profile,
> >     return &NewDOMUI<DevToolsUI>;
> >   if (url.host() == chrome::kChromeUIDownloadsHost)
> >     return &NewDOMUI<DownloadsUI>;
> > -  if (url.host() == chrome::kChromeUIExtensionsHost)
> > +  if (url.host() == chrome::kChromeUIExtensionsHost &&
> > +      browser_defaults::extensions_enabled)
> >     return &NewDOMUI<ExtensionsUI>;
> >   if (url.host() == chrome::kChromeUIHistoryHost)
> >     return &NewDOMUI<HistoryUI>;
> > Index: chrome/browser/extensions/extensions_service.cc
> > diff --git a/chrome/browser/extensions/extensions_service.cc
> > b/chrome/browser/extensions/extensions_service.cc
> > index
> >
>
e87e744b70437c47d7e490b3620c9f953d99ead1..a354bbb3778f1becac48203e68e133c9bc6df43c
> > 100644
> > --- a/chrome/browser/extensions/extensions_service.cc
> > +++ b/chrome/browser/extensions/extensions_service.cc
> > @@ -22,6 +22,7 @@
> >  #include "chrome/browser/browser_process.h"
> >  #include "chrome/browser/chrome_thread.h"
> >  #include "chrome/browser/debugger/devtools_manager.h"
> > +#include "chrome/browser/defaults.h"
> >  #include "chrome/browser/extensions/crx_installer.h"
> >  #include "chrome/browser/extensions/extension_accessibility_api.h"
> >  #include "chrome/browser/extensions/extension_bookmarks_module.h"
> > @@ -290,6 +291,9 @@ ExtensionsServiceBackend::ExtensionsServiceBackend(
> >           install_directory_(install_directory),
> >           alert_on_error_(false),
> >           external_extension_added_(false) {
> > +  if (!browser_defaults::extensions_enabled)
> > +    return;
> > +
> >   // TODO(aa): This ends up doing blocking IO on the UI thread because it
> > reads
> >   // pref data in the ctor and that is called on the UI thread. Would be
> > better
> >   // to re-read data each time we list external extensions, anyway.
> >
> >
> >
>

[Erik does not do reviews, 2010-09-27 17:28:19]

http://codereview.chromium.org/3462016/diff/1006/6004
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3462016/diff/1006/6004#newcode294
chrome/browser/extensions/extensions_service.cc:294: if
(!browser_defaults::extensions_enabled)
why not check this at the same place we check the command-line switch? (the
constructor)  This makes it seem like we have two different concepts of disabled
(we shouldn't).  Also, there should be an accessor that checks the command-line
flag and this value so that we can fix any place that checks the command-line
value.

Further, the enterprise folks would probably love to be able to trigger this via
admin policy.  Touch base with them to see what the best way is to hook into
their stuff.  It could be that they've already done something relevant here.

[whywhat, 2010-09-27 17:53:46]

http://codereview.chromium.org/3462016/diff/1006/6004
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3462016/diff/1006/6004#newcode294
chrome/browser/extensions/extensions_service.cc:294: if
(!browser_defaults::extensions_enabled)
On 2010/09/27 17:28:19, Erik Kay wrote:
> why not check this at the same place we check the command-line switch? (the
> constructor)  This makes it seem like we have two different concepts of
disabled
> (we shouldn't).  Also, there should be an accessor that checks the
command-line
> flag and this value so that we can fix any place that checks the command-line
> value.

It's just that not creating backend in ctor would result in many NULL pointer
checks, so it's simpler to not load default extensions in the backend. I changed
the code so we only check command line and prefs and pass the result to the
backend.

I wanted to add browser default to be able in the future to disable extensions
without chrome restart. I think though that it's possible with command line
argument if we check it each time extensions_enabled() called. Is it okay to
address command line for current process each time the method is called?

> 
> Further, the enterprise folks would probably love to be able to trigger this
via
> admin policy.  Touch base with them to see what the best way is to hook into
> their stuff.  It could be that they've already done something relevant here.

Could you give me contacts of someone or just add him/her to this code review?
If there's something already I could use to disable extensions on the fly, it
would be awesome!

[Erik does not do reviews, 2010-09-27 18:38:54]

http://codereview.chromium.org/3462016/diff/1006/6004
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3462016/diff/1006/6004#newcode294
chrome/browser/extensions/extensions_service.cc:294: if
(!browser_defaults::extensions_enabled)
On 2010/09/27 17:53:46, whywhat wrote:
> On 2010/09/27 17:28:19, Erik Kay wrote:
> > why not check this at the same place we check the command-line switch? (the
> > constructor)  This makes it seem like we have two different concepts of
> disabled
> > (we shouldn't).  Also, there should be an accessor that checks the
> command-line
> > flag and this value so that we can fix any place that checks the
command-line
> > value.
> 
> It's just that not creating backend in ctor would result in many NULL pointer
> checks, so it's simpler to not load default extensions in the backend. I
changed
> the code so we only check command line and prefs and pass the result to the
> backend.

I don't understand.  Look at ExtensionsService::ExtensionsService(), not the
backend.


> I wanted to add browser default to be able in the future to disable extensions
> without chrome restart. I think though that it's possible with command line
> argument if we check it each time extensions_enabled() called. Is it okay to
> address command line for current process each time the method is called?
> 
> > 
> > Further, the enterprise folks would probably love to be able to trigger this
> via
> > admin policy.  Touch base with them to see what the best way is to hook into
> > their stuff.  It could be that they've already done something relevant here.
> 
> Could you give me contacts of someone or just add him/her to this code review?
> If there's something already I could use to disable extensions on the fly, it
> would be awesome!
> 
> 

Actually, looking at ExtensionsService::ExtensionsService(), it looks like they
already added the hooks to make this driven off of a preference.  Given this, I
think most of your CL is unnecessary.  You just need to prepopulate that pref
with a default value.

If you want to talk to someone involved with the enterprise stuff, jochen@ or
pamg@ could probably tell you the right person to talk to.

[whywhat, 2010-09-27 18:49:34]

http://codereview.chromium.org/3462016/diff/1006/6004
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3462016/diff/1006/6004#newcode294
chrome/browser/extensions/extensions_service.cc:294: if
(!browser_defaults::extensions_enabled)
> Actually, looking at ExtensionsService::ExtensionsService(), it looks like
they
> already added the hooks to make this driven off of a preference.  Given this,
I
> think most of your CL is unnecessary.  You just need to prepopulate that pref
> with a default value.

We already pass --disable-extensions when starting Chrome in Guest mode. The
problem I'm fixing in this CL is that extensions from external_extensions.json
are loaded in ExtensionsServiceBackend ctor despite of flag or pref reference.
And despite the flag these extensions are shown in chrome://extensions. My CL
fixes only this issue, nothing more.

> 
> If you want to talk to someone involved with the enterprise stuff, jochen@ or
> pamg@ could probably tell you the right person to talk to.
>

[Erik does not do reviews, 2010-09-27 19:05:57]

OK.  I didn't see your last update.

LGTM with a couple of nits.

http://codereview.chromium.org/3462016/diff/15001/16001
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3462016/diff/15001/16001#newcode176
chrome/browser/extensions/extensions_service.cc:176: bool
IsExtensionsEnabled(const CommandLine* command_line,
Since this isn't used by anything else, it seems unnecessary to pull it out.

http://codereview.chromium.org/3462016/diff/15001/16001#newcode297
chrome/browser/extensions/extensions_service.cc:297: bool
load_default_extensions)
these aren't just default extensions.  Please use the same "external"
terminology.

[whywhat, 2010-09-27 20:12:28]

http://codereview.chromium.org/3462016/diff/15001/16001
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3462016/diff/15001/16001#newcode176
chrome/browser/extensions/extensions_service.cc:176: bool
IsExtensionsEnabled(const CommandLine* command_line,
On 2010/09/27 19:05:59, Erik Kay wrote:
> Since this isn't used by anything else, it seems unnecessary to pull it out.

Right. Done.

http://codereview.chromium.org/3462016/diff/15001/16001#newcode297
chrome/browser/extensions/extensions_service.cc:297: bool
load_default_extensions)
On 2010/09/27 19:05:59, Erik Kay wrote:
> these aren't just default extensions.  Please use the same "external"
> terminology.

Sure. Done.

[Erik does not do reviews, 2010-09-27 21:32:08]

LGTM

On Mon, Sep 27, 2010 at 1:12 PM, <avayvod@chromium.org> wrote:

>
> http://codereview.chromium.org/3462016/diff/15001/16001
> File chrome/browser/extensions/extensions_service.cc (right):
>
> http://codereview.chromium.org/3462016/diff/15001/16001#newcode176
> chrome/browser/extensions/extensions_service.cc:176: bool
> IsExtensionsEnabled(const CommandLine* command_line,
> On 2010/09/27 19:05:59, Erik Kay wrote:
>
>> Since this isn't used by anything else, it seems unnecessary to pull
>>
> it out.
>
> Right. Done.
>
>
> http://codereview.chromium.org/3462016/diff/15001/16001#newcode297
> chrome/browser/extensions/extensions_service.cc:297: bool
> load_default_extensions)
> On 2010/09/27 19:05:59, Erik Kay wrote:
>
>> these aren't just default extensions.  Please use the same "external"
>> terminology.
>>
>
> Sure. Done.
>
>
> http://codereview.chromium.org/3462016/show
>