Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2962413002:
Separate manufacturer and model fields from PPD info (Closed)
|
Created:
3 years, 5 months ago by skau Modified:
3 years, 5 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSeparate manufacturer and model fields from PPD info
Current representation of manufacturer and model impedes merger into a
single make_and_model field.
BUG=730242
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2962413002
Cr-Commit-Position: refs/heads/master@{#484808}
Committed: https://chromium.googlesource.com/chromium/src/+/30d0afecb4c669479620ccdb8e8d95b608040d83
Patch Set 1 #Patch Set 2 : clear the right field #
Total comments: 9
Patch Set 3 : alphabetize #Patch Set 4 : rebase #
Messages
Total messages: 37 (21 generated)
Description was changed from ========== Separate manufacturer and model fields from PPD info Current representation of manufacturer and model impedes merger into a single make_and_model field. BUG=730242 ========== to ========== Separate manufacturer and model fields from PPD info Current representation of manufacturer and model impedes merger into a single make_and_model field. BUG=730242 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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.
skau@chromium.org changed reviewers: + justincarlson@chromium.org, xdai@chromium.org
https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:47: printerModel: '', Since these two field printerManufacturer and printerModel are not used in js side any more, can we remove them? and only preserve them in c++ side for migration reason? https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:334: CHECK(printer_dict->GetString("printerManufacturer", &printer_manufacturer)); printer_manufacturer and printer_model won't have value any more after this change, right?
This is going to be a multi-stage migration. We'll need to keep the old fields around until M66 since we assume old clients will stick around for a while. go/bolton-make-model-migration https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:47: printerModel: '', On 2017/07/01 00:00:11, xdai1 wrote: > Since these two field printerManufacturer and printerModel are not used in js > side any more, can we remove them? and only preserve them in c++ side for > migration reason? They are still used for detected manufacturer and model information. It needs to pass through the javascript until the clients which don't need them go away :( https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:334: CHECK(printer_dict->GetString("printerManufacturer", &printer_manufacturer)); On 2017/07/01 00:00:11, xdai1 wrote: > printer_manufacturer and printer_model won't have value any more after this > change, right? We still need them since we'll have old clients floating around. go/bolton-make-model-migration
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
skau@chromium.org changed reviewers: + michaelpg@chromium.org
michealpg@: OWNER of chrome/browser/resources/settings/*, chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc
https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:47: printerModel: '', On 2017/07/01 00:11:49, skau wrote: > On 2017/07/01 00:00:11, xdai1 wrote: > > Since these two field printerManufacturer and printerModel are not used in js > > side any more, can we remove them? and only preserve them in c++ side for > > migration reason? > > They are still used for detected manufacturer and model information. It needs > to pass through the javascript until the clients which don't need them go away > :( I still don't quite understand. Can you elaborate? As far as I understand, these two old fields are preserved because you want to display data correctly across different Chrome versions, correct? So suppose we have two Chromebooks with two version Chrome, new A and old B. If we set up a new printer on new A, we need to make sure chromeos::Printer::manufacuter_ and chromeos::Printer::model_ fileds are properly populated so that old B can display the printer information correctly. And if we set up a new printer on old B, we need to make sure chromeos::Printer::make_and_model_ is populated so that new A can display the information correctly. But both the two cases don't seem require the preservation of the two old fields?
On 2017/07/05 23:16:26, xdai1 wrote: > https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js > (right): > > https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:47: > printerModel: '', > On 2017/07/01 00:11:49, skau wrote: > > On 2017/07/01 00:00:11, xdai1 wrote: > > > Since these two field printerManufacturer and printerModel are not used in > js > > > side any more, can we remove them? and only preserve them in c++ side for > > > migration reason? > > > > They are still used for detected manufacturer and model information. It needs > > to pass through the javascript until the clients which don't need them go away > > :( > > I still don't quite understand. Can you elaborate? As far as I understand, these > two old fields are preserved because you want to display data correctly across > different Chrome versions, correct? So suppose we have two Chromebooks with two > version Chrome, new A and old B. If we set up a new printer on new A, we need to > make sure chromeos::Printer::manufacuter_ and chromeos::Printer::model_ fileds > are properly populated so that old B can display the printer information > correctly. And if we set up a new printer on old B, we need to make sure > chromeos::Printer::make_and_model_ is populated so that new A can display the > information correctly. But both the two cases don't seem require the > preservation of the two old fields? Not quite. Since we can't modify the code on old B, we handle computing make_and_model in new A. For printers created in new A and used on old B, we need the old fields specifically for USB printers. For USB printers, we have manufacturer and model information as separate fields. This is saved into printerManufacturer and printerModel. Then, when we go to save the printer, we read printerManufacturer and printerModel into the Printer::manufacturer_ and ::model_ fields so we can persist them with Sync. If we did not do this, we'd have to try to parse make_and_model into manufacturer_ and model_ for USB printers, introducing error unnecessarily. When old B devices are no longer used, we'll remove the fields persisting the old information. The particular code path we're concerned with is usb_printer_util -> JS -> cups_printers_handler -> printers_manager.
On 2017/07/05 23:44:31, skau wrote: > > Not quite. Since we can't modify the code on old B, we handle computing > make_and_model in new A. For printers created in new A and used on old B, we > need the old fields specifically for USB printers. For USB printers, we have > manufacturer and model information as separate fields. This is saved into > printerManufacturer and printerModel. Then, when we go to save the printer, we > read printerManufacturer and printerModel into the Printer::manufacturer_ and > ::model_ fields so we can persist them with Sync. If we did not do this, we'd > have to try to parse make_and_model into manufacturer_ and model_ for USB > printers, introducing error unnecessarily. When old B devices are no longer > used, we'll remove the fields persisting the old information. The particular > code path we're concerned with is usb_printer_util -> JS -> > cups_printers_handler -> printers_manager. I see. Was not realized that printerManufacturer and printerModel are still used in onPrinterFound_() function. Thanks for the explanation. lgtm
The CQ bit was checked by skau@chromium.org
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...)
lgtm w/ nit https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:53: ppdManufacturer: '', optional nit: keep alphabetical https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js (right): https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js:23: * ppdManufacturer: string, optional nit: keep alphabetical
Thanks for the reviews. https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:53: ppdManufacturer: '', On 2017/07/06 20:10:43, michaelpg wrote: > optional nit: keep alphabetical Done. https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js (right): https://codereview.chromium.org/2962413002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_printers_browser_proxy.js:23: * ppdManufacturer: string, On 2017/07/06 20:10:43, michaelpg wrote: > optional nit: keep alphabetical Done.
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org, michaelpg@chromium.org, justincarlson@chromium.org Link to the patchset: https://codereview.chromium.org/2962413002/#ps40001 (title: "alphabetize")
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org, michaelpg@chromium.org, justincarlson@chromium.org Link to the patchset: https://codereview.chromium.org/2962413002/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499390554022010,
"parent_rev": "51da2c60872af869b2a2aeaa42952943fa3872ad", "commit_rev":
"bd325d6fa5cc434a4a56a8bdb8d7b6a444e5538b"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499390554022010,
"parent_rev": "b5c048ee41a40b38c74a47c81b299628dc832699", "commit_rev":
"30d0afecb4c669479620ccdb8e8d95b608040d83"}
Message was sent while issue was closed.
Description was changed from ========== Separate manufacturer and model fields from PPD info Current representation of manufacturer and model impedes merger into a single make_and_model field. BUG=730242 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Separate manufacturer and model fields from PPD info Current representation of manufacturer and model impedes merger into a single make_and_model field. BUG=730242 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2962413002 Cr-Commit-Position: refs/heads/master@{#484808} Committed: https://chromium.googlesource.com/chromium/src/+/30d0afecb4c669479620ccdb8e8d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/30d0afecb4c669479620ccdb8e8d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
