Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(64)

Issue 11150002: New post-sideload UI: (Closed)

Created:
7 years, 9 months ago by Matt Perry
Modified:
7 years, 9 months ago
CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, oshima+watch_chromium.org
Visibility:
Public.

Description

New post-sideload UI: - External extensions are installed initially disabled. - Global error appears in menu, which will show an install dialog when clicked. TBR=sky@chromium.org BUG=137469, 131510 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162380

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Total comments: 35

Patch Set 4 : jyasskin #

Patch Set 5 : failed upload #

Patch Set 6 : fixed unit tests, disable on chromeos, feature switch #

Total comments: 15

Patch Set 7 : sail #

Total comments: 6

Patch Set 8 : yoz #

Total comments: 4

Patch Set 9 : nits #

Patch Set 10 : sync #

Patch Set 11 : failed upload #

Patch Set 12 : ... #

Patch Set 13 : ... #

Patch Set 14 : !!! #

Unified diffs Side-by-side diffs Delta from patch set Stats (+662 lines, -55 lines) Patch
M chrome/app/chrome_command_ids.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 2 chunks +21 lines, -1 line 0 comments Download
M chrome/app/theme/theme_resources.grd View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_browsertest.h View 1 2 3 4 5 6 7 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_browsertest.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/extensions/extension_install_prompt.h View 1 2 3 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_install_prompt.cc View 1 2 3 4 5 7 chunks +33 lines, -8 lines 0 comments Download
M chrome/browser/extensions/extension_prefs.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_prefs.cc View 1 2 3 5 chunks +17 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_service.h View 1 2 3 3 chunks +10 lines, -3 lines 0 comments Download
M chrome/browser/extensions/extension_service.cc View 1 2 3 4 5 6 7 11 chunks +77 lines, -20 lines 0 comments Download
M chrome/browser/extensions/extension_service_unittest.cc View 1 2 3 4 5 6 7 8 7 chunks +90 lines, -11 lines 0 comments Download
A chrome/browser/extensions/external_install_ui.h View 1 2 3 4 5 1 chunk +26 lines, -0 lines 0 comments Download
A chrome/browser/extensions/external_install_ui.cc View 1 2 3 4 5 6 7 8 1 chunk +297 lines, -0 lines 0 comments Download
M chrome/browser/extensions/pending_extension_manager.cc View 4 5 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/global_error/global_error.h View 1 2 3 4 5 6 2 chunks +11 lines, -0 lines 0 comments Download
M chrome/browser/ui/global_error/global_error.cc View 1 chunk +11 lines, -0 lines 0 comments Download
M chrome/browser/ui/global_error/global_error_service.cc View 1 chunk +11 lines, -3 lines 0 comments Download
M chrome/browser/ui/global_error/global_error_service_unittest.cc View 1 3 chunks +14 lines, -5 lines 0 comments Download
M chrome/chrome_browser_extensions.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/common/extensions/feature_switch.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/extensions/feature_switch.cc View 1 2 3 4 5 6 7 3 chunks +9 lines, -1 line 0 comments Download

Messages

