Make "source" a required argument of AppLaunchParams and fill in the argument in all call points.

Make "source" a required argument of AppLaunchParams and fill in the argument at all call points.

BUG=400619
Committed: https://crrev.com/96cd75fce7e9523041c86a15b9916c8d57c99ac3
Cr-Commit-Position: refs/heads/master@{#305038}

[cylee1, 2014-11-12 17:22:44]

cylee@chromium.org changed reviewers:
+ benwells@chromium.org, oshima@chromium.org

[cylee1, 2014-11-12 17:22:45]

Hi Oshima,
  A small change in athena/extensions/chrome/extensions_delegate_impl.cc needs
OWNER review.

[oshima, 2014-11-12 18:06:26]

athena/ lgtm with anit

https://codereview.chromium.org/722703002/diff/1/athena/extensions/chrome/ext...
File athena/extensions/chrome/extensions_delegate_impl.cc (right):

https://codereview.chromium.org/722703002/diff/1/athena/extensions/chrome/ext...
athena/extensions/chrome/extensions_delegate_impl.cc:64: SOURCE_APP_LAUNCHER);
nit: one parameter per line.

[cylee1, 2014-11-12 20:02:06]

cylee@chromium.org changed reviewers:
+ asvitkine@chromium.org, sky@chromium.org

[cylee1, 2014-11-12 20:02:07]

https://codereview.chromium.org/722703002/diff/1/athena/extensions/chrome/ext...
File athena/extensions/chrome/extensions_delegate_impl.cc (right):

https://codereview.chromium.org/722703002/diff/1/athena/extensions/chrome/ext...
athena/extensions/chrome/extensions_delegate_impl.cc:64: SOURCE_APP_LAUNCHER);
On 2014/11/12 18:06:26, oshima wrote:
> nit: one parameter per line.

Tried, but after running "git cl format" and it re-format like so.

[oshima, 2014-11-12 20:37:59]

On 2014/11/12 20:02:07, cylee1 wrote:
>
https://codereview.chromium.org/722703002/diff/1/athena/extensions/chrome/ext...
> File athena/extensions/chrome/extensions_delegate_impl.cc (right):
> 
>
https://codereview.chromium.org/722703002/diff/1/athena/extensions/chrome/ext...
> athena/extensions/chrome/extensions_delegate_impl.cc:64: SOURCE_APP_LAUNCHER);
> On 2014/11/12 18:06:26, oshima wrote:
> > nit: one parameter per line.
> 
> Tried, but after running "git cl format" and it re-format like so.

ok if "git cl format" does it, fine.

[Alexei Svitkine (slow), 2014-11-12 20:41:34]

histograms lgtm

[cylee1, 2014-11-17 10:42:56]

Ping benwells,
  Got time to take a look? Thanks

[benwells, 2014-11-18 21:43:16]

lgtm

https://codereview.chromium.org/722703002/diff/20001/extensions/common/consta...
File extensions/common/constants.h (right):

https://codereview.chromium.org/722703002/diff/20001/extensions/common/consta...
extensions/common/constants.h:115: SOURCE_UNTRACKED = 0,
Please add a comment that UNTRACKED should be used in tests.

[cylee1, 2014-11-19 13:47:47]

Hi Scott,
Some files needs OWNER approval:
    chrome/browser/*
    chrome/test/*
PTAL. Thanks.

https://codereview.chromium.org/722703002/diff/20001/extensions/common/consta...
File extensions/common/constants.h (right):

https://codereview.chromium.org/722703002/diff/20001/extensions/common/consta...
extensions/common/constants.h:115: SOURCE_UNTRACKED = 0,
On 2014/11/18 21:43:16, benwells wrote:
> Please add a comment that UNTRACKED should be used in tests.

Done.

[cylee1, 2014-11-19 16:45:17]

On 2014/11/19 13:47:47, cylee1 wrote:
> Hi Scott,
> Some files needs OWNER approval:
>     chrome/browser/*
>     chrome/test/*
> PTAL. Thanks.
> 
>
https://codereview.chromium.org/722703002/diff/20001/extensions/common/consta...
> File extensions/common/constants.h (right):
> 
>
https://codereview.chromium.org/722703002/diff/20001/extensions/common/consta...
> extensions/common/constants.h:115: SOURCE_UNTRACKED = 0,
> On 2014/11/18 21:43:16, benwells wrote:
> > Please add a comment that UNTRACKED should be used in tests.
> 
> Done.

Hi Scott,
  Ben has permission for files related to extensions/apps, but it looks the
following files still needs OWNER approval:

chrome/browser/background/background_mode_manager.cc
chrome/browser/chromeos/app_mode/startup_app_launcher.cc
chrome/browser/chromeos/extensions/wallpaper_manager_util.cc
chrome/browser/chromeos/first_run/first_run.cc
chrome/browser/chromeos/login/demo_mode/demo_app_launcher.cc
chrome/browser/chromeos/policy/device_local_account_browsertest.cc
chrome/browser/policy/policy_browsertest.cc
chrome/browser/search/hotword_service.cc
chrome/browser/signin/easy_unlock_service_regular.cc
chrome/browser/ui/app_list/app_list_controller_delegate_impl.cc
chrome/browser/ui/ash/chrome_new_window_delegate_chromeos.cc
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
chrome/browser/ui/browser_browsertest.cc
chrome/browser/ui/chrome_pages.cc
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
chrome/browser/ui/extensions/app_launch_params.cc
chrome/browser/ui/extensions/app_launch_params.h
chrome/browser/ui/extensions/application_launch.cc
chrome/browser/ui/startup/startup_browser_creator_impl.cc
chrome/browser/ui/views/frame/browser_window_property_manager_browsertest_win.cc
chrome/browser/ui/webui/chromeos/login/error_screen_handler.cc
chrome/browser/ui/webui/extensions/extension_settings_handler.cc
chrome/browser/ui/webui/ntp/app_launcher_handler.cc
chrome/test/ppapi/ppapi_browsertest.cc
chrome/test/remoting/remote_desktop_browsertest.cc

All changes basically just add a new parameter "source" to the constructor of
"AppLaunchParams"

[sky, 2014-11-19 16:52:08]

LGTM

https://codereview.chromium.org/722703002/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_new_window_delegate_chromeos.cc (right):

https://codereview.chromium.org/722703002/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_new_window_delegate_chromeos.cc:42:
OpenApplication(AppLaunchParams(profile, extension, 0 /* event_flags */,
Seems like there should be a constant for 0.

