Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2232203003:
Implement a dbus client for CupsTool. (Closed)
|
Created:
4 years, 4 months ago by skau Modified:
4 years, 3 months ago Reviewers:
hashimoto CC:
chromium-reviews, hashimoto+watch_chromium.org, oshima+watch_chromium.org, adlr, xdai Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd CupsAdd and CupsRemove operations to debug daemon client.
lpadmin is being invoked over dbus using debugd on CrOS. This will facilitate Chrome calling lpadmin to administer printers.
BUG=616855
TEST=Compiles. Passes CQ. Verified with test integration w/ UI.
Committed: https://crrev.com/b70837e14e59c993291042f6e3603a5f3a6f9474
Cr-Commit-Position: refs/heads/master@{#417983}
Patch Set 1 #Patch Set 2 : Implement all the things #Patch Set 3 : functional implementation #Patch Set 4 : clean logging #Patch Set 5 : rebase #
Total comments: 20
Patch Set 6 : merge cups client into debug_daemon_client #Patch Set 7 : revert client bundle #
Total comments: 16
Patch Set 8 : rebase #Patch Set 9 : finish addressing comments #
Dependent Patchsets: Messages
Total messages: 32 (24 generated)
Description was changed from ========== Implement a dbus client for CupsTool. ========== to ========== Implement a dbus client for CupsTool. lpadmin is being invoked over dbus using debugd on CrOS. This will facilitate Chrome calling lpadmin to administer printers. BUG=616855 TEST=Covered by chromeos_unittests ==========
skau@chromium.org changed reviewers: + adlr@chromium.org, hashimoto@chromium.org, xdai@google.com
On 2016/09/06 17:34:21, skau wrote: > mailto:skau@chromium.org changed reviewers: > + mailto:adlr@chromium.org, mailto:hashimoto@chromium.org, mailto:xdai@google.com hashimoto: please review adlr, xdai: fyi
skau@chromium.org changed reviewers: + hashimoto@google.com - adlr@chromium.org, hashimoto@chromium.org, xdai@google.com
skau@chromium.org changed reviewers: + hashimoto@chromium.org - hashimoto@google.com
The CQ bit was checked by skau@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by skau@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.
Both methods belong to debugd. Please add methods to DebugDaemonClient, instead of adding a new client class. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... File chromeos/dbus/cups_client.cc (right): https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client.cc:21: const int kTimeoutMs = 1000; Is there any reason to avoid using ObjectProxy::TIMEOUT_USE_DEFAULT? If yes, please add a comment to describe the reason. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... File chromeos/dbus/cups_client.h (right): https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client.h:21: typedef base::Callback<void(bool success)> AddPrinterCallback; Please use using instead of typedef for new code. using AddPrinterCallback = base::Callback<void(bool success)>; https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client.h:24: typedef base::Callback<void(bool success)> RemovePrinterCallback; ditto. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client.h:52: // Create() shoudl be used instead nit: should https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client.h:57: }; nit: Please add a blank line. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... File chromeos/dbus/cups_client_unittest.cc (right): https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client_unittest.cc:230: TEST_F(CupsClientTest, AddPrinterCallsAdd) { Thank you for trying to add test code for this new D-Bus code, but we don't usually have test code for D-Bus clients as the test tends to be more complicated than the tested code as you've seen. Also, the functionality of a D-Bus client cannot be fully verified without running against the real D-Bus service process on a real Chrome OS device anyways. If you feel anxious about adding untested code, you can add more granular test instead (e.g. Move code appending values to MessageWriter into a function, and have a test of that function), but it's not mandatory. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... File chromeos/dbus/fake_cups_client.h (right): https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... chromeos/dbus/fake_cups_client.h:14: namespace chromeos { nit: Please add a blank line. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... chromeos/dbus/fake_cups_client.h:37: }; nit: Please add a blank line. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... File chromeos/dbus/fake_cups_client_unittest.cc (right): https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... chromeos/dbus/fake_cups_client_unittest.cc:18: if (called == nullptr || success == nullptr) Is this null check needed? If not, please remove it. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... chromeos/dbus/fake_cups_client_unittest.cc:26: if (!called) ditto.
The CQ bit was checked by skau@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...
Description was changed from ========== Implement a dbus client for CupsTool. lpadmin is being invoked over dbus using debugd on CrOS. This will facilitate Chrome calling lpadmin to administer printers. BUG=616855 TEST=Covered by chromeos_unittests ========== to ========== Implement a dbus client for CupsTool. lpadmin is being invoked over dbus using debugd on CrOS. This will facilitate Chrome calling lpadmin to administer printers. BUG=616855 TEST=Compiles. Passes CQ. Verified with followup CL. ==========
The CQ bit was checked by skau@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...
Thanks for the review. I merged the necessary code into debug_daemon_client. PTAL. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... File chromeos/dbus/cups_client.cc (right): https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client.cc:21: const int kTimeoutMs = 1000; On 2016/09/07 05:53:17, hashimoto wrote: > Is there any reason to avoid using ObjectProxy::TIMEOUT_USE_DEFAULT? > If yes, please add a comment to describe the reason. I wasn't sure that was the right thing. Fixed now. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... File chromeos/dbus/cups_client.h (right): https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client.h:21: typedef base::Callback<void(bool success)> AddPrinterCallback; On 2016/09/07 05:53:17, hashimoto wrote: > Please use using instead of typedef for new code. > > using AddPrinterCallback = base::Callback<void(bool success)>; Done. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client.h:24: typedef base::Callback<void(bool success)> RemovePrinterCallback; On 2016/09/07 05:53:17, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client.h:52: // Create() shoudl be used instead On 2016/09/07 05:53:17, hashimoto wrote: > nit: should Acknowledged. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client.h:57: }; On 2016/09/07 05:53:17, hashimoto wrote: > nit: Please add a blank line. Acknowledged. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... File chromeos/dbus/cups_client_unittest.cc (right): https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/cups_clie... chromeos/dbus/cups_client_unittest.cc:230: TEST_F(CupsClientTest, AddPrinterCallsAdd) { On 2016/09/07 05:53:17, hashimoto wrote: > Thank you for trying to add test code for this new D-Bus code, but we don't > usually have test code for D-Bus clients as the test tends to be more > complicated than the tested code as you've seen. > Also, the functionality of a D-Bus client cannot be fully verified without > running against the real D-Bus service process on a real Chrome OS device > anyways. > > If you feel anxious about adding untested code, you can add more granular test > instead (e.g. Move code appending values to MessageWriter into a function, and > have a test of that function), but it's not mandatory. Okay. We're going to cover this with an integration test down the line so I'm not too worried. I have a test harness to verify that it works. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... File chromeos/dbus/fake_cups_client.h (right): https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... chromeos/dbus/fake_cups_client.h:14: namespace chromeos { On 2016/09/07 05:53:17, hashimoto wrote: > nit: Please add a blank line. Acknowledged. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... chromeos/dbus/fake_cups_client.h:37: }; On 2016/09/07 05:53:17, hashimoto wrote: > nit: Please add a blank line. Acknowledged. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... File chromeos/dbus/fake_cups_client_unittest.cc (right): https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... chromeos/dbus/fake_cups_client_unittest.cc:18: if (called == nullptr || success == nullptr) On 2016/09/07 05:53:17, hashimoto wrote: > Is this null check needed? > If not, please remove it. Acknowledged. https://codereview.chromium.org/2232203003/diff/80001/chromeos/dbus/fake_cups... chromeos/dbus/fake_cups_client_unittest.cc:26: if (!called) On 2016/09/07 05:53:17, hashimoto wrote: > ditto. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Implement a dbus client for CupsTool. lpadmin is being invoked over dbus using debugd on CrOS. This will facilitate Chrome calling lpadmin to administer printers. BUG=616855 TEST=Compiles. Passes CQ. Verified with followup CL. ========== to ========== Add CupsAdd and CupsRemove operations to debug daemon client. lpadmin is being invoked over dbus using debugd on CrOS. This will facilitate Chrome calling lpadmin to administer printers. BUG=616855 TEST=Compiles. Passes CQ. Verified with test integration w/ UI. ==========
lgtm with requests https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... File chromeos/dbus/debug_daemon_client.cc (right): https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.cc:721: bool result; nit: bool result = false; Please don't leave this value uninitialized. https://google.github.io/styleguide/cppguide.html#Local_Variables https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.cc:733: bool result; ditto. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... File chromeos/dbus/debug_daemon_client.h (right): https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:212: // A callback to handle the result of AddPrinter. nit: "the result of CupsAddPrinter." https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:213: using AddPrinterCallback = base::Callback<void(bool success)>; nit: CupsAddPrinterCallback should be more intuitive? https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:218: // after method success, |error_callback| otherwise. nit: In "|callback| is called after method success", IIUC "success" means that the D-Bus method call is properly done (i.e. the method call is sent to the debugd, and the response for it is received). OTOH the "success" parameter of the callback means if any error happened in debugd when doing the actual printer-related work to respond to the D-Bus method call. Also, |error_callback| will be called if an error occurs at D-Bus protocol level, but it won't be called when an error occurs when debugd fails to do the actual printer-related work. Please make this distinction clear. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:226: // A callback to handle the result of RemovePrinter. ditto. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:227: using RemovePrinterCallback = base::Callback<void(bool success)>; ditto. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:230: // CUPS. |callback| is called on success, |error_callback| otherwise. ditto.
Thank for the review. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... File chromeos/dbus/debug_daemon_client.cc (right): https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.cc:721: bool result; On 2016/09/12 05:20:36, hashimoto wrote: > nit: bool result = false; > Please don't leave this value uninitialized. > https://google.github.io/styleguide/cppguide.html#Local_Variables Done. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.cc:733: bool result; On 2016/09/12 05:20:36, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... File chromeos/dbus/debug_daemon_client.h (right): https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:212: // A callback to handle the result of AddPrinter. On 2016/09/12 05:20:36, hashimoto wrote: > nit: "the result of CupsAddPrinter." Done. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:213: using AddPrinterCallback = base::Callback<void(bool success)>; On 2016/09/12 05:20:36, hashimoto wrote: > nit: CupsAddPrinterCallback should be more intuitive? Done. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:218: // after method success, |error_callback| otherwise. On 2016/09/12 05:20:36, hashimoto wrote: > nit: In "|callback| is called after method success", IIUC "success" means that > the D-Bus method call is properly done (i.e. the method call is sent to the > debugd, and the response for it is received). > OTOH the "success" parameter of the callback means if any error happened in > debugd when doing the actual printer-related work to respond to the D-Bus method > call. > > Also, |error_callback| will be called if an error occurs at D-Bus protocol > level, but it won't be called when an error occurs when debugd fails to do the > actual printer-related work. > > Please make this distinction clear. Done. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:226: // A callback to handle the result of RemovePrinter. On 2016/09/12 05:20:36, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:227: using RemovePrinterCallback = base::Callback<void(bool success)>; On 2016/09/12 05:20:36, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/2232203003/diff/120001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.h:230: // CUPS. |callback| is called on success, |error_callback| otherwise. On 2016/09/12 05:20:36, hashimoto wrote: > ditto. Done.
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2232203003/#ps160001 (title: "finish addressing 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 ========== Add CupsAdd and CupsRemove operations to debug daemon client. lpadmin is being invoked over dbus using debugd on CrOS. This will facilitate Chrome calling lpadmin to administer printers. BUG=616855 TEST=Compiles. Passes CQ. Verified with test integration w/ UI. ========== to ========== Add CupsAdd and CupsRemove operations to debug daemon client. lpadmin is being invoked over dbus using debugd on CrOS. This will facilitate Chrome calling lpadmin to administer printers. BUG=616855 TEST=Compiles. Passes CQ. Verified with test integration w/ UI. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add CupsAdd and CupsRemove operations to debug daemon client. lpadmin is being invoked over dbus using debugd on CrOS. This will facilitate Chrome calling lpadmin to administer printers. BUG=616855 TEST=Compiles. Passes CQ. Verified with test integration w/ UI. ========== to ========== Add CupsAdd and CupsRemove operations to debug daemon client. lpadmin is being invoked over dbus using debugd on CrOS. This will facilitate Chrome calling lpadmin to administer printers. BUG=616855 TEST=Compiles. Passes CQ. Verified with test integration w/ UI. Committed: https://crrev.com/b70837e14e59c993291042f6e3603a5f3a6f9474 Cr-Commit-Position: refs/heads/master@{#417983} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b70837e14e59c993291042f6e3603a5f3a6f9474 Cr-Commit-Position: refs/heads/master@{#417983} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