Total messages: 22 (0 generated)
Matt Perry
http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/extension_install_prompt.cc File chrome/browser/extensions/extension_install_prompt.cc (right): http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/extension_install_prompt.cc#newcode453 chrome/browser/extensions/extension_install_prompt.cc:453: show_dialog_callback_ = // @@@MP I'll remove this once http://codereview.chromium.org/11131002/ ...
7 years, 9 months ago (2012-10-13 00:33:18 UTC) #1
Matt Perry
I'm still working on a browsertest, but wanted to get this out sooner due to ...
7 years, 9 months ago (2012-10-13 00:34:33 UTC) #2
Jeffrey Yasskin
Didn't get all the way through, but here are some initial comments. http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd ...
7 years, 9 months ago (2012-10-13 21:17:34 UTC) #3
Jeffrey Yasskin
The rest of my comments: http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/extension_service.cc File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/11150002/diff/18/chrome/browser/extensions/extension_service.cc#newcode857 chrome/browser/extensions/extension_service.cc:857: void ExtensionService::EnableExtension(const std::string& extension_id) ...
7 years, 9 months ago (2012-10-15 00:43:59 UTC) #4
Matt Perry
http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resources.grd#newcode4622 chrome/app/generated_resources.grd:4622: Add "<ph name="EXTENSION_NAME">$1<ex>Gmail Checker</ex></ph>"? On 2012/10/13 21:17:35, Jeffrey Yasskin ...
7 years, 9 months ago (2012-10-15 19:13:38 UTC) #5
Matt Perry
-jyasskin +yoz Latest version wraps the feature in a command-line flag. Also, I've fixed some ...
7 years, 9 months ago (2012-10-16 21:09:50 UTC) #6
Matt Perry
sail: can you please review global_error/*
7 years, 9 months ago (2012-10-16 21:15:31 UTC) #7
sail
http://codereview.chromium.org/11150002/diff/15001/chrome/browser/ui/global_error/global_error.h File chrome/browser/ui/global_error/global_error.h (right): http://codereview.chromium.org/11150002/diff/15001/chrome/browser/ui/global_error/global_error.h#newcode23 chrome/browser/ui/global_error/global_error.h:23: NUM_SEVERITY, It seems like this isn't used anywhere. Can ...
7 years, 9 months ago (2012-10-16 21:26:32 UTC) #8
Matt Perry
http://codereview.chromium.org/11150002/diff/15001/chrome/browser/ui/global_error/global_error.h File chrome/browser/ui/global_error/global_error.h (right): http://codereview.chromium.org/11150002/diff/15001/chrome/browser/ui/global_error/global_error.h#newcode23 chrome/browser/ui/global_error/global_error.h:23: NUM_SEVERITY, On 2012/10/16 21:26:32, sail wrote: > It seems ...
7 years, 9 months ago (2012-10-16 21:36:31 UTC) #9
sail
global_error/* LGTM
7 years, 9 months ago (2012-10-16 21:42:58 UTC) #10
Yoyo Zhou
First pass of comments - I still have tests to look at. http://codereview.chromium.org/11150002/diff/18/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd ...
7 years, 9 months ago (2012-10-16 23:28:05 UTC) #11
Matt Perry
http://codereview.chromium.org/11150002/diff/15001/chrome/app/chrome_command_ids.h File chrome/app/chrome_command_ids.h (right): http://codereview.chromium.org/11150002/diff/15001/chrome/app/chrome_command_ids.h#newcode189 chrome/app/chrome_command_ids.h:189: #define IDC_EXTERNAL_EXTENSION_ALERT 40035 On 2012/10/16 23:28:05, Yoyo Zhou wrote: ...
7 years, 9 months ago (2012-10-17 00:02:44 UTC) #12
Yoyo Zhou
LGTM, only nits http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/extension_service.cc File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/11150002/diff/15003/chrome/browser/extensions/extension_service.cc#newcode1844 chrome/browser/extensions/extension_service.cc:1844: // Note: this can sometimes overcount ...
7 years, 9 months ago (2012-10-17 00:23:55 UTC) #13
Matt Perry
http://codereview.chromium.org/11150002/diff/26002/chrome/browser/extensions/extension_service_unittest.cc File chrome/browser/extensions/extension_service_unittest.cc (right): http://codereview.chromium.org/11150002/diff/26002/chrome/browser/extensions/extension_service_unittest.cc#newcode5579 chrome/browser/extensions/extension_service_unittest.cc:5579: // Should return false, meaning there aren't any extensions ...
7 years, 9 months ago (2012-10-17 00:35:20 UTC) #14
Matt Perry
sky: OWNERS review for chrome/browser/ui
7 years, 9 months ago (2012-10-17 00:36:24 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/11150002/27002
7 years, 9 months ago (2012-10-17 01:05:54 UTC) #16
Jeffrey Yasskin
http://codereview.chromium.org/11150002/diff/15001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/11150002/diff/15001/chrome/app/generated_resources.grd#newcode5322 chrome/app/generated_resources.grd:5322: New extension added (<ph name="EXTENSION_NAME">$1<ex>Babylon Toolbar</ex></ph>) On 2012/10/15 19:13:38, ...
7 years, 9 months ago (2012-10-17 07:08:55 UTC) #17
Jeffrey Yasskin
But LGTM from me too. The comment fixes aren't that important.
7 years, 9 months ago (2012-10-17 07:10:09 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/11150002/27002
7 years, 9 months ago (2012-10-17 11:16:47 UTC) #19
commit-bot: I haz the power
Change committed as 162380
7 years, 9 months ago (2012-10-17 13:18:06 UTC) #20
sky
LGTM
7 years, 9 months ago (2012-10-17 14:11:18 UTC) #21
Matt Perry
7 years, 9 months ago (2012-10-17 18:38:17 UTC) #22
http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/11150002/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1846: //
UpdateExternalExtensionAlert may get called multiple times). But that
On 2012/10/17 07:08:56, Jeffrey Yasskin wrote:
> On 2012/10/15 19:13:38, Matt Perry wrote:
> > On 2012/10/13 21:17:35, Jeffrey Yasskin wrote:
> > > Won't it get called for different extensions in that case?
> > 
> > Not necessarily. This method is called both on startup (1) and each time an
> > install finishes (2). I can't remember the details now, but there was a
> > particular order that resulted in this getting called twice: once when only
> > extensionA is installed, then again with extensionA+extensionB. If we end up
> > choosing to display extensionB the second time, then the first call for
> > extensionA was an overcount.
> 
> Since this only increments the count for the extension it chooses to install,
> I'm not seeing how it could overcount. Could you figure that out, and include
it
> in the comment, or weaken the comment to just say that we don't care about any
> possible overcount, but not that there definitely is one?

FYI I removed this comment entirely after restructuring the code so that this
doesn't overcount anymore.

Powered by Google App Engine
This is Rietveld 408576698