Remove HostDesktopType from AppLaunchParams

Remove HostDesktopType from AppLaunchParams

This one is a bit subtle. The two removed "helper" AppLaunchParams
constructors were using HostDesktopType as an unrelated boolean to
choose one or the other of the two constructors (unrelated to the actual
value of HostDesktopType).

I pulled those two out and had them use the now-single constructor, and
tried to give the two names that made some sense according to their
comments.

R=benwells@chromium.org, sky@chromium.org
BUG=558054, 587387, 586862
Committed: https://crrev.com/74deaca32d2f31b93fb799062a62d77eba28e516
Cr-Commit-Position: refs/heads/master@{#381770}

[scottmg, 2016-03-16 01:36:21]

Description was changed from

==========
Remove HostDesktopType from AppLaunchParams

This one is a bit subtle. The two "helper" AppLaunchParams constructors
were using HostDesktopType as a sort of unrelated boolean to choose a
different helper (but not actually using the value of HostDesktopType).
I pulled those two ctors out into factory functions instead.

R=benwells@chromium.org
BUG=558054
==========

to

==========
Remove HostDesktopType from AppLaunchParams

This one is a bit subtle. The two "helper" AppLaunchParams constructors
were using HostDesktopType as a sort of unrelated boolean to choose a
different helper (but not actually using the value of HostDesktopType).
I pulled those two ctors out into factory functions instead.

R=benwells@chromium.org, sky@chromium.org
BUG=558054
==========

[scottmg, 2016-03-16 01:36:22]

scottmg@chromium.org changed reviewers:
+ sky@chromium.org

[scottmg, 2016-03-16 01:38:57]

Description was changed from

==========
Remove HostDesktopType from AppLaunchParams

This one is a bit subtle. The two "helper" AppLaunchParams constructors
were using HostDesktopType as a sort of unrelated boolean to choose a
different helper (but not actually using the value of HostDesktopType).
I pulled those two ctors out into factory functions instead.

R=benwells@chromium.org, sky@chromium.org
BUG=558054
==========

to

==========
Remove HostDesktopType from AppLaunchParams

This one is a bit subtle. The two removed "helper" AppLaunchParams
constructors were using HostDesktopType as an unrelated boolean to
choose one or the other of the two constructors (unrelated to the actual
value of HostDesktopType).

I pulled those two out and had them use the now-single constructor, and
tried to give the two names that made some sense according to their
comments.

R=benwells@chromium.org, sky@chromium.org
BUG=558054, 587387, 586862
==========

[benwells, 2016-03-16 06:29:43]

https://codereview.chromium.org/1807743003/diff/60001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/app_launch_params.cc (right):

https://codereview.chromium.org/1807743003/diff/60001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/app_launch_params.cc:40: AppLaunchParams
CreateAppLaunchParamsOverriddenContainer(
All but one of the callers of this call it with raw_disposition == CURRENT_TAB,
which seems equivalent to the above.

The one exception uses ui::DispositionFromEventFlags(event_flags). Instead of
having two constructors with the same parameter types, could this one take the
event_flags, look up the disposition from that, and then override the container
appropriately? That is, CreateAppLaunchParamsWithEventFlags or something.

[sky, 2016-03-16 15:48:13]

LGTM

[scottmg, 2016-03-16 16:38:54]

https://codereview.chromium.org/1807743003/diff/60001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/app_launch_params.cc (right):

https://codereview.chromium.org/1807743003/diff/60001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/app_launch_params.cc:40: AppLaunchParams
CreateAppLaunchParamsOverriddenContainer(
On 2016/03/16 06:29:43, benwells wrote:
> All but one of the callers of this call it with raw_disposition ==
CURRENT_TAB,
> which seems equivalent to the above.

OK, for these 3, I switched to CreateAppLaunchParamsUseContainer() and switched
CURRENT_TAB to NEW_FOREGROUND_TAB, which is what the other function would have
done (I think).

> 
> The one exception uses ui::DispositionFromEventFlags(event_flags). Instead of
> having two constructors with the same parameter types, could this one take the
> event_flags, look up the disposition from that, and then override the
container
> appropriately? That is, CreateAppLaunchParamsWithEventFlags or something.

Renamed the second factory as suggested.

[benwells, 2016-03-16 22:50:13]

lgtm once the bots are green

https://codereview.chromium.org/1807743003/diff/80001/chrome/browser/apps/app...
File chrome/browser/apps/app_browsertest_util.cc (right):

https://codereview.chromium.org/1807743003/diff/80001/chrome/browser/apps/app...
chrome/browser/apps/app_browsertest_util.cc:125: browser()->profile(),
extension, LAUNCH_CONTAINER_NONE, NEW_WINDOW,
Does this compile? I think it should stay as it wsa, as the container is passed
in.

[scottmg, 2016-03-17 00:22:55]

Patchset #8 (id:140001) has been deleted

[scottmg, 2016-03-17 00:24:12]

I also removed the now-always-empty FillLaunchParams().

https://codereview.chromium.org/1807743003/diff/80001/chrome/browser/apps/app...
File chrome/browser/apps/app_browsertest_util.cc (right):

https://codereview.chromium.org/1807743003/diff/80001/chrome/browser/apps/app...
chrome/browser/apps/app_browsertest_util.cc:125: browser()->profile(),
extension, LAUNCH_CONTAINER_NONE, NEW_WINDOW,
On 2016/03/16 22:50:12, benwells wrote:
> Does this compile? I think it should stay as it wsa, as the container is
passed
> in.

Thanks. I used cs.chromium.org to (attempt to) xref to the correct overrides of
the AppLaunchParams ctors, but when I popped over to my editor I changed the
wrong one (LaunchPlatformApp() instead of LaunchHostedApp()). Reverted here and
fixed below.

[scottmg, 2016-03-17 19:16:35]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-03-17 19:16:35]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/1807743003/#ps200001
(title: "cros")

[commit-bot: I haz the power, 2016-03-17 19:17:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1807743003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1807743003/200001

[commit-bot: I haz the power, 2016-03-17 19:22:53]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-03-17 19:23:50]

Description was changed from

==========
Remove HostDesktopType from AppLaunchParams

This one is a bit subtle. The two removed "helper" AppLaunchParams
constructors were using HostDesktopType as an unrelated boolean to
choose one or the other of the two constructors (unrelated to the actual
value of HostDesktopType).

I pulled those two out and had them use the now-single constructor, and
tried to give the two names that made some sense according to their
comments.

R=benwells@chromium.org, sky@chromium.org
BUG=558054, 587387, 586862
==========

to

==========
Remove HostDesktopType from AppLaunchParams

This one is a bit subtle. The two removed "helper" AppLaunchParams
constructors were using HostDesktopType as an unrelated boolean to
choose one or the other of the two constructors (unrelated to the actual
value of HostDesktopType).

I pulled those two out and had them use the now-single constructor, and
tried to give the two names that made some sense according to their
comments.

R=benwells@chromium.org, sky@chromium.org
BUG=558054, 587387, 586862

Committed: https://crrev.com/74deaca32d2f31b93fb799062a62d77eba28e516
Cr-Commit-Position: refs/heads/master@{#381770}
==========

[commit-bot: I haz the power, 2016-03-17 19:23:52]

Patchset 10 (id:??) landed as
https://crrev.com/74deaca32d2f31b93fb799062a62d77eba28e516
Cr-Commit-Position: refs/heads/master@{#381770}