Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2427323003:
MD Settings: Migrate cups_printers_list.html to settings-action-menu. (Closed)
|
Created:
4 years, 2 months ago by dpapad Modified:
4 years, 2 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, vitalyp+closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Migrate cups_printers_list.html to settings-action-menu.
BUG=639718, 603976
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/3188c5dbeaac9424446fb392f02756cb4374f9f3
Cr-Commit-Position: refs/heads/master@{#426672}
Patch Set 1 #Patch Set 2 : Nit. #Patch Set 3 : Remove cast. #Patch Set 4 : Remove paper-item dependency. #
Total comments: 13
Patch Set 5 : Address comments. #
Total comments: 4
Patch Set 6 : Nit #
Messages
Total messages: 22 (9 generated)
Description was changed from ========== MD Settings: Migrate cups_printers_list.html to settings-action-menu. BUG=639718 ========== to ========== MD Settings: Migrate cups_printers_list.html to settings-action-menu. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Migrate cups_printers_list.html to settings-action-menu. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrate cups_printers_list.html to settings-action-menu. BUG=639718,603976 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@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...
dpapad@chromium.org changed reviewers: + michaelpg@chromium.org
FYI, I discovered an exception being thrown from the cups UI (even without my CL), will file a bug about it (see exception at http://imgur.com/a/s6CuJ).
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org, xdai@chromium.org
lgtm +xdai@ FYI
On 2016/10/20 00:27:15, stevenjb wrote: > lgtm > > +xdai@ FYI Er, ignore unintentional "quick lgtm"
On 2016/10/20 00:27:34, stevenjb wrote: > On 2016/10/20 00:27:15, stevenjb wrote: > > lgtm > > > > +xdai@ FYI > > Er, ignore unintentional "quick lgtm" lgtm
On 2016/10/20 at 16:51:08, xdai wrote: > On 2016/10/20 00:27:34, stevenjb wrote: > > On 2016/10/20 00:27:15, stevenjb wrote: > > > lgtm > > > > > > +xdai@ FYI > > > > Er, ignore unintentional "quick lgtm" > > lgtm @Steven: Are you planning to review this, or should I consider xdai's LG sufficient?
https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_list.html (right): https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.html:27: <dialog is="settings-action-menu"> how does this menu react when it's opened for Printer X, and a change comes from Chrome which removes Printer X? https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.html:29: $i18n{cupsPrinterDetails} indents off https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.html:42: <paper-icon-button icon="cr:more-vert" on-tap="onDotsTap_"> nit: name what the button is, not what it looks like https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_list.js (right): https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.js:39: * @param {{model: {item: !CupsPrinterInfo}}} e nit: more !s https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.js:71: this.activePrinter_.printerName); maybe set this.activePrinter_ = null; so it's not referencing a (soon-to-be) removed printer?
https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_list.html (right): https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.html:27: <dialog is="settings-action-menu"> On 2016/10/20 at 18:00:39, michaelpg wrote: > how does this menu react when it's opened for Printer X, and a change comes from Chrome which removes Printer X? This is not handled at the moment. I can move this dialog inside the dom-repeat again, and wrap it with a lazy-render or dom-if. This way it would be removed from the DOM when the dom-repeat updates. Or perhaps there is a way to still have 1 action menu for N rows, and somehow observe whether the current printer just got removed. BTW, case you are describing probably breaks other such menus in our codebase where we chosen to have 1 menu for N rows. https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.html:29: $i18n{cupsPrinterDetails} On 2016/10/20 at 18:00:39, michaelpg wrote: > indents off Done. https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.html:42: <paper-icon-button icon="cr:more-vert" on-tap="onDotsTap_"> On 2016/10/20 at 18:00:39, michaelpg wrote: > nit: name what the button is, not what it looks like OK, but https://cs.chromium.org/search/?q=onDotsTap_&sq=package:chromium&type=cs. https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_list.js (right): https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.js:39: * @param {{model: {item: !CupsPrinterInfo}}} e On 2016/10/20 at 18:00:39, michaelpg wrote: > nit: more !s Done. https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.js:71: this.activePrinter_.printerName); On 2016/10/20 at 18:00:39, michaelpg wrote: > maybe set this.activePrinter_ = null; so it's not referencing a (soon-to-be) removed printer? Updated closeDropdownMenu_ to set the activePrinter back to null, and moved the calls at the end of onRemoveTap/onDetailsTap.
I was planning to let michael and xdai@ do the review On Thu, Oct 20, 2016 at 11:15 AM, dpapad@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > > https://codereview.chromium.org/2427323003/diff/60001/ > chrome/browser/resources/settings/printing_page/cups_printers_list.html > File > chrome/browser/resources/settings/printing_page/cups_printers_list.html > (right): > > https://codereview.chromium.org/2427323003/diff/60001/ > chrome/browser/resources/settings/printing_page/cups_ > printers_list.html#newcode27 > chrome/browser/resources/settings/printing_page/cups_ > printers_list.html:27: > <dialog is="settings-action-menu"> > On 2016/10/20 at 18:00:39, michaelpg wrote: > > how does this menu react when it's opened for Printer X, and a change > comes from Chrome which removes Printer X? > > This is not handled at the moment. I can move this dialog inside the > dom-repeat again, and wrap it with a lazy-render or dom-if. This way it > would be removed from the DOM when the dom-repeat updates. Or perhaps > there is a way to still have 1 action menu for N rows, and somehow > observe whether the current printer just got removed. > > BTW, case you are describing probably breaks other such menus in our > codebase where we chosen to have 1 menu for N rows. > > https://codereview.chromium.org/2427323003/diff/60001/ > chrome/browser/resources/settings/printing_page/cups_ > printers_list.html#newcode29 > chrome/browser/resources/settings/printing_page/cups_ > printers_list.html:29: > $i18n{cupsPrinterDetails} > On 2016/10/20 at 18:00:39, michaelpg wrote: > > indents off > > Done. > > https://codereview.chromium.org/2427323003/diff/60001/ > chrome/browser/resources/settings/printing_page/cups_ > printers_list.html#newcode42 > chrome/browser/resources/settings/printing_page/cups_ > printers_list.html:42: > <paper-icon-button icon="cr:more-vert" on-tap="onDotsTap_"> > On 2016/10/20 at 18:00:39, michaelpg wrote: > > nit: name what the button is, not what it looks like > > OK, but > https://cs.chromium.org/search/?q=onDotsTap_&sq=package:chromium&type=cs. > > https://codereview.chromium.org/2427323003/diff/60001/ > chrome/browser/resources/settings/printing_page/cups_printers_list.js > File > chrome/browser/resources/settings/printing_page/cups_printers_list.js > (right): > > https://codereview.chromium.org/2427323003/diff/60001/ > chrome/browser/resources/settings/printing_page/cups_ > printers_list.js#newcode39 > chrome/browser/resources/settings/printing_page/cups_printers_list.js:39: > * @param {{model: {item: !CupsPrinterInfo}}} e > On 2016/10/20 at 18:00:39, michaelpg wrote: > > nit: more !s > > Done. > > https://codereview.chromium.org/2427323003/diff/60001/ > chrome/browser/resources/settings/printing_page/cups_ > printers_list.js#newcode71 > chrome/browser/resources/settings/printing_page/cups_printers_list.js:71: > this.activePrinter_.printerName); > On 2016/10/20 at 18:00:39, michaelpg wrote: > > maybe set this.activePrinter_ = null; so it's not referencing a > (soon-to-be) removed printer? > > Updated closeDropdownMenu_ to set the activePrinter back to null, and > moved the calls at the end of onRemoveTap/onDetailsTap. > > https://codereview.chromium.org/2427323003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_list.html (right): https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.html:27: <dialog is="settings-action-menu"> On 2016/10/20 18:15:56, dpapad wrote: > On 2016/10/20 at 18:00:39, michaelpg wrote: > > how does this menu react when it's opened for Printer X, and a change comes > from Chrome which removes Printer X? > > This is not handled at the moment. I can move this dialog inside the dom-repeat > again, and wrap it with a lazy-render or dom-if. This way it would be removed > from the DOM when the dom-repeat updates. Or perhaps there is a way to still > have 1 action menu for N rows, and somehow observe whether the current printer > just got removed. Would that cause a dom-if template to be created for each item? :-\ I think it's fine if we just call close() in the menu item handlers. > > BTW, case you are describing probably breaks other such menus in our codebase > where we chosen to have 1 menu for N rows. I just checked, and you're right. Filed http://crbug.com/657970 https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.html:42: <paper-icon-button icon="cr:more-vert" on-tap="onDotsTap_"> On 2016/10/20 18:15:56, dpapad wrote: > On 2016/10/20 at 18:00:39, michaelpg wrote: > > nit: name what the button is, not what it looks like > > OK, but > https://cs.chromium.org/search/?q=onDotsTap_&sq=package:chromium&type=cs. Huh, ok then. Somehow I'd never seen it before. https://codereview.chromium.org/2427323003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_list.js (right): https://codereview.chromium.org/2427323003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.js:46: menu.showAt(/** @type {!Element} */ ( why is this cast necessary? just to tell closure it's non-null? https://codereview.chromium.org/2427323003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.js:58: rm blank line
https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_list.html (right): https://codereview.chromium.org/2427323003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.html:27: <dialog is="settings-action-menu"> On 2016/10/20 at 19:49:05, michaelpg wrote: > On 2016/10/20 18:15:56, dpapad wrote: > > On 2016/10/20 at 18:00:39, michaelpg wrote: > > > how does this menu react when it's opened for Printer X, and a change comes > > from Chrome which removes Printer X? > > > > This is not handled at the moment. I can move this dialog inside the dom-repeat > > again, and wrap it with a lazy-render or dom-if. This way it would be removed > > from the DOM when the dom-repeat updates. Or perhaps there is a way to still > > have 1 action menu for N rows, and somehow observe whether the current printer > > just got removed. > > Would that cause a dom-if template to be created for each item? :-\ > > I think it's fine if we just call close() in the menu item handlers. > > > > > BTW, case you are describing probably breaks other such menus in our codebase > > where we chosen to have 1 menu for N rows. > > I just checked, and you're right. Filed http://crbug.com/657970 Yes, it would cause a dom-if template node for each item. We already do this for the certificate action menu, see https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif.... We do close the menu when an action happens, but don't bother removing it from dom, we just use cr-lazy-render. So if you were to open 100 menus for 100 different items, the page would get gradually more populated. https://codereview.chromium.org/2427323003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_list.js (right): https://codereview.chromium.org/2427323003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.js:46: menu.showAt(/** @type {!Element} */ ( On 2016/10/20 at 19:49:06, michaelpg wrote: > why is this cast necessary? just to tell closure it's non-null? Correct. https://codereview.chromium.org/2427323003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_list.js:58: On 2016/10/20 at 19:49:06, michaelpg wrote: > rm blank line Done.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, xdai@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2427323003/#ps100001 (title: "Nit")
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.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Migrate cups_printers_list.html to settings-action-menu. BUG=639718,603976 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrate cups_printers_list.html to settings-action-menu. BUG=639718,603976 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/3188c5dbeaac9424446fb392f02756cb4374f9f3 Cr-Commit-Position: refs/heads/master@{#426672} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3188c5dbeaac9424446fb392f02756cb4374f9f3 Cr-Commit-Position: refs/heads/master@{#426672} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
