Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2250843002:
[CUPS] WebUI handler for CUPS printing: Retrieve the printer list from user preference. (Closed)
|
Created:
4 years, 4 months ago by xdai1 Modified:
4 years, 4 months 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[CUPS] WebUI handler for CUPS printing: Retrieve the printer list from user preference.
BUG=626752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/cd2cbd9440df5479f3e0f3e99b54c6788cd5f9d7
Cr-Commit-Position: refs/heads/master@{#413275}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address skau@ and dbeam@'s comments. #
Total comments: 4
Patch Set 3 : Call the URL parse directly instead of modifying GURL. #Patch Set 4 : Address skau@'s comments. #
Messages
Total messages: 27 (11 generated)
xdai@chromium.org changed reviewers: + skau@chromium.org
skau@, could you help review this CL please? Thanks!
https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:23: void ExtractProtocolAndIpAddress(const std::string& uri, why can't you use GURL?
https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:23: void ExtractProtocolAndIpAddress(const std::string& uri, On 2016/08/18 02:00:54, Dan Beam wrote: > why can't you use GURL? Yes. You definitely want to use GURL to break up the address. https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:52: return "Internet Printing Protocol (IPP)"; Don't these need to be translated? https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:106: for (; iter != printers.end(); ++iter) { This can be for(const std::unique_ptr<Printer>& printer : printers) then you don't need to mess with *iter. https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:108: new base::DictionaryValue); use base::MakeUnique<base::DictionaryValue>() https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:116: std::string uri = (*iter)->uri(); Per above, just turn this into a GURL. https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:120: printer_info->SetString("printerAddress", ip_address); Do we still have 'queue' in the UI? If so, you'll have to separate the path and authority. https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:138: std::unique_ptr<Printer> printer = base::MakeUnique<Printer>(printer_id); Are you handling add differently? RegisterPrinter expects to assign ids to new printers.
Description was changed from ========== [CUPS] WebUI handler for CUPS printing: Retrieve the printer list from user preference. BUG=626752 ========== to ========== [CUPS] WebUI handler for CUPS printing: Retrieve the printer list from user preference. BUG=626752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
xdai@chromium.org changed reviewers: + dbeam@chromium.org
skau@, dbeam@, please take another look at this CL, thanks! https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:23: void ExtractProtocolAndIpAddress(const std::string& uri, On 2016/08/18 05:27:39, skau wrote: > On 2016/08/18 02:00:54, Dan Beam wrote: > > why can't you use GURL? > > Yes. You definitely want to use GURL to break up the address. Since the printer URI's scheme is not standard, I have to modify the GRUL to make it parse correctly. https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:52: return "Internet Printing Protocol (IPP)"; On 2016/08/18 05:27:39, skau wrote: > Don't these need to be translated? I don't know... But it might need to be translated. So I will handle this logic in the js. https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:106: for (; iter != printers.end(); ++iter) { On 2016/08/18 05:27:39, skau wrote: > This can be for(const std::unique_ptr<Printer>& printer : printers) then you > don't need to mess with *iter. Done. https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:108: new base::DictionaryValue); On 2016/08/18 05:27:39, skau wrote: > use base::MakeUnique<base::DictionaryValue>() Done. https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:116: std::string uri = (*iter)->uri(); On 2016/08/18 05:27:39, skau wrote: > Per above, just turn this into a GURL. Done. https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:120: printer_info->SetString("printerAddress", ip_address); On 2016/08/18 05:27:39, skau wrote: > Do we still have 'queue' in the UI? If so, you'll have to separate the path and > authority. Yes you're right. Seems we'll need 'queue' information when manually setting up a new printer. However, I don't think I need to separate the path and authority since based on the URI format in https://www.cups.org/doc/network.html#PROTOCOLS, the authority part is after the scheme part and two slashes (//), while the path part is after the host part. https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:138: std::unique_ptr<Printer> printer = base::MakeUnique<Printer>(printer_id); On 2016/08/18 05:27:39, skau wrote: > Are you handling add differently? RegisterPrinter expects to assign ids to new > printers. I think adding printer should also be able to use RegisterPrinter() function. wdyt? For updating printer, the printer_id should have been created so I think it's fine to call RegisterPrinter() here.
lgtm but i didn't look that hard into the url/ changes
https://codereview.chromium.org/2250843002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js (right): https://codereview.chromium.org/2250843002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js:20: * printerQueue: string, is this code actually compiled? i swear @typedef doesn't work with trailing commas
also, do you have plans to use printerQueue soon? could you link to those?
skau@, dbeam@, I've addressed your comments/offline comments. Please take another look, thanks! https://codereview.chromium.org/2250843002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js (right): https://codereview.chromium.org/2250843002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js:20: * printerQueue: string, On 2016/08/18 21:17:29, Dan Beam wrote: > is this code actually compiled? i swear @typedef doesn't work with trailing > commas Oh I didn't know that. Modified as you suggested. However, it did compile before... And I just searched in the code base, it seems there are other places that use @typedef with trailing commas, like: https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin.... printerQueue will be used when manually setting up a CUPS printer, see mock here: https://docs.google.com/presentation/d/1FTAbefCL14zkYedIHGAyWhatuIIstUlLTYSCU.... I will work on it very soon.
Patchset #3 (id:40001) has been deleted
lgtm Do you know who owns/should approve the url_util changes? https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:23: void ExtractProtocolAndIpAddress(const std::string& uri, On 2016/08/18 21:12:18, xdai1 wrote: > On 2016/08/18 05:27:39, skau wrote: > > On 2016/08/18 02:00:54, Dan Beam wrote: > > > why can't you use GURL? > > > > Yes. You definitely want to use GURL to break up the address. > > Since the printer URI's scheme is not standard, I have to modify the GRUL to > make it parse correctly. Per offline discussion, we should probably use https://cs.chromium.org/chromium/src/url/third_party/mozilla/url_parse.h?l=22... rather than maintaining our own url parsing :) https://codereview.chromium.org/2250843002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (left): https://codereview.chromium.org/2250843002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:86: std::unique_ptr<base::DictionaryValue> response(new base::DictionaryValue()); base::MakeUnique...
On 2016/08/19 00:08:45, skau wrote: > lgtm > > Do you know who owns/should approve the url_util changes? > > https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... > File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): > > https://codereview.chromium.org/2250843002/diff/1/chrome/browser/ui/webui/set... > chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:23: void > ExtractProtocolAndIpAddress(const std::string& uri, > On 2016/08/18 21:12:18, xdai1 wrote: > > On 2016/08/18 05:27:39, skau wrote: > > > On 2016/08/18 02:00:54, Dan Beam wrote: > > > > why can't you use GURL? > > > > > > Yes. You definitely want to use GURL to break up the address. > > > > Since the printer URI's scheme is not standard, I have to modify the GRUL to > > make it parse correctly. > > Per offline discussion, we should probably use > https://cs.chromium.org/chromium/src/url/third_party/mozilla/url_parse.h?l=22... > rather than maintaining our own url parsing :) I guess you probably don't review the newest patch (patchset 3)? It's addressed in the new patch and there is no url_util changes any more. > > https://codereview.chromium.org/2250843002/diff/20001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (left): > > https://codereview.chromium.org/2250843002/diff/20001/chrome/browser/ui/webui... > chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:86: > std::unique_ptr<base::DictionaryValue> response(new base::DictionaryValue()); > base::MakeUnique... Thanks! Will address it in a new patch.
Comments addressed. Please take another look, thanks! skau@, per offline discussion, I used the third party URL parse function instead of modifying GURL, so there is no url_utls change. Let me know if it looks good to you. https://codereview.chromium.org/2250843002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (left): https://codereview.chromium.org/2250843002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:86: std::unique_ptr<base::DictionaryValue> response(new base::DictionaryValue()); On 2016/08/19 00:08:45, skau wrote: > base::MakeUnique... Done.
lgtm
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 dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2250843002/#ps80001 (title: "Address skau@'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.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [CUPS] WebUI handler for CUPS printing: Retrieve the printer list from user preference. BUG=626752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [CUPS] WebUI handler for CUPS printing: Retrieve the printer list from user preference. BUG=626752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/cd2cbd9440df5479f3e0f3e99b54c6788cd5f9d7 Cr-Commit-Position: refs/heads/master@{#413275} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cd2cbd9440df5479f3e0f3e99b54c6788cd5f9d7 Cr-Commit-Position: refs/heads/master@{#413275} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
