Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2351023002:
[CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome… (Closed)
Description[CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome OS.
Activating the button should take the user to the Printer Management Settings
subpage (chrome://md-settings/cupsPrinters).
BUG=625374, 626752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/3a881a0380881933aa502ede02c4597088e8121d
Cr-Commit-Position: refs/heads/master@{#420522}
Patch Set 1 : [CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome… #
Total comments: 5
Patch Set 2 : Address dpapad@'s comments. #Patch Set 3 : Address dbeam@'s comments. #
Total comments: 11
Patch Set 4 : Address comments. Rebase #Patch Set 5 : Address dpapad@'s comments. #
Total comments: 8
Patch Set 6 : Address comments. #Patch Set 7 : Address dpapad@'s comments. #
Total comments: 3
Messages
Total messages: 44 (19 generated)
Description was changed from ========== [CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome OS. Activating the button should take the user to the Printer Management Settings subpage (chrome://md-settings/cupsPrinters). BUG=627652, 625374 ========== to ========== [CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome OS. Activating the button should take the user to the Printer Management Settings subpage (chrome://md-settings/cupsPrinters). BUG=627652, 625374 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
xdai@chromium.org changed reviewers: + dpapad@chromium.org
dpapad@, could you help review this CL please? Thanks!
Description was changed from ========== [CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome OS. Activating the button should take the user to the Printer Management Settings subpage (chrome://md-settings/cupsPrinters). BUG=627652, 625374 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome OS. Activating the button should take the user to the Printer Management Settings subpage (chrome://md-settings/cupsPrinters). BUG=625374, 627652 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== [CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome OS. Activating the button should take the user to the Printer Management Settings subpage (chrome://md-settings/cupsPrinters). BUG=625374, 627652 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome OS. Activating the button should take the user to the Printer Management Settings subpage (chrome://md-settings/cupsPrinters). BUG=625374, 626752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2351023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2351023002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:691: if (cr.isChromeOS) { Is this the right place to intercept? How about adding this conditional logic within the event handler of the event that is already dispatched below? Also how is this working if chrome://md-settings is not enabled? AFAIK this is still behind a feature flag, but the behavior here will change even for when the feature flag is not enabled.
dpapad@, thanks for your comments! Please see my inline questions below. https://codereview.chromium.org/2351023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2351023002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:691: if (cr.isChromeOS) { On 2016/09/19 22:21:19, dpapad wrote: > Is this the right place to intercept? How about adding this conditional logic > within the event handler of the event that is already dispatched below? Do you mean PrintPreview.onManageLocalDestinationsActivated_() in print_preview.js or the webui handler function PrintPreviewHandler::HandleManagePrinters() in print_preview_handler.cc? > > > Also how is this working if chrome://md-settings is not enabled? AFAIK this is > still behind a feature flag, but the behavior here will change even for when the > feature flag is not enabled. The CUPS Printing is also behind a command flag (--enable-native-cups) and currently is targeting M56. I guess the MD setting should have been launched by that moment? If not, I will need check with the PM to see if there is any other option for this issue.
+dbeam https://codereview.chromium.org/2351023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2351023002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:691: if (cr.isChromeOS) { On 2016/09/19 at 22:53:40, xdai1 wrote: > On 2016/09/19 22:21:19, dpapad wrote: > > Is this the right place to intercept? How about adding this conditional logic > > within the event handler of the event that is already dispatched below? > > Do you mean PrintPreview.onManageLocalDestinationsActivated_() in print_preview.js or the webui handler function PrintPreviewHandler::HandleManagePrinters() in print_preview_handler.cc? I meant the one in print_preview.js. It seems more appropriate to handle the event there. > > > > > > > Also how is this working if chrome://md-settings is not enabled? AFAIK this is > > still behind a feature flag, but the behavior here will change even for when the > > feature flag is not enabled. > > The CUPS Printing is also behind a command flag (--enable-native-cups) and currently is targeting M56. I guess the MD setting should have been launched by that moment? If not, I will need check with the PM to see if there is any other option for this issue. https://codereview.chromium.org/2351023002/diff/20001/chrome/common/url_const... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2351023002/diff/20001/chrome/common/url_const... chrome/common/url_constants.cc:129: const char kChromeUIMdCupsSettingsURL[] = "chrome://md-settings/cupsPrinters"; When MD Settings launches, it will be moved under chrome://settings, not chrome://md-settings. I don't know the correct way to do this and have it work before and after launching. +dbeam: Is there a more robust way to link from the print_preview page to the new MD Settings page (without hard-coding the chrome://md-settings string)?
dpapad@, I've addressed your comments. Please take another look. In the meantime, I'll wait for dbeam@'s comment to see if there is a better way to link to the new MD settings subpage. https://codereview.chromium.org/2351023002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2351023002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:691: if (cr.isChromeOS) { On 2016/09/20 00:03:23, dpapad wrote: > On 2016/09/19 at 22:53:40, xdai1 wrote: > > On 2016/09/19 22:21:19, dpapad wrote: > > > Is this the right place to intercept? How about adding this conditional > logic > > > within the event handler of the event that is already dispatched below? > > > > Do you mean PrintPreview.onManageLocalDestinationsActivated_() in > print_preview.js or the webui handler function > PrintPreviewHandler::HandleManagePrinters() in print_preview_handler.cc? > > I meant the one in print_preview.js. It seems more appropriate to handle the > event there. > > > > > > > > > > > > Also how is this working if chrome://md-settings is not enabled? AFAIK this > is > > > still behind a feature flag, but the behavior here will change even for when > the > > > feature flag is not enabled. > > > > The CUPS Printing is also behind a command flag (--enable-native-cups) and > currently is targeting M56. I guess the MD setting should have been launched by > that moment? If not, I will need check with the PM to see if there is any other > option for this issue. Done.
On 2016/09/20 00:03:23, dpapad wrote: > +dbeam > > https://codereview.chromium.org/2351023002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/print_preview/search/destination_search.js > (right): > > https://codereview.chromium.org/2351023002/diff/20001/chrome/browser/resource... > chrome/browser/resources/print_preview/search/destination_search.js:691: if > (cr.isChromeOS) { > On 2016/09/19 at 22:53:40, xdai1 wrote: > > On 2016/09/19 22:21:19, dpapad wrote: > > > Is this the right place to intercept? How about adding this conditional > logic > > > within the event handler of the event that is already dispatched below? > > > > Do you mean PrintPreview.onManageLocalDestinationsActivated_() in > print_preview.js or the webui handler function > PrintPreviewHandler::HandleManagePrinters() in print_preview_handler.cc? > > I meant the one in print_preview.js. It seems more appropriate to handle the > event there. > > > > > > > > > > > > Also how is this working if chrome://md-settings is not enabled? AFAIK this > is > > > still behind a feature flag, but the behavior here will change even for when > the > > > feature flag is not enabled. > > > > The CUPS Printing is also behind a command flag (--enable-native-cups) and > currently is targeting M56. I guess the MD setting should have been launched by > that moment? If not, I will need check with the PM to see if there is any other > option for this issue. > > https://codereview.chromium.org/2351023002/diff/20001/chrome/common/url_const... > File chrome/common/url_constants.cc (right): > > https://codereview.chromium.org/2351023002/diff/20001/chrome/common/url_const... > chrome/common/url_constants.cc:129: const char kChromeUIMdCupsSettingsURL[] = > "chrome://md-settings/cupsPrinters"; > When MD Settings launches, it will be moved under chrome://settings, not > chrome://md-settings. I don't know the correct way to do this and have it work > before and after launching. > > +dbeam: Is there a more robust way to link from the print_preview page to the > new MD Settings page (without hard-coding the chrome://md-settings string)? xdai@: don't use that URL, it's a development-only URL. additionally, if you're linking to something that's only in the new settings, you probably want to use if (base::FeatureList::IsEnabled(features::kMaterialDesignSettings)) around your code
xdai@chromium.org changed reviewers: + dbeam@chromium.org
dpapad@, dbeam@, please take another look, thanks!
https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:101: cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton') ? showLocalManageButton will be falsy if not in ChromeOS, so hte first part of this check is probably redundant. https://codereview.chromium.org/2351023002/diff/60001/chrome/common/url_const... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2351023002/diff/60001/chrome/common/url_const... chrome/common/url_constants.cc:129: const char kChromeUIMdCupsSettingsURL[] = "chrome://md-settings/cupsPrinters"; I am guessing that since the code now requires features::kMaterialDesignSettings to be true to take effect, this URL should be chrome://settings/cupsPrinters, no?
https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:966: if (cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton')) { getBoolean() fails if the boolean isn't set it seems like this boolean is only set on ChromeOS SO, you can do one of two things: 1) loadTimeData.valueExists('showLocalManageButton') && loadTimeData.getBoolean('showLocalManageButton') 2) set it to false on !defined(OS_CHROMEOS) https://codereview.chromium.org/2351023002/diff/60001/chrome/common/url_const... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2351023002/diff/60001/chrome/common/url_const... chrome/common/url_constants.cc:129: const char kChromeUIMdCupsSettingsURL[] = "chrome://md-settings/cupsPrinters"; On 2016/09/21 00:39:01, dpapad wrote: > I am guessing that since the code now requires features::kMaterialDesignSettings > to be true to take effect, this URL should be chrome://settings/cupsPrinters, > no? yeah, i think you want to drop the "md-"
dpapad@, dbeam@, I've addressed your comments. Please take another look, thanks! https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:966: if (cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton')) { On 2016/09/21 02:08:40, Dan Beam wrote: > getBoolean() fails if the boolean isn't set > > it seems like this boolean is only set on ChromeOS > > SO, you can do one of two things: > > 1) loadTimeData.valueExists('showLocalManageButton') && > loadTimeData.getBoolean('showLocalManageButton') > > 2) set it to false on !defined(OS_CHROMEOS) I think that's the reason we need the first clause (cr.isChromeOS): showLocalManageButton only exists on ChromeOS, so it's guaranteed that getBoolean() doesn't fail on ChromeOS. https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:101: cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton') ? On 2016/09/21 00:39:01, dpapad wrote: > showLocalManageButton will be falsy if not in ChromeOS, so hte first part of > this check is probably redundant. showLocalManageButton doesn't exist if not on ChromeOS, so the first part is still necessary? https://codereview.chromium.org/2351023002/diff/60001/chrome/common/url_const... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2351023002/diff/60001/chrome/common/url_const... chrome/common/url_constants.cc:129: const char kChromeUIMdCupsSettingsURL[] = "chrome://md-settings/cupsPrinters"; On 2016/09/21 02:08:40, Dan Beam wrote: > On 2016/09/21 00:39:01, dpapad wrote: > > I am guessing that since the code now requires > features::kMaterialDesignSettings > > to be true to take effect, this URL should be chrome://settings/cupsPrinters, > > no? > > yeah, i think you want to drop the "md-" Done.
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, with nit. https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:966: if (cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton')) { On 2016/09/21 at 18:03:21, xdai1 wrote: > On 2016/09/21 02:08:40, Dan Beam wrote: > > getBoolean() fails if the boolean isn't set > > > > it seems like this boolean is only set on ChromeOS > > > > SO, you can do one of two things: > > > > 1) loadTimeData.valueExists('showLocalManageButton') && > > loadTimeData.getBoolean('showLocalManageButton') > > > > 2) set it to false on !defined(OS_CHROMEOS) > > I think that's the reason we need the first clause (cr.isChromeOS): showLocalManageButton only exists on ChromeOS, so it's guaranteed that getBoolean() doesn't fail on ChromeOS. dbeam's suggestion 2, seems a bit cleaner. showLocalManageButton will exist always, it will be false on non-ChromeOS, and the ifdef-like conditions will be limited to the C++, instead of being spread across C++ and JS. https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:101: cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton') ? On 2016/09/21 at 18:03:21, xdai1 wrote: > On 2016/09/21 00:39:01, dpapad wrote: > > showLocalManageButton will be falsy if not in ChromeOS, so hte first part of > > this check is probably redundant. > > showLocalManageButton doesn't exist if not on ChromeOS, so the first part is still necessary? See other related. Making it exist for non-ChromeOS seems to make things cleaner.
Comments addressed. Please take another look, thanks! https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/print_preview.js:966: if (cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton')) { On 2016/09/21 18:13:47, dpapad wrote: > On 2016/09/21 at 18:03:21, xdai1 wrote: > > On 2016/09/21 02:08:40, Dan Beam wrote: > > > getBoolean() fails if the boolean isn't set > > > > > > it seems like this boolean is only set on ChromeOS > > > > > > SO, you can do one of two things: > > > > > > 1) loadTimeData.valueExists('showLocalManageButton') && > > > loadTimeData.getBoolean('showLocalManageButton') > > > > > > 2) set it to false on !defined(OS_CHROMEOS) > > > > I think that's the reason we need the first clause (cr.isChromeOS): > showLocalManageButton only exists on ChromeOS, so it's guaranteed that > getBoolean() doesn't fail on ChromeOS. > > dbeam's suggestion 2, seems a bit cleaner. showLocalManageButton will exist > always, it will be false on non-ChromeOS, and the ifdef-like conditions will be > limited to the C++, instead of being spread across C++ and JS. See the comment below. I think I made a mistake in the previous patch. showLocalManageButton should always be true for systems other than ChromeOS. so the first clause is necessary here. https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2351023002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:101: cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton') ? On 2016/09/21 18:13:47, dpapad wrote: > On 2016/09/21 at 18:03:21, xdai1 wrote: > > On 2016/09/21 00:39:01, dpapad wrote: > > > showLocalManageButton will be falsy if not in ChromeOS, so hte first part of > > > this check is probably redundant. > > > > showLocalManageButton doesn't exist if not on ChromeOS, so the first part is > still necessary? > > See other related. Making it exist for non-ChromeOS seems to make things > cleaner. Sorry I made a mistake here. We should always show "Manage" button on systems other than ChromeOS. so I need to set showLocalManageButton to true on other systems.
https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:412: source->AddBoolean("showLocalManageButton", true); i'm confused. why should this be true on !chromeos? md-settings (where you're linking to) hasn't shipped on Mac, Windows, or Linux...
See the inline comment. https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:412: source->AddBoolean("showLocalManageButton", true); On 2016/09/21 19:12:02, Dan Beam wrote: > i'm confused. why should this be true on !chromeos? md-settings (where you're > linking to) hasn't shipped on Mac, Windows, or Linux... "Manage" button has been on other systems before this change (see code here: https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/s...). On the other systems, it will open the system printer settings, see code here: https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/p....
lgtm w/suggestion and questions Q: is CUPS printing ever going to be supported outside of ChromeOS? S: rather than forcing people that touch this code later to understand both variables and how checking one is required before using another, try your hardest to bake things like enableness/existence in the same place as the value. especially for code that might eventually be used on other platforms. if a local printing management URL was planned to be used on other platforms, we should be checking solely whether the URL exists and using it, regardless of platform, so this code doesn't need to be changed as much if e.g. Windows adds CUPS support in the future. https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:966: if (cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton')) { if (loadTimeData.valueExists('localPrintersManagementURL')) window.open(loadTimeData.getString('localPrintersManagementURL')); else this.nativeLayer_.startManageLocalDestinations(); https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:409: source->AddString("localPrintersManagementURL", what do you think about this? #if defined(OS_CHROMEOS) bool cups_and_md_settings = ... switch and feature checks ...; source->AddBoolean("showLocalManageButton", cups_and_md_settings); if (cups_and_md_settings) { source->AddString("localPrintersManagementURL", chrome::kChromeUIMdCupsSettingsURL); } #else source->AddBoolean("showLocalManageButton", true); #endif
https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:966: if (cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton')) { I think the confusing part is here. Why would this listener execute if loadTimeData.getBoolean('showLocalManageButton') is false in the first place? Wouldn't that cause the button to never be even shown? I think 'showLocalManageButton' naming is confusing. Would renaming as something as 'showWebUICupsConfig' make more sense? Then it would be if (cr.isChromeOS && loadTimeData.getBoolean('showWebUICupsConfig')) { ... }
Please take another look, thanks again! https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:966: if (cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton')) { On 2016/09/21 20:58:23, dpapad wrote: > I think the confusing part is here. Why would this listener execute if > loadTimeData.getBoolean('showLocalManageButton') is false in the first place? > Wouldn't that cause the button to never be even shown? > > I think 'showLocalManageButton' naming is confusing. Would renaming as something > as 'showWebUICupsConfig' make more sense? Then it would be > > if (cr.isChromeOS && loadTimeData.getBoolean('showWebUICupsConfig')) { > ... > } You're right that we won't get the event in the first place if loadTimeData.getBoolean('showLocalManageButton') is false. Since 'showLocalManageButton' can only be false on Chrome OS, maybe we only check if it's Chromeos? https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:409: source->AddString("localPrintersManagementURL", On 2016/09/21 20:56:25, Dan Beam wrote: > what do you think about this? > > #if defined(OS_CHROMEOS) > bool cups_and_md_settings = ... switch and feature checks ...; > source->AddBoolean("showLocalManageButton", cups_and_md_settings); > > if (cups_and_md_settings) { > source->AddString("localPrintersManagementURL", > chrome::kChromeUIMdCupsSettingsURL); > } > #else > source->AddBoolean("showLocalManageButton", true); > #endif Done.
https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/2351023002/diff/100001/chrome/browser/resourc... chrome/browser/resources/print_preview/print_preview.js:966: if (cr.isChromeOS && loadTimeData.getBoolean('showLocalManageButton')) { > You're right that we won't get the event in the first place if loadTimeData.getBoolean('showLocalManageButton') is false. Since 'showLocalManageButton' can only be false on Chrome OS, maybe we only check if it's Chromeos? That looks fine. Alternatively, we could just do this check in C++ instead, and open the new tab in a similar manner as https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/pr.... The benefit is that localPrintersManagementURL would not need to be passed to JS at all, and we would trade a Javascript runtime isChromeOS check with a compile-time C++ ifdef. This is still LG as-is. Up to you if you want to handle the event in C++ instead.
dpapad@, modified as you suggested. Thanks for the review!
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2351023002/#ps140001 (title: "Address dpapad@'s comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome OS. Activating the button should take the user to the Printer Management Settings subpage (chrome://md-settings/cupsPrinters). BUG=625374, 626752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome OS. Activating the button should take the user to the Printer Management Settings subpage (chrome://md-settings/cupsPrinters). BUG=625374, 626752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome OS. Activating the button should take the user to the Printer Management Settings subpage (chrome://md-settings/cupsPrinters). BUG=625374, 626752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [CUPS] Add a "Manage..." button in the local destinations section of Print Preview Dialog on Chrome OS. Activating the button should take the user to the Printer Management Settings subpage (chrome://md-settings/cupsPrinters). BUG=625374, 626752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/3a881a0380881933aa502ede02c4597088e8121d Cr-Commit-Position: refs/heads/master@{#420522} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3a881a0380881933aa502ede02c4597088e8121d Cr-Commit-Position: refs/heads/master@{#420522}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2351023002/diff/140001/chrome/common/url_cons... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2351023002/diff/140001/chrome/common/url_cons... chrome/common/url_constants.cc:129: const char kChromeUIMdCupsSettingsURL[] = "chrome://settings/cupsPrinters"; Most of the other options are foo-bar, not fooBar.
Message was sent while issue was closed.
https://codereview.chromium.org/2351023002/diff/140001/chrome/common/url_cons... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2351023002/diff/140001/chrome/common/url_cons... chrome/common/url_constants.cc:129: const char kChromeUIMdCupsSettingsURL[] = "chrome://settings/cupsPrinters"; On 2016/09/23 01:39:28, Lei Zhang wrote: > Most of the other options are foo-bar, not fooBar. what? you mean in the ://host-name or in the /pathName? because those are different. this URL already exists, and it looks like it follows the rest of these chrome://settings/clearBrowserData chrome://settings/contentExceptions chrome://settings/createProfile chrome://settings/importData chrome://settings/syncSetup chrome://settings/resetProfileSettings
Message was sent while issue was closed.
https://codereview.chromium.org/2351023002/diff/140001/chrome/common/url_cons... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/2351023002/diff/140001/chrome/common/url_cons... chrome/common/url_constants.cc:129: const char kChromeUIMdCupsSettingsURL[] = "chrome://settings/cupsPrinters"; On 2016/09/23 01:58:21, Dan Beam wrote: > On 2016/09/23 01:39:28, Lei Zhang wrote: > > Most of the other options are foo-bar, not fooBar. > > what? you mean in the ://host-name or in the /pathName? because those are > different. > > this URL already exists, and it looks like it follows the rest of these > > chrome://settings/clearBrowserData > chrome://settings/contentExceptions > chrome://settings/createProfile > chrome://settings/importData > chrome://settings/syncSetup > chrome://settings/resetProfileSettings Ok, n/m. It's just that everything within +/- 10 lines is foo-bar. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
