Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2468063003:
Initial hookup of PpdProvider. (Closed)
|
Created:
4 years, 1 month ago by Carlson Modified:
4 years, 1 month ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial hookup of PpdProvider.
Adds machinery for creating the provider, and APIs for getting available
manufacturer/models. These still need to be hooked up on the javascript
side.
BUG=617254
Committed: https://crrev.com/f256972e7229d0d1827392bd7d03bd71971ddb52
Cr-Commit-Position: refs/heads/master@{#429780}
Patch Set 1 #Patch Set 2 : Fix callback name bug. #
Total comments: 6
Patch Set 3 : Convert to ResolveJavascriptCallback, address xdai comments. #
Total comments: 4
Patch Set 4 : Address skau comments #Patch Set 5 : Remove unused variable #
Total comments: 2
Patch Set 6 : string -> const string& #
Messages
Total messages: 44 (20 generated)
Description was changed from ========== Initial hookup of PpdProvider. Adds machinery for creating the provider, and APIs for getting available manufacturer/models. These still need to be hooked up on the javascript side. BUG=617254 ========== to ========== Initial hookup of PpdProvider. Adds machinery for creating the provider, and APIs for getting available manufacturer/models. These still need to be hooked up on the javascript side. BUG=617254 ==========
justincarlson@chromium.org changed reviewers: + skau@chromium.org, xdai@chromium.org
Daisy, Sean, can you look this over for general sanity? It's the first steps to plumbing in PpdProvider. Daisy, in particular, I've made some assumptions about how this hooks up to the UI that I'd like your feedback on. Also I'd really like to hear suggestions on how to test this, if you have any, as there's no real test coverage here AFAICT.
On 2016/11/02 20:26:31, Carlson wrote: > Daisy, Sean, can you look this over for general sanity? It's the first steps to > plumbing in PpdProvider. > > Daisy, in particular, I've made some assumptions about how this hooks up to the > UI that I'd like your feedback on. > > Also I'd really like to hear suggestions on how to test this, if you have any, > as there's no real test coverage here AFAICT. Generally it looks good. As an alternative, you might not need all these four webui listener callback function if you use cr.sendWithPromise() (js side) and WebUIMessageHandler::ResolveJavascriptCallback() (C++ side) in pair. I'd prefer the latter approach since it's conciser and simpler. But I don't have strong objections if you want to go with the current approach. For test, sorry I don't know how to test a webui handler function...
oops, forgot to send the review comments. https://codereview.chromium.org/2468063003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right): https://codereview.chromium.org/2468063003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:57: // result, or on-manufacturer-list-error with no arguments on error. nit: 'on-manufacturer-list-error' https://codereview.chromium.org/2468063003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:62: // invokes nit: no line break here. https://codereview.chromium.org/2468063003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:64: // result, or on-model-list-error with no arguments on error. nit: 'on-model-list-error'
Thanks for the ResolveJavascriptCallback pointer, converted to use that. https://codereview.chromium.org/2468063003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right): https://codereview.chromium.org/2468063003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:57: // result, or on-manufacturer-list-error with no arguments on error. On 2016/11/02 21:52:32, xdai1 wrote: > nit: 'on-manufacturer-list-error' Removed https://codereview.chromium.org/2468063003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:62: // invokes On 2016/11/02 21:52:32, xdai1 wrote: > nit: no line break here. Done. https://codereview.chromium.org/2468063003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:64: // result, or on-model-list-error with no arguments on error. On 2016/11/02 21:52:32, xdai1 wrote: > nit: 'on-model-list-error' Removed
lgtm
In order to test this, we can write browser tests but it's difficult to do anything less granular than that. Do you have a follow up CL to add the javascript events? lgtm https://codereview.chromium.org/2468063003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2468063003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:252: base::Unretained(this), js_callback)); Get the weakptr for CupsPrintersHandler in case we collect this class while the call is outstanding. https://codereview.chromium.org/2468063003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:264: base::Unretained(this), js_callback, manufacturer)); ditto
I don't think I need to do js in this change, it can be added separately. Much as I dislike js, I'm game to take a look unless Daisy wants to pick up that piece. -J
https://codereview.chromium.org/2468063003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2468063003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:252: base::Unretained(this), js_callback)); On 2016/11/02 23:20:13, skau wrote: > Get the weakptr for CupsPrintersHandler in case we collect this class while the > call is outstanding. Done. https://codereview.chromium.org/2468063003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:264: base::Unretained(this), js_callback, manufacturer)); On 2016/11/02 23:20:13, skau wrote: > ditto Done.
On 2016/11/02 23:27:44, Carlson wrote: > I don't think I need to do js in this change, it can be added separately. Much > as I dislike js, I'm game to take a look unless Daisy wants to pick up that > piece. > > -J Np. I'll try to do it before I leave for vacation next week.
On 2016/11/02 23:27:44, Carlson wrote: > I don't think I need to do js in this change, it can be added separately. Much > as I dislike js, I'm game to take a look unless Daisy wants to pick up that > piece. > > -J That's cool. Polymer takes a bit getting used to as well. Also, I missed the note in the description. :P lgtm
The CQ bit was checked by justincarlson@google.com 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: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by justincarlson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org, skau@chromium.org Link to the patchset: https://codereview.chromium.org/2468063003/#ps80001 (title: "Remove unused variable")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/11/03 21:11:40, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) I removed an extraneous variable, could one of you re-lgtm this? Thanks, -J
On 2016/11/03 21:28:12, Carlson wrote: > On 2016/11/03 21:11:40, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > I removed an extraneous variable, could one of you re-lgtm this? > > Thanks, > -J The error message says “ Missing LGTM from an OWNER for these files: chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h chrome/common/chrome_paths.cc chrome/common/chrome_paths.h ” Both of us are not owners of these files. You should find an owner of these files to review your CL.
On 2016/11/03 21:29:44, xdai1 wrote: > On 2016/11/03 21:28:12, Carlson wrote: > > On 2016/11/03 21:11:40, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > I removed an extraneous variable, could one of you re-lgtm this? > > > > Thanks, > > -J > > The error message says “ > Missing LGTM from an OWNER for these files: > chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc > chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h > chrome/common/chrome_paths.cc > chrome/common/chrome_paths.h ” > > Both of us are not owners of these files. You should find an owner of these > files to review your CL. Tip (you might already have known): Try run "git cl owner" to find an owner.
On 2016/11/03 21:31:27, xdai1 wrote: > On 2016/11/03 21:29:44, xdai1 wrote: > > On 2016/11/03 21:28:12, Carlson wrote: > > > On 2016/11/03 21:11:40, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > > > I removed an extraneous variable, could one of you re-lgtm this? > > > > > > Thanks, > > > -J > > > > The error message says “ > > Missing LGTM from an OWNER for these files: > > chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc > > chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h > > chrome/common/chrome_paths.cc > > chrome/common/chrome_paths.h ” > > > > Both of us are not owners of these files. You should find an owner of these > > files to review your CL. > > Tip (you might already have known): Try run "git cl owner" to find an owner. Oh, shoot, I was thinking ChromeOS. Thanks.
justincarlson@chromium.org changed reviewers: + thestig@chromium.org
Lei, are you good for reviewing this? You know the library better than anyone else that can sign off on it, I think.
lgtm https://codereview.chromium.org/2468063003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right): https://codereview.chromium.org/2468063003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:70: std::string js_callback, Pass by const ref? Also line 74 and 75.
Thanks for the quick review. https://codereview.chromium.org/2468063003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h (right): https://codereview.chromium.org/2468063003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h:70: std::string js_callback, On 2016/11/03 22:17:47, Lei Zhang wrote: > Pass by const ref? Also line 74 and 75. Done. (Wasn't sure if this was safe to do for Bind() bound arguments.)
The CQ bit was checked by justincarlson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, xdai@chromium.org, skau@chromium.org Link to the patchset: https://codereview.chromium.org/2468063003/#ps100001 (title: "string -> const string&")
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
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_...)
The CQ bit was checked by justincarlson@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 justincarlson@chromium.org
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 ========== Initial hookup of PpdProvider. Adds machinery for creating the provider, and APIs for getting available manufacturer/models. These still need to be hooked up on the javascript side. BUG=617254 ========== to ========== Initial hookup of PpdProvider. Adds machinery for creating the provider, and APIs for getting available manufacturer/models. These still need to be hooked up on the javascript side. BUG=617254 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Initial hookup of PpdProvider. Adds machinery for creating the provider, and APIs for getting available manufacturer/models. These still need to be hooked up on the javascript side. BUG=617254 ========== to ========== Initial hookup of PpdProvider. Adds machinery for creating the provider, and APIs for getting available manufacturer/models. These still need to be hooked up on the javascript side. BUG=617254 Committed: https://crrev.com/f256972e7229d0d1827392bd7d03bd71971ddb52 Cr-Commit-Position: refs/heads/master@{#429780} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f256972e7229d0d1827392bd7d03bd71971ddb52 Cr-Commit-Position: refs/heads/master@{#429780} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
