Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161933003:
Define printer configuration objects for Chrome OS. (Closed)
|
Created:
4 years, 5 months ago by skau Modified:
4 years, 4 months ago CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org, adlr, Lei Zhang, briannorris Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDefine printer configuration objects for Chrome OS.
Models will represent complete information required to setup a printer.
They will be stored in local storage until sync can be implemented with
the Chrome Sync team.
BUG=616853
TEST=Unit tests for all object translation code.
Committed: https://crrev.com/2b3975549595c92fa10c8a93b12d3aaefa2e9406
Cr-Commit-Position: refs/heads/master@{#409463}
Patch Set 1 #
Total comments: 2
Patch Set 2 : ready for review #
Total comments: 58
Patch Set 3 : fix #Patch Set 4 : \ #
Total comments: 7
Patch Set 5 : comments #
Total comments: 4
Patch Set 6 : Done #
Total comments: 10
Patch Set 7 : mend #Patch Set 8 : lint #
Total comments: 6
Patch Set 9 : polish #Patch Set 10 : Export symbols. #Patch Set 11 : export id constant #
Total comments: 1
Messages
Total messages: 50 (24 generated)
Description was changed from ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616855 TEST=Unit tests for all object translation code. Behavior verified on device with logs. ========== to ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616855 TEST=Unit tests for all object translation code. Behavior verified on device with logs. ==========
skau@chromium.org changed reviewers: + xdai@chromium.org
Please see the question below. Thanks! https://codereview.chromium.org/2161933003/diff/1/chrome/browser/chromeos/pri... File chrome/browser/chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2161933003/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_configuration.h:24: std::string id; Compared with PrinterBasicInfo (https://cs.chromium.org/chromium/src/printing/backend/print_backend.h?rcl=0&l=26), seems there are some items missing? e.g., printer_name, printer_description, printer_status(not sure if it should be put here though), is_default, and probably also is_enterprise_printer, ip_address(included in the URI?)?
Description was changed from ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616855 TEST=Unit tests for all object translation code. Behavior verified on device with logs. ========== to ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616855 TEST=Unit tests for all object translation code. ==========
skau@chromium.org changed reviewers: + sky@chromium.org, stevenjb@chromium.org
I'm defining preference objects for Chrome OS. Please ping me if you need more information (e.g. a PRD). https://codereview.chromium.org/2161933003/diff/1/chrome/browser/chromeos/pri... File chrome/browser/chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2161933003/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_configuration.h:24: std::string id; On 2016/07/19 19:09:43, xdai1 wrote: > Compared with PrinterBasicInfo > (https://cs.chromium.org/chromium/src/printing/backend/print_backend.h?rcl=0&l=26), > seems there are some items missing? e.g., printer_name, printer_description, > printer_status(not sure if it should be put here though), is_default, and > probably also is_enterprise_printer, ip_address(included in the URI?)? Description has been added. Status will come from a different API. We should consider if is_default should be stored separately. Enterprise printers will come from a different list. ip_address is part of the uri.
Oh hey, I'm not on the reviewers list. Then from the peanut gallery: - With multiple reviewers, please specify who reviews what. - Please wrap lines in CL descriptions at 72 chars when possible, as that is what goes in the git commit message. - You are adding a new c/b/chromeos/printing/ directory. Add some OWNERS.
I think this code can go in src/chromeos/printing. The only caveat is that such code must only depend on: base/ net/ dbus/ (code with dbus dependencies should be in chromeos/dbus). That way if we want to use this code in Ash instead of (or in addition to) Chrome, we do not need to move it. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_configuration.cc (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.cc:5: #include "chrome/browser/chromeos/printing/printer_configuration.h" Do we anticipate this having any src/chrome or src/content dependencies? If not, this could go in src/chromeos/printing/ https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:14: // Identifier from the quirks server. -1 otherwise. nit: Typically we only use a single space between sentences in comments. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:26: }; Can we embed this in the Printer class instead of in the global chromeos namespace? https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:30: Printer(); Do we need a default constructor? https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:75: // The manufacturer of the printer. e.g. HP punctuation nit: ', e.g. HP.' https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:78: // The model of the printer. e.g. OfficeJet 415 ditto https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:89: std::string uuid_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:26: using base::DictionaryValue; nit: This is fine, but generally not necessary. base::ListValue is used below without a typedef, so we should be consistent one way or the other. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:31: PrefService* user_state = profile->GetPrefs(); nit: local not necessary: 'return profile->GetPrefs()>GetList(prefs::kPrintingDevices)'. clang-format makes formatting long lines easy, so even if that were to cause wrapping, we generally avoid declaring locals that are used only once unless it clarifies the code. Here I would suggest that 'user_state' is less clear than 'profile->GetPrefs()'. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:35: DictionaryValue* FindMatchingPrinter(base::ListValue* values, const base::ListValue& https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:37: DictionaryValue* match = nullptr; Not needed, see below https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:39: for (size_t i = 0; i < size; ++i) { for (const auto& value : values) { DictionaryValue* printer_dictionary; if (!value->GetAsDictionary(&printer_dictionary)) continue; ... https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:48: match = printer_dictionary; return printer_dictionary; https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:53: return match; return nullptr; https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:60: base::ListValue* printer_list = update.Get(); DCHECK(printer_list) This documents that update.Get() should logically never return nullptr. (If that is not the case we need to handle a null result). https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:64: std::unique_ptr<base::Value>(std::move(printer_dictionary))); return; no else https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:88: for (size_t i = 0; i < size; ++i) { for (const auto& value : *values), etc. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:89: const DictionaryValue* printer_dictionary; = nullptr; https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:90: if (values->GetDictionary(i, &printer_dictionary)) { value->GetAsDictionary(&printer_dictionary); DCHECK(printer_dictionary); https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:95: NOTREACHED(); elim (use DCHECK mentioned above instead) https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:105: } nit: no {} around single line if statement (optional, but more common pattern in chrome) https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.h (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.h:24: static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry); static methods should be with other public methods (after the constructor/destructor, typically after non-static methods): https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.h:29: void RegisterPrinter(std::unique_ptr<Printer> printer); Document all public methods, including what is returned and any input parameters. (Exception: trivial getters/setters). https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_translator.cc (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_translator.cc:73: } invert logic and elim. else Also, should this be a LOG(ERROR)? https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_translator.h (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_translator.h:24: void PrinterToPref(const Printer& printer, base::DictionaryValue* pref); These should be in their own namespace, e.g. 'printing'.
Description was changed from ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616855 TEST=Unit tests for all object translation code. ========== to ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616855 TEST=Unit tests for all object translation code. ==========
Clarification: Everything except printer_pref_manager can go in src/chromeos. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:18: #include "chrome/common/pref_names.h" Note: This file will need to remain in chrome/browser/chromeos/printing/ since it depends on Profile.
Description was changed from ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616855 TEST=Unit tests for all object translation code. ========== to ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616855 TEST=Unit tests for all object translation code. ==========
Description was changed from ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616855 TEST=Unit tests for all object translation code. ========== to ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616853 TEST=Unit tests for all object translation code. ==========
Thanks for the review. PTAL. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_configuration.cc (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.cc:5: #include "chrome/browser/chromeos/printing/printer_configuration.h" On 2016/07/29 16:56:50, stevenjb wrote: > Do we anticipate this having any src/chrome or src/content dependencies? If not, > this could go in src/chromeos/printing/ Nope. This should be plain old data. It's moved. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:14: // Identifier from the quirks server. -1 otherwise. On 2016/07/29 16:56:50, stevenjb wrote: > nit: Typically we only use a single space between sentences in comments. Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:26: }; On 2016/07/29 16:56:50, stevenjb wrote: > Can we embed this in the Printer class instead of in the global chromeos > namespace? Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:30: Printer(); On 2016/07/29 16:56:50, stevenjb wrote: > Do we need a default constructor? I think it's clearer than constructing it with an empty string. But if you have strong objections, I can remove it. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:75: // The manufacturer of the printer. e.g. HP On 2016/07/29 16:56:50, stevenjb wrote: > punctuation nit: ', e.g. HP.' Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:78: // The model of the printer. e.g. OfficeJet 415 On 2016/07/29 16:56:50, stevenjb wrote: > ditto Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:89: std::string uuid_; On 2016/07/29 16:56:50, stevenjb wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:18: #include "chrome/common/pref_names.h" On 2016/07/29 16:59:26, stevenjb wrote: > Note: This file will need to remain in chrome/browser/chromeos/printing/ since > it depends on Profile. Acknowledged. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:26: using base::DictionaryValue; On 2016/07/29 16:56:50, stevenjb wrote: > nit: This is fine, but generally not necessary. base::ListValue is used below > without a typedef, so we should be consistent one way or the other. Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:31: PrefService* user_state = profile->GetPrefs(); On 2016/07/29 16:56:51, stevenjb wrote: > nit: local not necessary: 'return > profile->GetPrefs()>GetList(prefs::kPrintingDevices)'. > clang-format makes formatting long lines easy, so even if that were to cause > wrapping, we generally avoid declaring locals that are used only once unless it > clarifies the code. Here I would suggest that 'user_state' is less clear than > 'profile->GetPrefs()'. > Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:35: DictionaryValue* FindMatchingPrinter(base::ListValue* values, On 2016/07/29 16:56:51, stevenjb wrote: > const base::ListValue& Thanks... Why can I get a mutable DictionaryValue from a const ListValue? The const version of GetAsDictionary is supposed to point to a const Dictionary value. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:37: DictionaryValue* match = nullptr; On 2016/07/29 16:56:51, stevenjb wrote: > Not needed, see below Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:39: for (size_t i = 0; i < size; ++i) { On 2016/07/29 16:56:50, stevenjb wrote: > for (const auto& value : values) { > DictionaryValue* printer_dictionary; > if (!value->GetAsDictionary(&printer_dictionary)) > continue; > ... Thanks. I didn't notice that ListValue implemented begin and end. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:48: match = printer_dictionary; On 2016/07/29 16:56:51, stevenjb wrote: > return printer_dictionary; Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:53: return match; On 2016/07/29 16:56:50, stevenjb wrote: > return nullptr; Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:60: base::ListValue* printer_list = update.Get(); On 2016/07/29 16:56:50, stevenjb wrote: > DCHECK(printer_list) > > This documents that update.Get() should logically never return nullptr. (If that > is not the case we need to handle a null result). Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:64: std::unique_ptr<base::Value>(std::move(printer_dictionary))); On 2016/07/29 16:56:51, stevenjb wrote: > return; > no else > Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:88: for (size_t i = 0; i < size; ++i) { On 2016/07/29 16:56:51, stevenjb wrote: > for (const auto& value : *values), etc. Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:89: const DictionaryValue* printer_dictionary; On 2016/07/29 16:56:50, stevenjb wrote: > = nullptr; Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:90: if (values->GetDictionary(i, &printer_dictionary)) { On 2016/07/29 16:56:51, stevenjb wrote: > value->GetAsDictionary(&printer_dictionary); > DCHECK(printer_dictionary); Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:95: NOTREACHED(); On 2016/07/29 16:56:51, stevenjb wrote: > elim (use DCHECK mentioned above instead) Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:105: } On 2016/07/29 16:56:50, stevenjb wrote: > nit: no {} around single line if statement (optional, but more common pattern in > chrome) > Righto. I've been writing too much Java. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.h (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.h:24: static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry); On 2016/07/29 16:56:51, stevenjb wrote: > static methods should be with other public methods (after the > constructor/destructor, typically after non-static methods): > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.h:29: void RegisterPrinter(std::unique_ptr<Printer> printer); On 2016/07/29 16:56:51, stevenjb wrote: > Document all public methods, including what is returned and any input > parameters. (Exception: trivial getters/setters). > > Done. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_translator.cc (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_translator.cc:73: } On 2016/07/29 16:56:51, stevenjb wrote: > invert logic and elim. else > Also, should this be a LOG(ERROR)? It's an unexpected condition that we don't want to crash for. Is there some guidance for this? I would say maybe a warning? https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_translator.h (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_translator.h:24: void PrinterToPref(const Printer& printer, base::DictionaryValue* pref); On 2016/07/29 16:56:51, stevenjb wrote: > These should be in their own namespace, e.g. 'printing'. Just this or the Printer class too?
https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:30: Printer(); On 2016/08/01 19:37:19, skau wrote: > On 2016/07/29 16:56:50, stevenjb wrote: > > Do we need a default constructor? > > I think it's clearer than constructing it with an empty string. But if you have > strong objections, I can remove it. Generally we try to avoid multiple constructors since it tends to result in duplicated code that needs to be maintained, but if there is an active use case where passing "" is awkward we can keep it. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:35: DictionaryValue* FindMatchingPrinter(base::ListValue* values, On 2016/08/01 19:37:19, skau wrote: > On 2016/07/29 16:56:51, stevenjb wrote: > > const base::ListValue& > > Thanks... Why can I get a mutable DictionaryValue from a const ListValue? The > const version of GetAsDictionary is supposed to point to a const Dictionary > value. Good question, I don't know. Anyway, my apologies, I didn't notice this was returning (and modifying) a non-const *. The way you had this before was correct. Maybe add a comment, or rename the method (e.g. FindPrinterPref) to make that more clear. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_translator.cc (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_translator.cc:73: } On 2016/08/01 19:37:21, skau wrote: > On 2016/07/29 16:56:51, stevenjb wrote: > > invert logic and elim. else > > Also, should this be a LOG(ERROR)? > > It's an unexpected condition that we don't want to crash for. Is there some > guidance for this? I would say maybe a warning? Logging guide line is here: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... CHECK/etc is here: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... I would suggest that an unexpected condition should use LOG(ERROR) or LOG(WARNING), depending on how likely it is to cause other problems (LOG(ERROR) is more likely to stand out in the logs). https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_translator.h (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_translator.h:24: void PrinterToPref(const Printer& printer, base::DictionaryValue* pref); On 2016/08/01 19:37:21, skau wrote: > On 2016/07/29 16:56:51, stevenjb wrote: > > These should be in their own namespace, e.g. 'printing'. > Just this or the Printer class too? Classes are a namespace unto themselves, so putting them inside a 'printing' namespace is optional, but globals variables and functions, which could conceivably have meaning in another context (e.g. a chromeos settings class), should be more specifically namespaced. https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/OWNERS (right): https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/OWNERS:1: file://chromeos/printing/OWNERS You should add yourself as an owner. https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:34: for (const auto& value : values) { This should still work with *values and without the const. https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.h (right): https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.h:32: // Add or update a printer. Printers are identified by the id field. Use an nit: Adds or Updates
Thanks again. PTAL. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_configuration.h:30: Printer(); On 2016/08/01 20:48:16, stevenjb wrote: > On 2016/08/01 19:37:19, skau wrote: > > On 2016/07/29 16:56:50, stevenjb wrote: > > > Do we need a default constructor? > > > > I think it's clearer than constructing it with an empty string. But if you > have > > strong objections, I can remove it. > > Generally we try to avoid multiple constructors since it tends to result in > duplicated code that needs to be maintained, but if there is an active use case > where passing "" is awkward we can keep it. The use cases are different so I'm going to leave it. Alternatively, I could roll a builder but I felt that might be overkill. https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2161933003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:35: DictionaryValue* FindMatchingPrinter(base::ListValue* values, On 2016/08/01 20:48:16, stevenjb wrote: > On 2016/08/01 19:37:19, skau wrote: > > On 2016/07/29 16:56:51, stevenjb wrote: > > > const base::ListValue& > > > > Thanks... Why can I get a mutable DictionaryValue from a const ListValue? > The > > const version of GetAsDictionary is supposed to point to a const Dictionary > > value. > > Good question, I don't know. Anyway, my apologies, I didn't notice this was > returning (and modifying) a non-const *. The way you had this before was > correct. Maybe add a comment, or rename the method (e.g. FindPrinterPref) to > make that more clear. It looks like values behaves like a const collection where I can retrieve references to elements and modify them. I can remove the const if it's confusing. I've renamed the function and added a comment, I hope that's clearer. https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/OWNERS (right): https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/OWNERS:1: file://chromeos/printing/OWNERS On 2016/08/01 20:48:16, stevenjb wrote: > You should add yourself as an owner. I just added that file and my name is already in there :) https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:34: for (const auto& value : values) { On 2016/08/01 20:48:16, stevenjb wrote: > This should still work with *values and without the const. See comment on earlier revision. This works as well. https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.h (right): https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.h:32: // Add or update a printer. Printers are identified by the id field. Use an On 2016/08/01 20:48:16, stevenjb wrote: > nit: Adds or Updates Done.
lgtm with the suggested change. https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/OWNERS (right): https://codereview.chromium.org/2161933003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/OWNERS:1: file://chromeos/printing/OWNERS On 2016/08/01 21:10:10, skau wrote: > On 2016/08/01 20:48:16, stevenjb wrote: > > You should add yourself as an owner. > > I just added that file and my name is already in there :) Oh, right, this is a reference to another file. Nevermind :) https://codereview.chromium.org/2161933003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2161933003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:34: base::DictionaryValue* FindPrinterPref(const base::ListValue& values, Since we are returning a modifiable pointer to a member of |values|, I think it is more clear to make |values| a mutable parameter, even though the list itself is not getting modified. https://codereview.chromium.org/2161933003/diff/80001/chromeos/printing/OWNERS File chromeos/printing/OWNERS (right): https://codereview.chromium.org/2161933003/diff/80001/chromeos/printing/OWNER... chromeos/printing/OWNERS:3: thestig@chromium.org nit: alphabetize
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...
xdai@ can you lgtm if you don't have comments? sky@ can you take a look at *.gypi and browser_prefs.cc? https://codereview.chromium.org/2161933003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2161933003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:34: base::DictionaryValue* FindPrinterPref(const base::ListValue& values, On 2016/08/01 21:19:14, stevenjb wrote: > Since we are returning a modifiable pointer to a member of |values|, I think it > is more clear to make |values| a mutable parameter, even though the list itself > is not getting modified. Done. https://codereview.chromium.org/2161933003/diff/80001/chromeos/printing/OWNERS File chromeos/printing/OWNERS (right): https://codereview.chromium.org/2161933003/diff/80001/chromeos/printing/OWNER... chromeos/printing/OWNERS:3: thestig@chromium.org On 2016/08/01 21:19:15, stevenjb wrote: > nit: alphabetize Done.
chromeos/printing/printer_configuration.h && .cc lgtm! I thought I weren't a reviewer:)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
thestig@chromium.org changed reviewers: + thestig@chromium.org
Try bots are red... https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... File chromeos/printing/printer_translator.cc (right): https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.cc:47: value->GetInteger(kVersionNumber, &version_number); Check return value like you do above and below? https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.cc:55: void PPDFileToDictionary(const Printer::PPDFile& ppd, Same comment about returning a std::unique_ptr<base::DictionaryValue>. https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.cc:127: dictionary->Set(kPPD, std::unique_ptr<base::Value>(std::move(ppd))); Does this work? dictionary->Set(kPPD, std::move(ppd)); https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... File chromeos/printing/printer_translator.h (right): https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.h:21: bool MergePrinterPreference(const base::DictionaryValue& pref, Same as below. https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.h:25: void PrinterToPref(const Printer& printer, base::DictionaryValue* pref); Are there any situations where you actually want to update an existing |pref|? Can this be: std::unique_ptr<DictionaryValue> PrinterToPref(const Printer& printer) ?
LGTM
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...
Comments addressed. Thanks for all the reviews. https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... File chromeos/printing/printer_translator.cc (right): https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.cc:47: value->GetInteger(kVersionNumber, &version_number); On 2016/08/02 02:35:52, Lei Zhang wrote: > Check return value like you do above and below? Done. https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.cc:55: void PPDFileToDictionary(const Printer::PPDFile& ppd, On 2016/08/02 02:35:52, Lei Zhang wrote: > Same comment about returning a std::unique_ptr<base::DictionaryValue>. Done. https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.cc:127: dictionary->Set(kPPD, std::unique_ptr<base::Value>(std::move(ppd))); On 2016/08/02 02:35:52, Lei Zhang wrote: > Does this work? > > dictionary->Set(kPPD, std::move(ppd)); That does work. Neat. https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... File chromeos/printing/printer_translator.h (right): https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.h:21: bool MergePrinterPreference(const base::DictionaryValue& pref, On 2016/08/02 02:35:53, Lei Zhang wrote: > Same as below. There could be instances where we would want to apply a partial update but I don't have a good use case in mind right now. I'll change it. https://codereview.chromium.org/2161933003/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.h:25: void PrinterToPref(const Printer& printer, base::DictionaryValue* pref); On 2016/08/02 02:35:52, Lei Zhang wrote: > Are there any situations where you actually want to update an existing |pref|? > Can this be: std::unique_ptr<DictionaryValue> PrinterToPref(const Printer& > printer) ? This should return a new dictionary. In the case where we update an existing record, we could save the allocation of a new dictionary but that might be a premature optimization.
lgtm https://codereview.chromium.org/2161933003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2161933003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_pref_manager.cc:60: std::unique_ptr<base::Value>(std::move(printer_dictionary))); Do you need the "std::unique_ptr<base::Value>(" bit here? https://codereview.chromium.org/2161933003/diff/140001/chromeos/printing/prin... File chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2161933003/diff/140001/chromeos/printing/prin... chromeos/printing/printer_configuration.h:10: #include <utility> Probably not needed here. https://codereview.chromium.org/2161933003/diff/140001/chromeos/printing/prin... File chromeos/printing/printer_translator.h (right): https://codereview.chromium.org/2161933003/diff/140001/chromeos/printing/prin... chromeos/printing/printer_translator.h:25: // Returns a dictionary genterated from the |printer| fields. Only gentlemen genterates? (typo)
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, xdai@chromium.org, sky@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2161933003/#ps160001 (title: "polish")
https://codereview.chromium.org/2161933003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2161933003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_pref_manager.cc:60: std::unique_ptr<base::Value>(std::move(printer_dictionary))); On 2016/08/02 20:52:41, Lei Zhang wrote: > Do you need the "std::unique_ptr<base::Value>(" bit here? Done. https://codereview.chromium.org/2161933003/diff/140001/chromeos/printing/prin... File chromeos/printing/printer_configuration.h (right): https://codereview.chromium.org/2161933003/diff/140001/chromeos/printing/prin... chromeos/printing/printer_configuration.h:10: #include <utility> On 2016/08/02 20:52:41, Lei Zhang wrote: > Probably not needed here. Done. https://codereview.chromium.org/2161933003/diff/140001/chromeos/printing/prin... File chromeos/printing/printer_translator.h (right): https://codereview.chromium.org/2161933003/diff/140001/chromeos/printing/prin... chromeos/printing/printer_translator.h:25: // Returns a dictionary genterated from the |printer| fields. On 2016/08/02 20:52:41, Lei Zhang wrote: > Only gentlemen genterates? (typo) lol. whoops.
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: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, stevenjb@chromium.org, xdai@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2161933003/#ps180001 (title: "Export symbols.")
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: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, stevenjb@chromium.org, xdai@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2161933003/#ps200001 (title: "export id constant")
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 ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616853 TEST=Unit tests for all object translation code. ========== to ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616853 TEST=Unit tests for all object translation code. ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616853 TEST=Unit tests for all object translation code. ========== to ========== Define printer configuration objects for Chrome OS. Models will represent complete information required to setup a printer. They will be stored in local storage until sync can be implemented with the Chrome Sync team. BUG=616853 TEST=Unit tests for all object translation code. Committed: https://crrev.com/2b3975549595c92fa10c8a93b12d3aaefa2e9406 Cr-Commit-Position: refs/heads/master@{#409463} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/2b3975549595c92fa10c8a93b12d3aaefa2e9406 Cr-Commit-Position: refs/heads/master@{#409463}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2207053002/ by aboxhall@chromium.org. The reason for reverting is: This appears to be causing ASAN test failures: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom....
Message was sent while issue was closed.
https://codereview.chromium.org/2161933003/diff/200001/chromeos/printing/prin... File chromeos/printing/printer_configuration.cc (right): https://codereview.chromium.org/2161933003/diff/200001/chromeos/printing/prin... chromeos/printing/printer_configuration.cc:18: ppd_ = *(ppd.release()); Ya, probably a bad release() here. Should be a .get() instead, and let |ppd| hold on to the raw pointer.
Message was sent while issue was closed.
On 2016/08/03 16:28:06, Lei Zhang wrote: > https://codereview.chromium.org/2161933003/diff/200001/chromeos/printing/prin... > File chromeos/printing/printer_configuration.cc (right): > > https://codereview.chromium.org/2161933003/diff/200001/chromeos/printing/prin... > chromeos/printing/printer_configuration.cc:18: ppd_ = *(ppd.release()); > Ya, probably a bad release() here. Should be a .get() instead, and let |ppd| > hold on to the raw pointer. Yep. release is wrong. I thought it would get moved since it was an rvalue but that doesn't make much sense. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
