New issue
Advanced search Search tips

Issue 702551 link

Starred by 1 user
Status: Fixed
Owner:
tapted@chromium.org
Closed: Apr 2017
Cc:
dominickn@chromium.org
benwells@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 1
Type: Bug



Sign in to add a comment

IDC_CREATE_SHORTCUTS is effectively dead[code] - let's delete CreateUrlApplicationShortcutView

Project Member Reported by tapted@chromium.org, Mar 17 2017 
IDC_CREATE_SHORTCUTS is #ifdefed out on Mac in browser_command_controller.cc -- it's always disabled.

On other platforms, it's only added to app_menu_model.cc when `extensions::util::IsNewBookmarkAppsEnabled()` is false, but the definition of that is

bool IsNewBookmarkAppsEnabled() {
#if defined(OS_MACOSX)
  return base::CommandLine::ForCurrentProcess()->HasSwitch(
      switches::kEnableNewBookmarkApps);
#else
  return !base::CommandLine::ForCurrentProcess()->HasSwitch(
      switches::kDisableNewBookmarkApps);
#endif
}

I think we have fully launched this on non-mac, so that flag can be made mac-only.


Then CreateUrlApplicationShortcutView and the stuff it uses can be deleted (e.g. AppInfoView, and IDC_CREATE_SHORTCUTS). This would reduce the number of dialogs we need to check for Harmony.

Comment 1 by tapted@chromium.org, Mar 21 2017

Owner: tapted@chromium.org
Status: Started (was: Untriaged)

Comment 2 by tapted@chromium.org, Mar 23 2017 

fix flags - https://codereview.chromium.org/2765083002/
remove IDC_CREATE_SHORTCUT https://codereview.chromium.org/2772713002/

UMA link: https://uma.googleplex.com/p/chrome/histograms/?endDate=20170321&dayCount=28&histograms=WrenchMenu.MenuAction&fixupData=true&showMax=true&filters=channel%2Ceq%2C4%2Cmilestone%2Ceq%2C57%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial

(shows that in m57 a total of 40 of 330,000,000 menu actions were IDC_CREATE_SHORTCUTS -- basically 0%. It can currently only be done by disabling the flag or via the X11 menu [which is a bug]).
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 23 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/70cdc81ee387422102b362949621882df0bacd25

commit 70cdc81ee387422102b362949621882df0bacd25
Author: tapted <tapted@chromium.org>
Date: Thu Mar 23 20:48:19 2017

Hardcode extensions::util::IsNewBookmarkAppsEnabled() to true, except on Mac.

On Mac, make it a Feature controlled by chrome://flags/#bookmark-apps.
(Currently one needs to "Enable" #disable-new-bookmark-apps which is
confusing). Feature flags have other benefits over command line flags
too (e.g. Finch integration).

On other platforms, it's been on by default for a long time and doesn't
need a disable flag. Hardcoding it to `true` means we have some more
obviously dead code around IDC_CREATE_SHORTCUTS which was never
implemented on Mac, and replaced by bookmark apps elsewhere.

BUG= 702551 

Review-Url: https://codereview.chromium.org/2765083002
Cr-Commit-Position: refs/heads/master@{#459209}

[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/browser/about_flags.cc
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/browser/apps/app_shim/app_shim_interactive_uitest_mac.mm
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/browser/extensions/extension_util.cc
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/browser/ui/app_list/app_list_service_impl_browsertest.cc
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/common/chrome_features.cc
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/common/chrome_features.h
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/common/chrome_switches.cc
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/common/chrome_switches.h
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/chrome/renderer/web_apps.cc
[modify] https://crrev.com/70cdc81ee387422102b362949621882df0bacd25/tools/metrics/histograms/histograms.xml
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b72edff87d31139a6c466b14c153776029bd6008

commit b72edff87d31139a6c466b14c153776029bd6008
Author: tapted <tapted@chromium.org>
Date: Thu Apr 06 01:59:21 2017

Remove IDC_CREATE_SHORTCUT and its associated UI.

IDC_CREATE_SHORTCUT allows a user to create a hosted app from an
arbitrary web page. It was never implemented for Mac. On other
platforms, it has beed replaced by "Add to {Desktop, Homescreen,
Applications}". That is, BookmarkAppConfirmationView is now used instead
of CreateUrlApplicationShortcutView.

And although it may appear that IDC_CREATE_SHORTCUTS is an "enabled"
command, there's actually no way to invoke that command on any platform
because app_menu_model.cc will always add IDC_CREATE_HOSTED_APP instead.
(One exception: the x11 menubar on Desktop Linux had an entry, but
that's accidental).

BUG= 702551 

Review-Url: https://codereview.chromium.org/2772713002
Cr-Commit-Position: refs/heads/master@{#462322}

[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/app/chrome_command_ids.h
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/app/generated_resources.grd
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/extensions/tab_helper.cc
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/extensions/tab_helper.h
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/shell_integration_linux.cc
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/ui/browser_browsertest.cc
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/ui/browser_command_controller.cc
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/ui/browser_commands.cc
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/ui/browser_commands.h
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/ui/browser_dialogs.h
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/ui/toolbar/app_menu_model.cc
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/ui/toolbar/app_menu_model.h
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/ui/views/create_application_shortcut_view.cc
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/ui/views/create_application_shortcut_view.h
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/ui/views/frame/global_menu_bar_x11.cc
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/web_applications/web_app.h
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/chrome/browser/web_applications/web_app_win.cc
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/tools/metrics/actions/actions.xml
[modify] https://crrev.com/b72edff87d31139a6c466b14c153776029bd6008/tools/metrics/histograms/histograms.xml

Comment 5 by tapted@chromium.org, Apr 6 2017

Status: Fixed (was: Started)
►

Sign in to add a comment