Refactoring ExtenionInstallUI to abstract the Browser references.

Refactoring ExtensionInstallUI to abstract the Browser references.

As part of the effort to build unit-tests on Android, this CL abstracts out
references to the Browser object from the extension install ui code.
For this, ExtensionInstallUI has been renamed ExtensionInstallPrompt and the
UI specific bits have been moved to a new class that took the old name
ExtensionInstallUI.

BUG=None
TEST=You should be able to install an extension.
     All browser tests/unit tests should pass.


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

[Jay Civelli, 2012-05-23 17:38:43]

Hi Yoyo,
Based on your previous suggestions in http://codereview.chromium.org/10388008/ I
decided to start a new CL.
Let me know if I am in the right direction.

Thanks!

Jay

[Yoyo Zhou, 2012-05-23 23:17:49]

This approach looks pretty good. I left some draft comments as well. I'll let
Aaron comment if he has any opinions on it.

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/auto...
File chrome/browser/automation/testing_automation_provider.cc (right):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/auto...
chrome/browser/automation/testing_automation_provider.cc:3904: const Extension*
theme =
nit: This change looks spurious.

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/down...
File chrome/browser/download/download_crx_util.h (right):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/down...
chrome/browser/download/download_crx_util.h:25: // Allow tests to install a mock
extension install Prompt object, to fake
nit: just ExtensionInstallPrompt

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_install_prompt.cc:86:
ExtensionInstallPrompt::Prompt::Prompt(PromptType type)
Names like this make me think the structure of the code is not ideal. But such
refactoring is probably beyond the scope of this CL.

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_install_prompt.cc:336: SetIcon(icon);
It seems surprising to me that these are necessary.
http://codereview.chromium.org/5690004 is where they were added. Maybe aa can
comment on whether they are still useful.

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/extension_install_prompt.h (right):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_install_prompt.h:214: virtual void
OnInstallSuccess(const extensions::Extension* extension,
I was originally thinking that OnInstallSuccess and OnInstallFailure could be
moved entirely to the ExtensionInstallUI. Does that look possible?

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/extension_install_ui.h (left):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_install_ui.h:36: // Displays all the UI
around extension installation.
This comment (or something like it) should still be here, even if just to
describe this as an interface.

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_install_ui.h:212: // Installation was
successful. This is declared virtual for testing.
These comments may not make sense anymore, but there should be some comment for
these functions.

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/extension_install_ui.h (right):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_install_ui.h:41: Profile* profile_;
Consider moving this to install_ui_default. As long as the base class doesn't do
anything with profile_, there is no need to keep it here.

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/extension_management_api.h (right):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_management_api.h:112:
scoped_ptr<ExtensionInstallPrompt> install_ui_;
nit: rename this variable

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/extension_navigation_observer.h (right):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_navigation_observer.h:55:
scoped_ptr<ExtensionInstallPrompt> extension_install_ui_;
nit: rename

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/extension_webstore_private_api.h (right):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
chrome/browser/extensions/extension_webstore_private_api.h:151:
scoped_ptr<ExtensionInstallPrompt> install_ui_;
nit: rename

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/exte...
chrome/browser/extensions/webstore_inline_installer.cc:264: average_rating_
>ExtensionInstallPrompt::kMaxExtensionRating) {
nit: missing space

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/ui/v...
File chrome/browser/ui/views/extensions/extension_installed_bubble.cc (right):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/ui/v...
chrome/browser/ui/views/extensions/extension_installed_bubble.cc:16: #include
"chrome/browser/extensions/extension_install_prompt.h"
Looks like this should actually be extension_install_ui.h

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/ui/w...
File chrome/browser/ui/webui/extensions/extension_settings_handler.h (right):

https://chromiumcodereview.appspot.com/10388252/diff/2001/chrome/browser/ui/w...
chrome/browser/ui/webui/extensions/extension_settings_handler.h:14: #include
"chrome/browser/extensions/extension_install_prompt.h"
This looks unneeded.

[Aaron Boodman, 2012-05-30 00:27:24]

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.cc:336: SetIcon(icon);
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> It seems surprising to me that these are necessary.
> http://codereview.chromium.org/5690004 is where they were added. Maybe aa can
> comment on whether they are still useful.

No idea... this code has changed so much. It's easy enough to test though.
According to the old CL, the bug was that the icon in the install bubble did not
show up after install. So with this patch, when you install an extension, does
the install bubble show up correctly? (It's the thingy in the top right corner
pointing to either the icon in the toolbar or the wrench. It's supposed to have
the extension icon in it).

[Jay Civelli, 2012-05-30 20:20:34]

Sorry for the delay, got caught up with something else.
Addressed all your comments.

Thanks!

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:3904: const Extension*
theme =
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> nit: This change looks spurious.

Done.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/download/dow...
File chrome/browser/download/download_crx_util.h (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/download/dow...
chrome/browser/download/download_crx_util.h:25: // Allow tests to install a mock
extension install Prompt object, to fake
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> nit: just ExtensionInstallPrompt

Done.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.cc:86:
ExtensionInstallPrompt::Prompt::Prompt(PromptType type)
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> Names like this make me think the structure of the code is not ideal. But such
> refactoring is probably beyond the scope of this CL.

Agreed.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.cc:336: SetIcon(icon);
On 2012/05/30 00:27:24, Aaron Boodman wrote:
> On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> > It seems surprising to me that these are necessary.
> > http://codereview.chromium.org/5690004 is where they were added. Maybe aa
can
> > comment on whether they are still useful.
> 
> No idea... this code has changed so much. It's easy enough to test though.
> According to the old CL, the bug was that the icon in the install bubble did
not
> show up after install. So with this patch, when you install an extension, does
> the install bubble show up correctly? (It's the thingy in the top right corner
> pointing to either the icon in the toolbar or the wrench. It's supposed to
have
> the extension icon in it).

I just checked. I commented out the 2 lines above, installed  the GMail checker
app and the bubble did include the right icon. So I am assuming this is not
needed anymore.
Without this, I have been able to move this code directly to ExtensionInstallUI.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.h:214: virtual void
OnInstallSuccess(const extensions::Extension* extension,
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> I was originally thinking that OnInstallSuccess and OnInstallFailure could be
> moved entirely to the ExtensionInstallUI. Does that look possible?

Good idea! Done.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_ui.h (left):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_ui.h:36: // Displays all the UI
around extension installation.
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> This comment (or something like it) should still be here, even if just to
> describe this as an interface.

Done.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_ui.h:212: // Installation was
successful. This is declared virtual for testing.
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> These comments may not make sense anymore, but there should be some comment
for
> these functions.

Done.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_ui.h:41: Profile* profile_;
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> Consider moving this to install_ui_default. As long as the base class doesn't
do
> anything with profile_, there is no need to keep it here.

Done.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_management_api.h (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_management_api.h:112:
scoped_ptr<ExtensionInstallPrompt> install_ui_;
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> nit: rename this variable

Done.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_navigation_observer.h (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_navigation_observer.h:55:
scoped_ptr<ExtensionInstallPrompt> extension_install_ui_;
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> nit: rename

Done.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_api.h (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.h:151:
scoped_ptr<ExtensionInstallPrompt> install_ui_;
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> nit: rename

Done.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/w...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_inline_installer.cc:264: average_rating_
>ExtensionInstallPrompt::kMaxExtensionRating) {
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> nit: missing space

Done.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble.cc (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble.cc:16: #include
"chrome/browser/extensions/extension_install_prompt.h"
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> Looks like this should actually be extension_install_ui.h

Done.

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/ui/webui/ext...
File chrome/browser/ui/webui/extensions/extension_settings_handler.h (right):

http://codereview.chromium.org/10388252/diff/2001/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_handler.h:14: #include
"chrome/browser/extensions/extension_install_prompt.h"
On 2012/05/23 23:17:49, Yoyo Zhou wrote:
> This looks unneeded.

Done.

[Yoyo Zhou, 2012-06-01 21:07:43]

Mostly LGTM. There are a few things that I'm not sure are going to work on the
android build.

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.h:165: void
SetSkipPostInstallUI(bool skip_ui);
Can this be removed too? It's UI-specific, and the callers should be able to
call the one on install_ui.

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.h:26: static ExtensionInstallUI*
Create(Profile* profile);
Is it okay that this isn't implemented in Android?

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.h:37: virtual void
SetSkipPostInstallUI(bool skip_ui) = 0;
This one also isn't implemented on Android.

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/webstore_inline_install_browsertest.cc (right):

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/webstore_inline_install_browsertest.cc:10: #include
"chrome/browser/extensions/extension_install_prompt.h"
Probably not needed anymore.

http://codereview.chromium.org/10388252/diff/15001/chrome/chrome_browser_exte...
File chrome/chrome_browser_extensions.gypi (right):

http://codereview.chromium.org/10388252/diff/15001/chrome/chrome_browser_exte...
chrome/chrome_browser_extensions.gypi:669:
'browser/extensions/extension_install_ui_android.h',
This file doesn't exist.

[Jay Civelli, 2012-06-04 17:15:30]

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_prompt.h:165: void
SetSkipPostInstallUI(bool skip_ui);
On 2012/06/01 21:07:43, Yoyo Zhou wrote:
> Can this be removed too? It's UI-specific, and the callers should be able to
> call the one on install_ui.

Done.

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.h:26: static ExtensionInstallUI*
Create(Profile* profile);
On 2012/06/01 21:07:43, Yoyo Zhou wrote:
> Is it okay that this isn't implemented in Android?
I had not built on Android in a while.
Done, added missing methods.

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.h:37: virtual void
SetSkipPostInstallUI(bool skip_ui) = 0;
On 2012/06/01 21:07:43, Yoyo Zhou wrote:
> This one also isn't implemented on Android.

Done.

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/webstore_inline_install_browsertest.cc (right):

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/webstore_inline_install_browsertest.cc:10: #include
"chrome/browser/extensions/extension_install_prompt.h"
On 2012/06/01 21:07:43, Yoyo Zhou wrote:
> Probably not needed anymore.

Done.

http://codereview.chromium.org/10388252/diff/15001/chrome/chrome_browser_exte...
File chrome/chrome_browser_extensions.gypi (right):

http://codereview.chromium.org/10388252/diff/15001/chrome/chrome_browser_exte...
chrome/chrome_browser_extensions.gypi:669:
'browser/extensions/extension_install_ui_android.h',
On 2012/06/01 21:07:43, Yoyo Zhou wrote:
> This file doesn't exist.

Created.

[Yoyo Zhou, 2012-06-04 21:02:01]

LGTM. The tryjob results look inflamed, so please make sure that tests still
pass.

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_install_ui.h (right):

http://codereview.chromium.org/10388252/diff/15001/chrome/browser/extensions/...
chrome/browser/extensions/extension_install_ui.h:26: static ExtensionInstallUI*
Create(Profile* profile);
On 2012/06/04 17:15:30, Jay Civelli wrote:
> On 2012/06/01 21:07:43, Yoyo Zhou wrote:
> > Is it okay that this isn't implemented in Android?
> I had not built on Android in a while.
> Done, added missing methods.

This one still looks unimplemented on Android, for what it's worth.

[Jay Civelli, 2012-06-08 01:25:58]

Scott, could you look at the chrome/browser/ui/ files?
Randy, could you look at the download related files?

Thanks.

[sky, 2012-06-08 02:52:02]

Fix this and chrome/browser/ui LGTM

http://codereview.chromium.org/10388252/diff/51048/chrome/browser/ui/cocoa/ex...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

http://codereview.chromium.org/10388252/diff/51048/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:324:
Browser* browser = BrowserList::FindLastActiveWithProfile(profile);
The old code (browser::Find...) is right. FindLast... was moved to
browser_finder, which is a collection of functions in the browser namespace.

[Randy Smith (Not in Mondays), 2012-06-08 16:38:58]

chrome/browser/download LGTM.

[commit-bot: I haz the power, 2012-06-08 17:38:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/10388252/58004

[commit-bot: I haz the power, 2012-06-08 19:49:37]

Change committed as 141261