Add enable-app-shims to chrome://flags.

Add enable-app-shims to chrome://flags.

Adds a flag --enable-app-shims. This is passed through to the new tab page
and enables the "Create shortcut" context menu option on Mac. This flag will
be used in future to gate creation of app shim bundles and starting them
when the app launches.

BUG=168080
TEST=Go to the new tab page.
Right click a packaged app. There should be no option to create shortcut.
Go to chrome:flags and enable "Enable packaged app shortcuts".
Now the create shortcut option should be available.

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

[tapted, 2013-05-29 08:07:36]

looks good. but I'm nervous about whether the way the flag is described here
will match how the flag will be used when that part lands. I think it would also
be bad to add a chrome://flag that does nothing when it first lands -- it might
leak into a channel and get complaints when it doesn't work ;)

I think it's better to roll this CL into the change that checks for the flag as
well, and does some action. (and maybe that CL should just have the
chrome_switches part of this CL - not sure.)

https://codereview.chromium.org/16191003/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16191003/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:7211: +          Enable platform app
shortcuts.
nit: probably "packaged app" instead of platform apps, since there's precedent
for that in chrome://flags.

https://codereview.chromium.org/16191003/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/16191003/diff/1/chrome/common/chrome_switches...
chrome/common/chrome_switches.cc:1529: const char kEnableAppShims[] =
"enable-app-shims";
nit: align `=`

[jackhou1, 2013-05-30 04:43:17]

PTAL

Oops, patch description should say NTP.

https://codereview.chromium.org/16191003/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16191003/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:7211: +          Enable platform app
shortcuts.
On 2013/05/29 08:07:36, tapted wrote:
> nit: probably "packaged app" instead of platform apps, since there's precedent
> for that in chrome://flags.

Done.

https://codereview.chromium.org/16191003/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/16191003/diff/1/chrome/common/chrome_switches...
chrome/common/chrome_switches.cc:1529: const char kEnableAppShims[] =
"enable-app-shims";
On 2013/05/29 08:07:36, tapted wrote:
> nit: align `=`

Done.

[tapted, 2013-05-30 05:32:24]

+chrome-apps-syd-reviews. But, since the initial files have changed from the
initial upload, you're missing a lot of watchers -- maybe re-upload to a dummy
CL to see who should be on the CC list, and add them via `Edit Issue`.

And, I'm not really a webui expert, but Chrome seems to shy away from "static"
variables, see if you can find a precedent for a setting like this somewhere.

One thing that comes to mind is the `should_show_apps_page` (and
NewTabUI::ShouldShowApps()). This has some settings in
browser/ui/webui/ntp/ntp_resource_cache.h -- should you be doing something
similar?

https://codereview.chromium.org/16191003/diff/3001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://codereview.chromium.org/16191003/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/apps_page.js:46: disableCreateAppShortcut: false,
pretty sure this should be in initialize(), with a this. and and underscore at
the end. i.e. (in initialize)

this.disableCreateAppShortcut_ = false;

Then, you'll need a function to set it, since it's "private"

https://codereview.chromium.org/16191003/diff/3001/chrome/browser/resources/n...
File chrome/browser/resources/ntp4/page_list_view.js (right):

https://codereview.chromium.org/16191003/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/page_list_view.js:417: if
(data.disableCreateAppShortcut === true) {
maybe this check should be before `sectionready` is dispatched?

https://codereview.chromium.org/16191003/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp4/page_list_view.js:418:
this.appsPages[0].disableCreateAppShortcut();
this reads strangely -- it's only setting page[0], which is "wrong" (except that
it's currently a `static` variable, or it's on a singleton. Maybe call
AppContextMenu.getInstance() here (perhaps indirectly), and use that setter
function you'll add.

[jackhou1, 2013-05-30 06:06:38]

On 2013/05/30 05:32:24, tapted wrote:
> +chrome-apps-syd-reviews. But, since the initial files have changed from the
> initial upload, you're missing a lot of watchers -- maybe re-upload to a dummy
> CL to see who should be on the CC list, and add them via `Edit Issue`.
> 
> And, I'm not really a webui expert, but Chrome seems to shy away from "static"
> variables, see if you can find a precedent for a setting like this somewhere.
> 
> One thing that comes to mind is the `should_show_apps_page` (and
> NewTabUI::ShouldShowApps()). This has some settings in
> browser/ui/webui/ntp/ntp_resource_cache.h -- should you be doing something
> similar?

Done. This is much neater.

[tapted, 2013-05-30 07:05:13]

cool - lgtm

you'll need to rebase for that other, recent chrome://flag change that landed
too

[jackhou1, 2013-05-30 07:48:24]

dbeam, could you please review for OWNERS in chrome/browser/?

[Dan Beam, 2013-05-30 22:19:25]

please update the CL description to include that you're enabling this "create
shortcut" option on mac

https://codereview.chromium.org/16191003/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/ntp4/apps_page.js (left):

https://codereview.chromium.org/16191003/diff/21001/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.js:143: if (this.createShortcut_ &&
cr.isMac) {
what changed about this property being present?

https://codereview.chromium.org/16191003/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://codereview.chromium.org/16191003/diff/21001/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.js:146: // new-style packaged apps, so
hide the menu item.
^ does this comment need to be updated?

[jackhou1, 2013-05-30 23:07:18]

https://codereview.chromium.org/16191003/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/ntp4/apps_page.js (left):

https://codereview.chromium.org/16191003/diff/21001/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.js:143: if (this.createShortcut_ &&
cr.isMac) {
On 2013/05/30 22:19:25, Dan Beam wrote:
> what changed about this property being present?

Previously, this line would never be true due to line 76:
  if (!(cr.isChromeOS || cr.isMac)) { /* set this.createShortcut_ */ }
but now that this.createShortcut_ is set for Mac as well, it seems redundant to
check for both here.

https://codereview.chromium.org/16191003/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://codereview.chromium.org/16191003/diff/21001/chrome/browser/resources/...
chrome/browser/resources/ntp4/apps_page.js:146: // new-style packaged apps, so
hide the menu item.
On 2013/05/30 22:19:25, Dan Beam wrote:
> ^ does this comment need to be updated?

Done.

[Dan Beam, 2013-05-30 23:12:54]

lgtm

[commit-bot: I haz the power, 2013-05-30 23:24:45]

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

[commit-bot: I haz the power, 2013-05-31 07:47:06]

Change committed as 203363