Restructure ExtensionAppShimHandler testing.

Restructure ExtensionAppShimHandler testing.

This puts all dependencies into a inner Delegate class so the test can
mock them out.

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

[jackhou1, 2013-06-21 02:06:14]

I think better to get this cleaned up and committed before I make more changes
to ExtensionAppShimHandler.

[tapted, 2013-06-21 04:30:47]

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:56:
ExtensionAppShimHandler::Delegate::GetExtension(
since the is_platform_app check is moving in here, this might need a new name.
GetAppExtension maybe?

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:60:
extensions::ExtensionSystem::Get(profile)->extension_service()->
Should DCHECK here that that extension_service is non-null. Will need another
temporary, but that will help split up the line anyway.

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:123: if (launch_type ==
APP_SHIM_LAUNCH_REGISTER_ONLY &&
needs curlies if the condition is multi-line

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:124:
delegate_->GetWindows(profile, app_id).empty())
Should this be !empty()?

Maybe a comment, like ~"if the shim was launched in response to a window
appearing, but it closed by the time the shim process launched, shut it down."

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:132:
delegate_->LaunchApp(profile, extension);
Should it return false if LaunchApp returns NULL here?

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:145:
DCHECK(delegate_->ProfileExistsForPath(
nit: can this fit on the line above now?

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:148:
delegate_->ProfileForPath(host->GetProfilePath());
nit: one line?

up to 6 more below too

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
File apps/app_shim/extension_app_shim_handler_mac.h (right):

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.h:83: void
set_dependency_facade(Delegate* dependency_facade);
dependency_facade -> delegate

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
File apps/app_shim/extension_app_shim_handler_mac_unittest.cc (right):

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:25: : public
ExtensionAppShimHandler::Delegate {
nit: one line?

[jackhou1, 2013-06-21 05:46:27]

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:56:
ExtensionAppShimHandler::Delegate::GetExtension(
On 2013/06/21 04:30:47, tapted wrote:
> since the is_platform_app check is moving in here, this might need a new name.
> GetAppExtension maybe?

Done.

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:60:
extensions::ExtensionSystem::Get(profile)->extension_service()->
On 2013/06/21 04:30:47, tapted wrote:
> Should DCHECK here that that extension_service is non-null. Will need another
> temporary, but that will help split up the line anyway.

Done.

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:123: if (launch_type ==
APP_SHIM_LAUNCH_REGISTER_ONLY &&
On 2013/06/21 04:30:47, tapted wrote:
> needs curlies if the condition is multi-line

Done.

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:124:
delegate_->GetWindows(profile, app_id).empty())
On 2013/06/21 04:30:47, tapted wrote:
> Should this be !empty()?
> 
> Maybe a comment, like ~"if the shim was launched in response to a window
> appearing, but it closed by the time the shim process launched, shut it down."

It should be empty(), because it returns false if there are no windows. Added
comment.

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:132:
delegate_->LaunchApp(profile, extension);
On 2013/06/21 04:30:47, tapted wrote:
> Should it return false if LaunchApp returns NULL here?

Turns out LaunchApplication always returns NULL for platform apps.

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:145:
DCHECK(delegate_->ProfileExistsForPath(
On 2013/06/21 04:30:47, tapted wrote:
> nit: can this fit on the line above now?

Done.

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:148:
delegate_->ProfileForPath(host->GetProfilePath());
On 2013/06/21 04:30:47, tapted wrote:
> nit: one line?
> 
> up to 6 more below too

Done.

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
File apps/app_shim/extension_app_shim_handler_mac.h (right):

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.h:83: void
set_dependency_facade(Delegate* dependency_facade);
On 2013/06/21 04:30:47, tapted wrote:
> dependency_facade -> delegate

Done.

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
File apps/app_shim/extension_app_shim_handler_mac_unittest.cc (right):

https://codereview.chromium.org/17481002/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac_unittest.cc:25: : public
ExtensionAppShimHandler::Delegate {
On 2013/06/21 04:30:47, tapted wrote:
> nit: one line?

Done.

[tapted, 2013-06-21 06:07:02]

should add the app shim bug# too

https://codereview.chromium.org/17481002/diff/5001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/17481002/diff/5001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:46: base::FilePath full_path =
profile_manager->user_data_dir().Append(path);
some screwy stuff here from a rebase

https://codereview.chromium.org/17481002/diff/5001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:72:
chrome::OpenApplication(chrome::AppLaunchParams(
nit: might be marginally nicer to linebreak on the first paren

https://codereview.chromium.org/17481002/diff/5001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:139: if (launch_type ==
APP_SHIM_LAUNCH_NORMAL) {
nit: doesn't need curlies

[jackhou1, 2013-06-21 06:53:57]

https://codereview.chromium.org/17481002/diff/5001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/17481002/diff/5001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:46: base::FilePath full_path =
profile_manager->user_data_dir().Append(path);
On 2013/06/21 06:07:02, tapted wrote:
> some screwy stuff here from a rebase

Oh yeah, I rebased to pick up http://crrev.com/17524003/. This is intentional.

https://codereview.chromium.org/17481002/diff/5001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:72:
chrome::OpenApplication(chrome::AppLaunchParams(
On 2013/06/21 06:07:02, tapted wrote:
> nit: might be marginally nicer to linebreak on the first paren

Done.

https://codereview.chromium.org/17481002/diff/5001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:139: if (launch_type ==
APP_SHIM_LAUNCH_NORMAL) {
On 2013/06/21 06:07:02, tapted wrote:
> nit: doesn't need curlies

Done.

[tapted, 2013-06-21 08:02:32]

lgtm

https://codereview.chromium.org/17481002/diff/5001/apps/app_shim/extension_ap...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/17481002/diff/5001/apps/app_shim/extension_ap...
apps/app_shim/extension_app_shim_handler_mac.cc:46: base::FilePath full_path =
profile_manager->user_data_dir().Append(path);
On 2013/06/21 06:53:57, jackhou1 wrote:
> On 2013/06/21 06:07:02, tapted wrote:
> > some screwy stuff here from a rebase
> 
> Oh yeah, I rebased to pick up http://crrev.com/17524003/. This is intentional.

Ahh - I see, the rebase was not in a separate CL (they should be ;).

[commit-bot: I haz the power, 2013-06-21 08:19:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/17481002/13001

[commit-bot: I haz the power, 2013-06-21 16:50:21]

Change committed as 207862