Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
307873002:
Removed the App Info information from the Create App Shortcuts dialog (Closed)
|
Created:
6 years, 6 months ago by sashab Modified:
6 years, 6 months ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemoved the App Info information from the Create App Shortcuts dialog
Removed the App Info header from the Create Shortcuts dialog for Apps,
as requested by UI, since it is always clear which app the shortcuts
are being created for. The info still appears when creating shortcuts for
bookmark (URL) apps.
BUG=NONE
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276419
Patch Set 1 #
Total comments: 6
Patch Set 2 : Renamed InitControls to InitControlsWithAppInfoFrame #Patch Set 3 : Enum over boolean #
Total comments: 3
Patch Set 4 : Nits #
Total comments: 8
Patch Set 5 : Review feedback #
Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... File chrome/browser/ui/views/create_application_shortcut_view.cc (left): https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... chrome/browser/ui/views/create_application_shortcut_view.cc:539: base::Bind(&CreateChromeApplicationShortcutView::OnShortcutInfoLoaded, How come this can be deleted? https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... File chrome/browser/ui/views/create_application_shortcut_view.h (right): https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... chrome/browser/ui/views/create_application_shortcut_view.h:49: void InitDialog(); Did you consider passing a boolean into InitControls / InitDialog?
https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... File chrome/browser/ui/views/create_application_shortcut_view.cc (left): https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... chrome/browser/ui/views/create_application_shortcut_view.cc:539: base::Bind(&CreateChromeApplicationShortcutView::OnShortcutInfoLoaded, On 2014/05/29 04:49:34, benwells wrote: > How come this can be deleted? This loads the app's icon asynchronously, which is no longer needed. But good point - I can now delete OnShortcutInfoLoaded too, since URL Applications use DidDownloadFavicon(). https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... File chrome/browser/ui/views/create_application_shortcut_view.h (right): https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... chrome/browser/ui/views/create_application_shortcut_view.h:49: void InitDialog(); On 2014/05/29 04:49:34, benwells wrote: > Did you consider passing a boolean into InitControls / InitDialog? I'm happy either way. InitControls(false) looked a bit weird at first. Renamed to InitControlsWithAppInfoFrame which looks a bit better.
https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... File chrome/browser/ui/views/create_application_shortcut_view.h (right): https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... chrome/browser/ui/views/create_application_shortcut_view.h:49: void InitDialog(); On 2014/05/29 05:18:02, sasha_b wrote: > On 2014/05/29 04:49:34, benwells wrote: > > Did you consider passing a boolean into InitControls / InitDialog? > > I'm happy either way. InitControls(false) looked a bit weird at first. > > Renamed to InitControlsWithAppInfoFrame which looks a bit better. I understand your concern. InitControls(false) is not self documenting at all. Boolean parameters are often avoided for this reason. The problem with InitControlsWithAppInfoFrame(false) is that it is still confusing. When I ignorantly read that function name I think it is going to add the app info frame (WithAppInfoFrame) and I don't know what the false is for. The typical solution to this is to create an enum to name the boolean values, e.g. CreateApplicationShortcutView::DialogStyle::FOR_WEB_PAGE and ::FOR_APP Then you call InitControls(DialogStyle::FOR_WEB_PAGE). Or something like that.
https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... File chrome/browser/ui/views/create_application_shortcut_view.h (right): https://codereview.chromium.org/307873002/diff/1/chrome/browser/ui/views/crea... chrome/browser/ui/views/create_application_shortcut_view.h:49: void InitDialog(); On 2014/05/30 01:57:54, benwells wrote: > On 2014/05/29 05:18:02, sasha_b wrote: > > On 2014/05/29 04:49:34, benwells wrote: > > > Did you consider passing a boolean into InitControls / InitDialog? > > > > I'm happy either way. InitControls(false) looked a bit weird at first. > > > > Renamed to InitControlsWithAppInfoFrame which looks a bit better. > > I understand your concern. InitControls(false) is not self documenting at all. > Boolean parameters are often avoided for this reason. > > The problem with InitControlsWithAppInfoFrame(false) is that it is still > confusing. When I ignorantly read that function name I think it is going to add > the app info frame (WithAppInfoFrame) and I don't know what the false is for. > > The typical solution to this is to create an enum to name the boolean values, > e.g. > CreateApplicationShortcutView::DialogStyle::FOR_WEB_PAGE and ::FOR_APP > > Then you call InitControls(DialogStyle::FOR_WEB_PAGE). Or something like that. Nice suggestion. Done.
lgtm with the nits fixed https://codereview.chromium.org/307873002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/create_application_shortcut_view.cc (right): https://codereview.chromium.org/307873002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.cc:263: if (dialog_layout == DialogLayout::URL_SHORTCUT_LAYOUT) Nit1: as the body of this if spans multiple lines, it needs curlies. Nit2: You don't need the DialogLayout:: here or on the other lines you have.
https://codereview.chromium.org/307873002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/create_application_shortcut_view.cc (right): https://codereview.chromium.org/307873002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.cc:263: if (dialog_layout == DialogLayout::URL_SHORTCUT_LAYOUT) On 2014/06/02 01:04:31, benwells wrote: > Nit1: as the body of this if spans multiple lines, it needs curlies. > > Nit2: You don't need the DialogLayout:: here or on the other lines you have. Done. https://codereview.chromium.org/307873002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.cc:263: if (dialog_layout == DialogLayout::URL_SHORTCUT_LAYOUT) On 2014/06/02 01:04:31, benwells wrote: > Nit1: as the body of this if spans multiple lines, it needs curlies. > > Nit2: You don't need the DialogLayout:: here or on the other lines you have. Done.
The CQ bit was checked by sashab@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/307873002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
LGTM with the following changes. https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/create_application_shortcut_view.cc (right): https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.cc:262: enum DialogLayout dialog_layout) { no enum here either. https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/create_application_shortcut_view.h (right): https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.h:46: URL_SHORTCUT_LAYOUT, Follow naming convention for content, eg DIALOG_LAYOUT_URL_SHORTCUT and DIALOG_LAYOUT_APP_SHORTCUT. https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.h:57: void InitControls(enum DialogLayout dialog_layout); no enum here. https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.h:82: views::View* app_info_; Document this may be NULL.
https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/create_application_shortcut_view.cc (right): https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.cc:262: enum DialogLayout dialog_layout) { On 2014/06/03 17:14:12, sky wrote: > no enum here either. Done. https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/create_application_shortcut_view.h (right): https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.h:46: URL_SHORTCUT_LAYOUT, On 2014/06/03 17:14:12, sky wrote: > Follow naming convention for content, eg DIALOG_LAYOUT_URL_SHORTCUT and > DIALOG_LAYOUT_APP_SHORTCUT. Done. https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.h:57: void InitControls(enum DialogLayout dialog_layout); On 2014/06/03 17:14:12, sky wrote: > no enum here. Done. https://codereview.chromium.org/307873002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.h:82: views::View* app_info_; On 2014/06/03 17:14:12, sky wrote: > Document this may be NULL. Done.
The CQ bit was checked by sashab@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/307873002/80001
Message was sent while issue was closed.
Change committed as 276419 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
