Implement sideload wipeout for Extensions.

Implement sideload wipeout for Extensions.

BUG=154624
TEST=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=163350

[Finnur, 2012-10-19 18:06:37]

Aaron, can you review? (I'll upload screenshots to the bug)
Sky, can you take a look at selective Views parts?

Also, OWNERS approval needed from:

aa: 

chrome/app/generated_resources.grd
chrome/app/resources/locale_settings.grd
chrome/browser/extensions/extension_prefs.h
chrome/browser/extensions/extension_prefs.cc
chrome/browser/extensions/extension_service.h
chrome/browser/extensions/extension_service.cc
chrome/common/extensions/extension.h
chrome/common/extensions/feature_switch.h
chrome/common/extensions/feature_switch.cc

sail:

chrome/app/theme/theme_resources.grd

miket:

chrome/browser/resources/extensions/extension_list.js
chrome/browser/resources/extensions/extensions.css
chrome/browser/resources/extensions/extensions.html
chrome/browser/resources/extensions/extensions.js

dbeam:

chrome/browser/resources/uber/uber_shared.css

jhawkins:

chrome/browser/ui/webui/extensions/extension_settings_handler.cc

sky (can you do a full review on these files):

chrome/browser/ui/browser_ui_prefs.cc
chrome/browser/ui/views/extensions/disabled_extensions_view.h
chrome/browser/ui/views/extensions/disabled_extensions_view.cc
chrome/browser/ui/views/toolbar_view.h
chrome/browser/ui/views/toolbar_view.cc
chrome/chrome_browser_ui.gypi
chrome/common/chrome_switches.h
chrome/common/chrome_switches.cc
chrome/common/pref_names.h
chrome/common/pref_names.cc
chrome/common/url_constants.h
chrome/common/url_constants.cc

[miket_OOO, 2012-10-19 18:19:13]

> miket:
> 
> chrome/browser/resources/extensions/extension_list.js
> chrome/browser/resources/extensions/extensions.css
> chrome/browser/resources/extensions/extensions.html
> chrome/browser/resources/extensions/extensions.js

OWNERS LGTM.

[James Hawkins, 2012-10-19 18:23:06]

lgtm

[Aaron Boodman, 2012-10-19 18:40:45]

http://codereview.chromium.org/11189094/diff/1/chrome/app/generated_resources...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11189094/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:10144: To make Chrome even faster, we've
automatically disabled some extensions that may not have been installed by you. 
You can enable or disable any extension below.
Do we use the double-space in Chrome? It's not recommended by any English style
guides.

http://codereview.chromium.org/11189094/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:10144: To make Chrome even faster, we've
automatically disabled some extensions that may not have been installed by you. 
You can enable or disable any extension below.
The word "extension" needs to be substituted and be either 'app' or 'extension'.

http://codereview.chromium.org/11189094/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/11189094/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_prefs.cc:2212: // TODO(finnur): Should this
be sync'able?
No.

http://codereview.chromium.org/11189094/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11189094/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_service.cc:2195: if (done)
You could have a command-line flag to make this easy to test permanently
(--sideload-wipeout-always).

http://codereview.chromium.org/11189094/diff/1/chrome/browser/ui/views/extens...
File chrome/browser/ui/views/extensions/disabled_extensions_view.cc (right):

http://codereview.chromium.org/11189094/diff/1/chrome/browser/ui/views/extens...
chrome/browser/ui/views/extensions/disabled_extensions_view.cc:54: #if
defined(OS_WIN)
Can you reverse this and return early?

Also add a comment that this problem mainly occurs on Windows.

http://codereview.chromium.org/11189094/diff/1/chrome/browser/ui/views/extens...
File chrome/browser/ui/views/extensions/disabled_extensions_view.h (right):

http://codereview.chromium.org/11189094/diff/1/chrome/browser/ui/views/extens...
chrome/browser/ui/views/extensions/disabled_extensions_view.h:21: class
DisabledExtensionsView : public views::BubbleDelegateView,
Needs class-level comment.

http://codereview.chromium.org/11189094/diff/1/chrome/browser/ui/views/extens...
chrome/browser/ui/views/extensions/disabled_extensions_view.h:32: virtual
~DisabledExtensionsView();
Does this view get deleted through View::~View()? If so, make this private.

http://codereview.chromium.org/11189094/diff/1/chrome/browser/ui/views/extens...
chrome/browser/ui/views/extensions/disabled_extensions_view.h:34: //
views::ButtonListener implementation.
All of these can be private unless you are using them in unit tests.

http://codereview.chromium.org/11189094/diff/1/chrome/browser/ui/webui/extens...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/11189094/diff/1/chrome/browser/ui/webui/extens...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:148: string16
automatically_disabled_text;
insert blank line here

http://codereview.chromium.org/11189094/diff/1/chrome/common/extensions/featu...
File chrome/common/extensions/feature_switch.cc (right):

http://codereview.chromium.org/11189094/diff/1/chrome/common/extensions/featu...
chrome/common/extensions/feature_switch.cc:34: // TODO(finnur): Change to
DEFAULT_DISABLED before checking in.
Depends on timing. If we get this in by Monday, I think that perhaps it should
be on by default so that it gets testing.

[Aaron Boodman, 2012-10-19 18:45:02]

http://codereview.chromium.org/11189094/diff/1/chrome/common/extensions/featu...
File chrome/common/extensions/feature_switch.cc (right):

http://codereview.chromium.org/11189094/diff/1/chrome/common/extensions/featu...
chrome/common/extensions/feature_switch.cc:34: // TODO(finnur): Change to
DEFAULT_DISABLED before checking in.
On 2012/10/19 18:40:45, Aaron Boodman wrote:
> Depends on timing. If we get this in by Monday, I think that perhaps it should
> be on by default so that it gets testing.

Thought about this a little more. Think you should make it DEFAULT_DISABLED (as
your comment indicates). If we have time to do testing before next build and are
comfortable, then we will flip to ENABLED.

[sail, 2012-10-19 19:07:34]

theme/* LGTM

[Dan Beam, 2012-10-19 19:08:54]

http://codereview.chromium.org/11189094/diff/7003/chrome/browser/resources/ub...
File chrome/browser/resources/uber/uber_shared.css (right):

http://codereview.chromium.org/11189094/diff/7003/chrome/browser/resources/ub...
chrome/browser/resources/uber/uber_shared.css:171: padding: 8px 8px 8px 26px;
this needs to work in RTL, i.e.

 -webkit-padding-start: 26px;
 -webkit-padding-end: 8px;
 padding-bottom: 8px;
 padding-top: 8px;

[sky, 2012-10-19 21:41:35]

LGTM

https://codereview.chromium.org/11189094/diff/7003/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/disabled_extensions_view.cc (right):

https://codereview.chromium.org/11189094/diff/7003/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/disabled_extensions_view.cc:102:
views::Button* sender, const ui::Event& event) {
nit each param on its own line.

https://codereview.chromium.org/11189094/diff/7003/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/disabled_extensions_view.h (right):

https://codereview.chromium.org/11189094/diff/7003/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/disabled_extensions_view.h:21: class
DisabledExtensionsView : public views::BubbleDelegateView,
Add description.

https://codereview.chromium.org/11189094/diff/7003/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/disabled_extensions_view.h:27: static bool
MaybeShow(Browser* browser, views::View* anchor_view);
nit: constructor/destructor before other methods.

https://codereview.chromium.org/11189094/diff/7003/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/disabled_extensions_view.h:44: bool is_add,
View* parent, View* child) OVERRIDE;
nit: wrap each param to its own line.

https://codereview.chromium.org/11189094/diff/7003/chrome/browser/ui/views/to...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://codereview.chromium.org/11189094/diff/7003/chrome/browser/ui/views/to...
chrome/browser/ui/views/toolbar_view.cc:332: if
(DisabledExtensionsView::MaybeShow(browser_, app_menu_))
nit: convert to a single if.

https://codereview.chromium.org/11189094/diff/7003/chrome/common/chrome_switc...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/11189094/diff/7003/chrome/common/chrome_switc...
chrome/common/chrome_switches.h:176: extern const char kScriptBadges[];
Move kScriptBadges and your switch so that everything is sorted again
(hopefully).

[Finnur, 2012-10-21 19:55:42]

aa, dbeam: PTAL.

http://codereview.chromium.org/11189094/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11189094/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_service.cc:2195: if (done)
Deleting the pref value is easy enough, for now (given the constraints).

On 2012/10/19 18:40:45, Aaron Boodman wrote:
> You could have a command-line flag to make this easy to test permanently
> (--sideload-wipeout-always).

http://codereview.chromium.org/11189094/diff/1/chrome/browser/ui/views/extens...
File chrome/browser/ui/views/extensions/disabled_extensions_view.h (right):

http://codereview.chromium.org/11189094/diff/1/chrome/browser/ui/views/extens...
chrome/browser/ui/views/extensions/disabled_extensions_view.h:32: virtual
~DisabledExtensionsView();
Through ~Widget and ~RootView, yes. Marking as Private works.

On 2012/10/19 18:40:45, Aaron Boodman wrote:
> Does this view get deleted through View::~View()? If so, make this private.

[Dan Beam, 2012-10-22 02:11:16]

lgtm

[Aaron Boodman, 2012-10-22 02:34:00]

lgtm

http://codereview.chromium.org/11189094/diff/4010/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/11189094/diff/4010/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:214: const ExtensionSet*
GenerateSideloadWipoutExtensionsSet() const;
kinda cryptic name: maybe "GetWipedOutExtensions()" ?

[Aaron Boodman, 2012-10-22 18:57:34]

Pressed the 'commit' button. The remaining nits (including the ui nits in the
bug) can be a different CL.

[commit-bot: I haz the power, 2012-10-22 18:57:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/finnur@chromium.org/11189094/48