Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2760753002:
[CUPS] Implement the local CUPS printer setup waiting UI. (Closed)
Description[CUPS] Implement the local CUPS printer setup waiting UI.
BUG=677567
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2760753002
Cr-Commit-Position: refs/heads/master@{#461310}
Committed: https://chromium.googlesource.com/chromium/src/+/89ca6553d7d8d0303bfef17d17c20e0814b9ede2
Patch Set 1 : . #
Total comments: 1
Patch Set 2 : Address the offline comments from weifangsun@ and skau@. #
Total comments: 4
Patch Set 3 : Address skau@'s comment. Rebase #
Total comments: 19
Patch Set 4 : Address dpapad@ and thestig@'s comments. #
Total comments: 2
Patch Set 5 : Address thestig@'s comment. #
Total comments: 2
Patch Set 6 : nit fix. #
Total comments: 12
Patch Set 7 : Fix compile error. #Patch Set 8 : Address dpapad@'s comments. #
Total comments: 4
Patch Set 9 : Address dpapad@'s comments. Fix failed test. #Messages
Total messages: 51 (28 generated)
Description was changed from ========== [CUPS] Implement the local CUPS printer setup waiting UI. BUG=677567 ========== to ========== [CUPS] Implement the local CUPS printer setup waiting UI. BUG=677567 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
xdai@chromium.org changed reviewers: + skau@chromium.org
skau@, could you take a look at this CL to see this approach looks good to you? After that, I'll request the owner's review. Thanks! I recorded two videos FYI, see https://drive.google.com/a/google.com/file/d/0B4R9nvLdzmC9Vm5ScEd6S3J1TFk/vie... and https://drive.google.com/a/google.com/file/d/0B4R9nvLdzmC9X3hlWEFrN0h5bHc/vie.... Note I manually set 2 seconds timeout during setup in order to see the jumped dot animation. I also checked with weifangsun@, she prefers not blocking the entire UI while setting up printer for now.
...And if it looks good, I will go ahead and modify the test.
Patchset #1 (id:1) has been deleted
Description was changed from ========== [CUPS] Implement the local CUPS printer setup waiting UI. BUG=677567 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [CUPS] Implement the local CUPS printer setup waiting UI. BUG=677567 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
I'm taking a look now. How does this handle clicking on multiple items in the list? Can we get multiple SELECT events?
On 2017/03/20 18:42:23, skau wrote: > I'm taking a look now. How does this handle clicking on multiple items in the > list? Can we get multiple SELECT events? This CL doesn't handle this case right now. We can get multiple SELECT events and can try to set up multiple printer at the same time. Per the discussion with weifangsun@'s last week, she preferred do nothing for subsequent events (i.e., we only set up the first selected printer) and not block the entire print dialog UI. weifangsun@, could you provide more detail? If we agree on this approach, I can implement it on a following CL.
Separate from the desired UX, you should ensure that we only call onDestinationResolved_ for the last printer that was selected. https://codereview.chromium.org/2760753002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_list_item.js (right): https://codereview.chromium.org/2760753002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list_item.js:237: this.onDestinationResolved_(); This component is not designed to have this called more than once.
Hi all, Just to clarify - By "do nothing", I would prefer the behavior in the UI to be, select the first printer and if, while this printer is setting up, the user clicks on a different printer, this should not trigger an attempt to set up that printer. Does that make sense? Let me know if I should set up a quick sync if it's easier to discuss in person. Thanks, Weifang On Mon, Mar 20, 2017 at 3:16 PM, <skau@chromium.org> wrote: > Separate from the desired UX, you should ensure that we only call > onDestinationResolved_ for the last printer that was selected. > > > https://codereview.chromium.org/2760753002/diff/20001/ > chrome/browser/resources/print_preview/search/destination_list_item.js > File > chrome/browser/resources/print_preview/search/destination_list_item.js > (right): > > https://codereview.chromium.org/2760753002/diff/20001/ > chrome/browser/resources/print_preview/search/destination_list_item.js# > newcode237 > chrome/browser/resources/print_preview/search/ > destination_list_item.js:237: > this.onDestinationResolved_(); > This component is not designed to have this called more than once. > > https://codereview.chromium.org/2760753002/ > -- 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.
skau@, sorry for the late reply. I was distracted by other issues. Please take another look at this approach to see if it looks good to you. I also recorded two new videos for your reference, see the successful case https://drive.google.com/a/google.com/file/d/0B4R9nvLdzmC9ZUhXWkdueHBMVFk/vie... and error case https://drive.google.com/a/google.com/file/d/0B4R9nvLdzmC9M2QtTmlSSmFrbWc/vie.... According to weifangsun@'s opinion, the entire UI is not grayed out when one printer is in setup process.
Can you verify that we preserve the original behavior if we're not in ChromeOS? https://codereview.chromium.org/2760753002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_list_item.js (right): https://codereview.chromium.org/2760753002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list_item.js:255: onActivate_: function() { If we're not on ChromeOS, we don't need to check for resolution. We should check for cr.isChromeOS and activate the destination if we're not. https://codereview.chromium.org/2760753002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2760753002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:582: * allow configure more than one destination at the same time. nit: s/configure/configuring/
skau@, please take another look, thanks for your review! https://codereview.chromium.org/2760753002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_list_item.js (right): https://codereview.chromium.org/2760753002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list_item.js:255: onActivate_: function() { On 2017/03/29 20:17:11, skau wrote: > If we're not on ChromeOS, we don't need to check for resolution. We should > check for cr.isChromeOS and activate the destination if we're not. If we're not on Chrome OS, the check in line 596 in destination_search.js should fail and the configuration request should be rejected directly. But I agree with you that it's better to do a early return here. The original behavior on other OS doesn't change. https://codereview.chromium.org/2760753002/diff/40001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2760753002/diff/40001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:582: * allow configure more than one destination at the same time. On 2017/03/29 20:17:11, skau wrote: > nit: s/configure/configuring/ Done.
skau@chromium.org changed reviewers: + thestig@chromium.org
lgtm You'll need thestig@ for OWNERs approval.
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...
thestig@chromium.org changed reviewers: + dpapad@chromium.org
+dpapad for print preview CSS/JS. It looks like the new strings are included on non-ChromeOS desktop builds as well. Have we previously discussed this?
https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/cros_destination_resolver.html (left): https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/cros_destination_resolver.html:6: $i18n{resolveCrosPrinterMessage} Is this string used anywhere else? Can we remove it if not (https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/pr...). https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_list.js (right): https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list.js:235: }); find() returns undefined, if nothing is found, see documentation at [1]. The comment above is not accurate. return this.listItems_.find(function(listItem) { return listItem.destination.id == destinationId; }) || null; [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_list_item.html (right): https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list_item.html:19: <span>.</span><span>.</span><span>.</span> Can you check if this animation is running in the background, even when the span is hidden? If so, that would be bad for performance, and we should consider something else (either stopping the animation, or using a spinner that we start and stop). https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list_item.html:23: <span>$i18n{configuringFailedText}</span> Are both <span>s necessary? Why not <span class="configuring-failed-text" hidden> $i18n{configuringFailedText} </span> https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:583: * @param {Event} evt Contains the destination needs to be setup. !Event https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:586: onDestinationConfigureRequest_: function(evt) { |evt| is not an established abbreviation. Can you rename to |event| or just |e|? See https://google.github.io/styleguide/cppguide.html#General_Naming_Rules, from the C++ styleguide that states "do not abbreviate by deleting letters within a word". https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:588: ? this.localList_.getDestinationItem(evt.destination.id) It is way more common to put the binary operator in the previous line var destinationItem = evt.destination.isLocal ? this.localList_.getDestinationItem(evt.destination.id) : this.cloudList_.getDestinationItem(evt.destination.id); https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:590: if (!destinationItem) When can this happen? Please add a comment to explain, or if it should never happen, add an assertion instead of silently returning early. https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:627: * Called when the Promise is rejected. Nit: Maybe remove this comment? Does not add much extra information. The signature of then(a, b) already implies that b is called when the promise is rejected.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dpapad@, thetig@, I've addressed your comments. Please take another look, thanks for the review! https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/cros_destination_resolver.html (left): https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/cros_destination_resolver.html:6: $i18n{resolveCrosPrinterMessage} On 2017/03/30 20:56:23, dpapad wrote: > Is this string used anywhere else? Can we remove it if not > (https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/pr...). Thanks for the reminder! Removed it. https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_list.js (right): https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list.js:235: }); On 2017/03/30 20:56:23, dpapad wrote: > find() returns undefined, if nothing is found, see documentation at [1]. The > comment above is not accurate. > > return this.listItems_.find(function(listItem) { > return listItem.destination.id == destinationId; > }) || null; > > [1] > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... Thanks. Done. https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_list_item.html (right): https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list_item.html:19: <span>.</span><span>.</span><span>.</span> On 2017/03/30 20:56:23, dpapad wrote: > Can you check if this animation is running in the background, even when the span > is hidden? If so, that would be bad for performance, and we should consider > something else (either stopping the animation, or using a spinner that we start > and stop). Since it's an infinite animation, I think it's still running in the background. I manually start or stop the animation when the message gets shown or hidden. https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_list_item.html:23: <span>$i18n{configuringFailedText}</span> On 2017/03/30 20:56:23, dpapad wrote: > Are both <span>s necessary? Why not > > <span class="configuring-failed-text" hidden> > $i18n{configuringFailedText} > </span> Removed it. https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:583: * @param {Event} evt Contains the destination needs to be setup. On 2017/03/30 20:56:23, dpapad wrote: > !Event Done. https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:586: onDestinationConfigureRequest_: function(evt) { On 2017/03/30 20:56:23, dpapad wrote: > |evt| is not an established abbreviation. Can you rename to |event| or just |e|? > > See https://google.github.io/styleguide/cppguide.html#General_Naming_Rules, from > the C++ styleguide that states "do not abbreviate by deleting letters within a > word". Sure. Renamed it to event. https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:588: ? this.localList_.getDestinationItem(evt.destination.id) On 2017/03/30 20:56:24, dpapad wrote: > It is way more common to put the binary operator in the previous line > > var destinationItem = evt.destination.isLocal ? > this.localList_.getDestinationItem(evt.destination.id) : > this.cloudList_.getDestinationItem(evt.destination.id); Done. https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:590: if (!destinationItem) On 2017/03/30 20:56:24, dpapad wrote: > When can this happen? Please add a comment to explain, or if it should never > happen, add an assertion instead of silently returning early. I don't think it can happen. Modified to an assertion. https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:627: * Called when the Promise is rejected. On 2017/03/30 20:56:24, dpapad wrote: > Nit: Maybe remove this comment? Does not add much extra information. The > signature of then(a, b) already implies that b is called when the promise is > rejected. I would prefer to preserve this comment if the above comment is preserved. Or I can remove both comments here. What do you think?
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/2760753002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2760753002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:239: #if !defined(OS_CHROMEOS) Add the new strings here and change this to #if / #else ? |shortcut_text| should be in the #else section.
Comments are addressed. Please take another look, thanks! https://codereview.chromium.org/2760753002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2760753002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:239: #if !defined(OS_CHROMEOS) On 2017/03/30 23:26:02, Lei Zhang wrote: > Add the new strings here and change this to #if / #else ? |shortcut_text| should > be in the #else section. Done.
non CSS/HTML/JS bits lgtm https://codereview.chromium.org/2760753002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2760753002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:238: const base::string16 shortcut_text(base::UTF8ToUTF16(kBasicPrintShortcut)); Move this inside #else?
Thanks for the review! https://codereview.chromium.org/2760753002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2760753002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:238: const base::string16 shortcut_text(base::UTF8ToUTF16(kBasicPrintShortcut)); On 2017/03/30 23:37:56, Lei Zhang wrote: > Move this inside #else? Sorry! Missed this in your previous comment. 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2760753002/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/search/destination_search.js:627: * Called when the Promise is rejected. On 2017/03/30 at 23:18:44, xdai1 wrote: > On 2017/03/30 20:56:24, dpapad wrote: > > Nit: Maybe remove this comment? Does not add much extra information. The > > signature of then(a, b) already implies that b is called when the promise is > > rejected. > > I would prefer to preserve this comment if the above comment is preserved. Or I can remove both comments here. What do you think? I would prefer trimming the 1st comment to /** @param {!print_preview.PrinterSetupResponse} response */ and keeping it since it documents the response's type. The 2nd one as said above does not add any valuable info I think, but you are welcome to keep it if you think otherwise. https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/search/destination_list_item.js (right): https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_list_item.js:119: * @param {bool} shouldIgnore If true, which indicates another printer is s/bool/boolean. bool is not a valid type for JS Compiler. See details at https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the.... https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_list_item.js:123: if (!shouldIgnore) |shouldIgnore| naming makes the pubilc API of this method coupled with the implementation, unnecessarily. Can we rename as follows? shouldIgnore -> otherPrinterSetupInProgress Then this method will be if (!otherPrinterSetupInProgress) this.onDestinationActivated_(); This way, the public API does not leak how this method will handle the parameter. The public API simply states what information is expected to be passed in. Then you can also move the @param comment inside the method's body, since it is not interesting to the callers anymore. https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_list_item.js:255: if (show) { Lines 255-261 can be replaced by this.getChildElement('.configuring-text-jumping-dots').classList.toggle('jumping-dots', show); https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_list_item.js:279: configureEvt.destination = this.destination_; Can you create a CustomEvent instead of augmenting a standard Event instance? var configureEvent = new CustomEvevnt( 'DestinationListItem.EventType.CONFIGURE_REQUEST', {detail: {destination: this.destination_}}); Then in the receiving side /** @param {!CustomEvent} e */ myHandler: function(e) { // Use |e.detail.destination| } CustomEvent class (as implied by the name), is meant to be used for custom events, see https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent, and it is more appropriate than using Event. https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_search.js:571: * @param {Event} event Contains the search query. !Event https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_search.js:631: .onConfigureResolved({printerId: destination.id, success: false}); It's generally more robust to update internal state before sending out any notifications to observers/listeners. this.destinationInConfiguring_ = null; ...onConfigureResolved(...)
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...
dpapad@, I've addressed your comments. Please take another look, thanks for your review. https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/search/destination_list_item.js (right): https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_list_item.js:119: * @param {bool} shouldIgnore If true, which indicates another printer is On 2017/03/31 00:37:52, dpapad wrote: > s/bool/boolean. > > bool is not a valid type for JS Compiler. See details at > https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the.... Done. https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_list_item.js:123: if (!shouldIgnore) On 2017/03/31 00:37:52, dpapad wrote: > |shouldIgnore| naming makes the pubilc API of this method coupled with the > implementation, unnecessarily. Can we rename as follows? > > shouldIgnore -> otherPrinterSetupInProgress > > Then this method will be > > if (!otherPrinterSetupInProgress) > this.onDestinationActivated_(); > > This way, the public API does not leak how this method will handle the > parameter. The public API simply states what information is expected to be > passed in. Then you can also move the @param comment inside the method's body, > since it is not interesting to the callers anymore. Makes sense. Thanks! https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_list_item.js:255: if (show) { On 2017/03/31 00:37:52, dpapad wrote: > Lines 255-261 can be replaced by > > this.getChildElement('.configuring-text-jumping-dots').classList.toggle('jumping-dots', > show); Done. https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_list_item.js:279: configureEvt.destination = this.destination_; On 2017/03/31 00:37:52, dpapad wrote: > Can you create a CustomEvent instead of augmenting a standard Event instance? > > var configureEvent = new CustomEvevnt( > 'DestinationListItem.EventType.CONFIGURE_REQUEST', > {detail: {destination: this.destination_}}); > > Then in the receiving side > > /** @param {!CustomEvent} e */ > myHandler: function(e) { > // Use |e.detail.destination| > } > > CustomEvent class (as implied by the name), is meant to be used for custom > events, see https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent, and it > is more appropriate than using Event. Done. https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_search.js:571: * @param {Event} event Contains the search query. On 2017/03/31 00:37:52, dpapad wrote: > !Event Done. https://codereview.chromium.org/2760753002/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_search.js:631: .onConfigureResolved({printerId: destination.id, success: false}); On 2017/03/31 00:37:52, dpapad wrote: > It's generally more robust to update internal state before sending out any > notifications to observers/listeners. > > this.destinationInConfiguring_ = null; > ...onConfigureResolved(...) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
LGTM https://codereview.chromium.org/2760753002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/print_preview/search/destination_list_item.html (right): https://codereview.chromium.org/2760753002/diff/160001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_list_item.html:18: <span>$i18n{configuringInProgressText}</span> Nit: Can <span>$i18n{configuringInProgressText}</span> be replaced with $i18n{configuringInProgressText} (no wrapper)? https://codereview.chromium.org/2760753002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2760753002/diff/160001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_search.js:587: var destinationItem = event.detail.destination.isLocal ? Nit (optional): You can eliminate some redundancy as follows: var destination = event.detail.destination; var list = destination.isLocal ? this.localList_ : this.cloudList_; var item = list.getDestinationItem(destination.id); ...
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Thanks all for the review! https://codereview.chromium.org/2760753002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/print_preview/search/destination_list_item.html (right): https://codereview.chromium.org/2760753002/diff/160001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_list_item.html:18: <span>$i18n{configuringInProgressText}</span> On 2017/03/31 23:14:01, dpapad wrote: > Nit: Can <span>$i18n{configuringInProgressText}</span> be replaced with > $i18n{configuringInProgressText} (no wrapper)? Done. https://codereview.chromium.org/2760753002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/print_preview/search/destination_search.js (right): https://codereview.chromium.org/2760753002/diff/160001/chrome/browser/resourc... chrome/browser/resources/print_preview/search/destination_search.js:587: var destinationItem = event.detail.destination.isLocal ? On 2017/03/31 23:14:01, dpapad wrote: > Nit (optional): You can eliminate some redundancy as follows: > > var destination = event.detail.destination; > var list = destination.isLocal ? this.localList_ : this.cloudList_; > var item = list.getDestinationItem(destination.id); > ... Done.
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 skau@chromium.org, thestig@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2760753002/#ps180001 (title: "Address dpapad@'s comments. Fix failed test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491015643647700,
"parent_rev": "c4dab7a401b27b39bfc8798751462fda6f20a7db", "commit_rev":
"89ca6553d7d8d0303bfef17d17c20e0814b9ede2"}
Message was sent while issue was closed.
Description was changed from ========== [CUPS] Implement the local CUPS printer setup waiting UI. BUG=677567 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [CUPS] Implement the local CUPS printer setup waiting UI. BUG=677567 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2760753002 Cr-Commit-Position: refs/heads/master@{#461310} Committed: https://chromium.googlesource.com/chromium/src/+/89ca6553d7d8d0303bfef17d17c2... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/89ca6553d7d8d0303bfef17d17c2... |