https://codereview.chromium.org/722703002/diff/40001/chrome/browser/ui/chrome...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/722703002/diff/40001/chrome/browser/ui/chrome...
chrome/browser/ui/chrome_pages.cc:80: HelpSource help_source) {
Change the declaration to match the new name, although I would be inclined to
keep existing name and change what you have as source on line 87 to
app_launch_source.

[cylee1, 2014-11-19 17:51:17]

https://codereview.chromium.org/722703002/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_new_window_delegate_chromeos.cc (right):

https://codereview.chromium.org/722703002/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_new_window_delegate_chromeos.cc:42:
OpenApplication(AppLaunchParams(profile, extension, 0 /* event_flags */,
On 2014/11/19 16:52:08, sky wrote:
> Seems like there should be a constant for 0.

After tracking down the code I feel it's better to pass a WindowOpenDisposition
object directly  (see changes in app_launch_params.h/cc)

https://codereview.chromium.org/722703002/diff/40001/chrome/browser/ui/chrome...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/722703002/diff/40001/chrome/browser/ui/chrome...
chrome/browser/ui/chrome_pages.cc:80: HelpSource help_source) {
On 2014/11/19 16:52:08, sky wrote:
> Change the declaration to match the new name, although I would be inclined to
> keep existing name and change what you have as source on line 87 to
> app_launch_source.

Done.

[cylee1, 2014-11-20 10:25:55]

The CQ bit was checked by cylee@chromium.org

[commit-bot: I haz the power, 2014-11-20 10:27:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/722703002/60001

[commit-bot: I haz the power, 2014-11-20 10:31:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-20 10:31:01]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[cylee1, 2014-11-20 10:54:22]

The CQ bit was checked by cylee@chromium.org

[commit-bot: I haz the power, 2014-11-20 10:54:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/722703002/80001

[commit-bot: I haz the power, 2014-11-20 11:55:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-20 11:55:49]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cylee1, 2014-11-20 15:16:52]

https://codereview.chromium.org/722703002/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_new_window_delegate_chromeos.cc (right):

https://codereview.chromium.org/722703002/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_new_window_delegate_chromeos.cc:42:
OpenApplication(AppLaunchParams(profile, extension, 0 /* event_flags */,
On 2014/11/19 17:51:17, cylee1 wrote:
> On 2014/11/19 16:52:08, sky wrote:
> > Seems like there should be a constant for 0.
> 
> After tracking down the code I feel it's better to pass a
WindowOpenDisposition
> object directly  (see changes in app_launch_params.h/cc)

Introduced some bug in patch set 5, fixed in patch set 6 in app_launch_params.cc

[sky, 2014-11-20 15:21:14]

Better, thanks: LGTM

[cylee1, 2014-11-20 16:22:34]

The CQ bit was checked by cylee@chromium.org

[commit-bot: I haz the power, 2014-11-20 16:22:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/722703002/100001

[commit-bot: I haz the power, 2014-11-20 18:26:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-20 18:27:12]

Patchset 6 (id:??) landed as
https://crrev.com/96cd75fce7e9523041c86a15b9916c8d57c99ac3
Cr-Commit-Position: refs/heads/master@{#305038}

[Mr4D (OOO till 08-26), 2014-11-21 18:50:34]

Reverted due to build breakage. See CL 720633003.