printer_handler.h: AddedPrintersCallback's list of printers will have "provisional" dictionary key of type int, not bool |
||||
Issue description
ref:
chrome/browser/ui/webui/print_preview/printer_handler.h
using AddedPrintersCallback =
base::RepeatingCallback<void(const base::ListValue& printers)>;
And from https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/extension_printer_handler.cc?rcl=19a69a1389844f51c474efa446e3f22dd1fb36a8&l=362
printer_list.Append(
DictionaryBuilder()
.Set("id", GenerateProvisionalUsbPrinterId(extension.get(),
device.get()))
....
.Set("provisional", true) <--------------------------- PROBLEM?
.Build());
The issue is that DictionaryValue::Set(somepath, bool) would call overloaded
DictionaryValue::Set(somepath, int) [1]
So printer.provisional would be set to 1. I think this is *likely* not a problem (well it isn't, because things work I suppose). But filing the bug for keeping track.
Suggest what should be done and assign the bug back to me:
1. Keep this as int, play safe, but be clear and call Set(static_cast<int>(true)) or similar.
2. Change this to bool, but ensure that the consumer of this list works with bool.
[1]
There's a DictionaryValue::SetBoolean to set boolean, I discovered this issue while trying to unify SetBoolean and Set.
,
Apr 12 2018
I believe this is only used in one place where it is simply checked for truthiness and so could easily be replaced with a true boolean: https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/data/local_parsers.js?type=cs&l=100
,
Apr 12 2018
Thanks for the info, will change it to bool.
,
Apr 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/723fb47254fc5f49bda577df447303dc3554beb9 commit 723fb47254fc5f49bda577df447303dc3554beb9 Author: Istiaque Ahmed <lazyboy@chromium.org> Date: Thu Apr 12 14:58:53 2018 Disallow calling value builders' Set/Append(??, bool) methods. Currently calling DictionaryValue::Set(??, bool) and ListValue::Append(bool) would result in calling int variant from the overloaded methods(!). This is confusing and can be a source of bugs. (The current suggested way is to explicitly call SetBoolean and AppendBoolean). There is only once place where this was happening, in extension_printer_handler.cc. Fix that code to use boolean variant, by using SetBoolean() method. This CL will also let us overload the bool variant in future. This CL - Provides an explicit "const char*" param overload. - Declares a bool overload but makes it inaccessible (= delete). This will - Make sure attempting to call DictionaryValue::Set with bool will fail compile (call to deleted member function 'Set'). So accidental introduction of such bugs won't be possible. - The char* variant will make sure we don't attempt to resolve bool to this pointer variant. - Give us the ability to remove GetBoolean/AppendBoolean. Bug: 831839 Change-Id: If61ce8a44799ff8e12957d5206baf74467655894 Test: Expect no behavior change, touches webui/print_preview FYI Reviewed-on: https://chromium-review.googlesource.com/1008870 Reviewed-by: Reilly Grant <reillyg@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org> Cr-Commit-Position: refs/heads/master@{#550196} [modify] https://crrev.com/723fb47254fc5f49bda577df447303dc3554beb9/chrome/browser/ui/webui/print_preview/extension_printer_handler.cc [modify] https://crrev.com/723fb47254fc5f49bda577df447303dc3554beb9/chrome/browser/ui/webui/print_preview/extension_printer_handler_unittest.cc [modify] https://crrev.com/723fb47254fc5f49bda577df447303dc3554beb9/extensions/common/value_builder.cc [modify] https://crrev.com/723fb47254fc5f49bda577df447303dc3554beb9/extensions/common/value_builder.h
,
Apr 12 2018
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/723fb47254fc5f49bda577df447303dc3554beb9 commit 723fb47254fc5f49bda577df447303dc3554beb9 Author: Istiaque Ahmed <lazyboy@chromium.org> Date: Thu Apr 12 14:58:53 2018 Disallow calling value builders' Set/Append(??, bool) methods. Currently calling DictionaryValue::Set(??, bool) and ListValue::Append(bool) would result in calling int variant from the overloaded methods(!). This is confusing and can be a source of bugs. (The current suggested way is to explicitly call SetBoolean and AppendBoolean). There is only once place where this was happening, in extension_printer_handler.cc. Fix that code to use boolean variant, by using SetBoolean() method. This CL will also let us overload the bool variant in future. This CL - Provides an explicit "const char*" param overload. - Declares a bool overload but makes it inaccessible (= delete). This will - Make sure attempting to call DictionaryValue::Set with bool will fail compile (call to deleted member function 'Set'). So accidental introduction of such bugs won't be possible. - The char* variant will make sure we don't attempt to resolve bool to this pointer variant. - Give us the ability to remove GetBoolean/AppendBoolean. Bug: 831839 Change-Id: If61ce8a44799ff8e12957d5206baf74467655894 Test: Expect no behavior change, touches webui/print_preview FYI Reviewed-on: https://chromium-review.googlesource.com/1008870 Reviewed-by: Reilly Grant <reillyg@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org> Cr-Commit-Position: refs/heads/master@{#550196} [modify] https://crrev.com/723fb47254fc5f49bda577df447303dc3554beb9/chrome/browser/ui/webui/print_preview/extension_printer_handler.cc [modify] https://crrev.com/723fb47254fc5f49bda577df447303dc3554beb9/chrome/browser/ui/webui/print_preview/extension_printer_handler_unittest.cc [modify] https://crrev.com/723fb47254fc5f49bda577df447303dc3554beb9/extensions/common/value_builder.cc [modify] https://crrev.com/723fb47254fc5f49bda577df447303dc3554beb9/extensions/common/value_builder.h |
||||
►
Sign in to add a comment |
||||
Comment 1 by thestig@chromium.org
, Apr 11 2018