Remove --disable-app-shims.

Remove --disable-app-shims.

App shims have been enabled by default for 3 milestones
(since r242711).

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

[tapted, 2014-05-22 04:11:11]

description nit: I think it would be good to provide some context. E.g. ~app
shims have been enabled by default for 3 milestones (since r242711).

https://codereview.chromium.org/298953002/diff/1/apps/app_shim/app_shim_mac.cc
File apps/app_shim/app_shim_mac.cc (right):

https://codereview.chromium.org/298953002/diff/1/apps/app_shim/app_shim_mac.c...
apps/app_shim/app_shim_mac.cc:14: // be cleaned up.
Is this the only reason? That is, can we move this function to an anonymous
namespace in web_app_mac.mm and achieve the same?

[jackhou1, 2014-05-22 04:59:58]

https://codereview.chromium.org/298953002/diff/1/apps/app_shim/app_shim_mac.cc
File apps/app_shim/app_shim_mac.cc (right):

https://codereview.chromium.org/298953002/diff/1/apps/app_shim/app_shim_mac.c...
apps/app_shim/app_shim_mac.cc:14: // be cleaned up.
On 2014/05/22 04:11:11, tapted wrote:
> Is this the only reason? That is, can we move this function to an anonymous
> namespace in web_app_mac.mm and achieve the same?

Hmm, when I was considering this, I thought it was better as-is because the
check happens earlier on the UI thread. The entry points in web_app_mac are all
on the FILE thread. On looking at it again, that seems a minor issue. Moving it
to web_app_mac would also simplify AppShortcutsManager::OnceOffCreateShortcuts.

[tapted, 2014-05-22 05:05:15]

Sweet! lgtm

[jackhou1, 2014-05-22 05:18:48]

thakis, please review for OWNERS:

chrome/browser/app_controller_mac.mm

dbeam, please review for OWNERS:

chrome/browser/ui/webui/ntp/ntp_resource_cache.cc

[Nico, 2014-05-22 18:09:30]

lgtm

[Dan Beam, 2014-05-23 03:17:33]

if app shortcuts exist on Mac, when is the NTP/apps page code going to be
updated to reflect this? [1]

[1]
https://code.google.com/p/chromium/codesearch#search/&q=disableCreateAppShortcut

[jackhou1, 2014-05-23 05:37:42]

On 2014/05/23 03:17:33, Dan Beam wrote:
> if app shortcuts exist on Mac, when is the NTP/apps page code going to be
> updated to reflect this? [1]
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#search/&q=disableCreateAppShortcut

Ah good point, thanks. I've removed the check for disableCreateAppShortcut in
apps_page.js and the occurrence in app_launcher_handler.cc. Not sure about the
one under #if defined(OS_CHROMEOS), it might not do anything.

[Dan Beam, 2014-05-23 19:20:51]

On 2014/05/23 05:37:42, jackhou1 wrote:
> I've removed the check for disableCreateAppShortcut in
> apps_page.js and the occurrence in app_launcher_handler.cc. Not sure about the
> one under #if defined(OS_CHROMEOS), it might not do anything.

^ it doesn't do anything, nor does the code that sets disableAppWindowLaunch
(it's never accessed anywhere in the code as far as I can tell)

lgtm either way but less code is best code

https://chromiumcodereview.appspot.com/298953002/diff/40001/chrome/browser/ui...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (left):

https://chromiumcodereview.appspot.com/298953002/diff/40001/chrome/browser/ui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:383: 
can you just remove from here

https://chromiumcodereview.appspot.com/298953002/diff/40001/chrome/browser/ui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:396: #endif
to here?

[jackhou1, 2014-05-25 03:44:52]

https://codereview.chromium.org/298953002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (left):

https://codereview.chromium.org/298953002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:383: 
On 2014/05/23 19:20:51, Dan Beam wrote:
> can you just remove from here

Done.

[jackhou1, 2014-05-25 03:44:57]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-05-25 03:45:03]

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

[commit-bot: I haz the power, 2014-05-25 04:07:31]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/8941)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12022)

[commit-bot: I haz the power, 2014-05-25 04:11:10]

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

[commit-bot: I haz the power, 2014-05-25 04:11:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[jackhou1, 2014-05-25 04:51:04]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-05-25 04:51:15]

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

[commit-bot: I haz the power, 2014-05-25 08:27:08]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-25 08:30:45]

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

[commit-bot: I haz the power, 2014-05-25 08:30:45]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[jackhou1, 2014-05-26 00:27:54]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-05-26 00:28:24]

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

[commit-bot: I haz the power, 2014-05-26 04:21:46]

Change committed as 272786