ConstructInferredLabel should take ServerFieldType* instead of std::vector |
||||
Issue description
The usage patterns of ConstructInferredLabel make it very easy for it to take a ServerFieldType* instead of an std::vector. The current use of std::vector means a lot more code to construct these vectors, plus the allocation overhead, and none of the advantages of the std::vector seem to be used.
STL containers are great, but when C arrays are a reasonable choice they should be used to reduce code bloat and dynamic memory allocation. As an example, if ConstructInferredLabel took a pointer then this code:
std::vector<ServerFieldType> label_fields;
label_fields.push_back(NAME_FULL);
label_fields.push_back(ADDRESS_HOME_LINE1);
label_fields.push_back(ADDRESS_HOME_LINE2);
label_fields.push_back(ADDRESS_HOME_DEPENDENT_LOCALITY);
label_fields.push_back(ADDRESS_HOME_CITY);
label_fields.push_back(ADDRESS_HOME_STATE);
label_fields.push_back(ADDRESS_HOME_ZIP);
label_fields.push_back(ADDRESS_HOME_SORTING_CODE);
could be replaced with something like this:
static constexpr ServerFieldType label_fields[] = {
NAME_FULL,
ADDRESS_HOME_LINE1,
ADDRESS_HOME_LINE2,
ADDRESS_HOME_DEPENDENT_LOCALITY,
ADDRESS_HOME_CITY,
ADDRESS_HOME_STATE,
ADDRESS_HOME_ZIP,
ADDRESS_HOME_SORTING_CODE,
};
This gets rid of all of the dynamic initialization and the heap allocations. The call would then change to:
profile.ConstructInferredLabel(
label_fields, arraysize(label_fields), // Was label_fields.size()
g_browser_process->GetApplicationLocale())
This was found because some of the std::vector objects were tagged as static and therefore created a shutdown-hook for calling the destructor (more code).
,
Nov 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dcbe43d091a8b3765fde012d44503a6b5100b799 commit dcbe43d091a8b3765fde012d44503a6b5100b799 Author: Mohamed Heikal <mheikal@google.com> Date: Fri Nov 03 18:35:35 2017 ConstructInferredLabel now takes ServerFieldType* instead of std::vector ConstructInferredLabel can just as easily use a simple array of ServerFieldType in place of an std::vector. Calling the function with a static value for the parameter results in boilerplate code as well as unnecessary memory overhead for dynamic allocation. Bug: 778353 Change-Id: Ie2b3f771db3c23528148bf3af4e55a3873d62cca Reviewed-on: https://chromium-review.googlesource.com/747533 Commit-Queue: Mohamed Heikal <mheikal@google.com> Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org> Reviewed-by: Roger McFarlane <rogerm@chromium.org> Cr-Commit-Position: refs/heads/master@{#513849} [modify] https://crrev.com/dcbe43d091a8b3765fde012d44503a6b5100b799/chrome/browser/autofill/android/personal_data_manager_android.cc [modify] https://crrev.com/dcbe43d091a8b3765fde012d44503a6b5100b799/chrome/browser/ui/views/payments/payment_sheet_view_controller.cc [modify] https://crrev.com/dcbe43d091a8b3765fde012d44503a6b5100b799/components/autofill/core/browser/autofill_profile.cc [modify] https://crrev.com/dcbe43d091a8b3765fde012d44503a6b5100b799/components/autofill/core/browser/autofill_profile.h [modify] https://crrev.com/dcbe43d091a8b3765fde012d44503a6b5100b799/components/payments/core/strings_util.cc
,
Nov 3 2017
,
Nov 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ff2e5ef13eb78fb57880db2a987f900d3e9d61a5 commit ff2e5ef13eb78fb57880db2a987f900d3e9d61a5 Author: Sky Malice <skym@chromium.org> Date: Fri Nov 03 23:49:52 2017 Revert "ConstructInferredLabel now takes ServerFieldType* instead of std::vector" This reverts commit dcbe43d091a8b3765fde012d44503a6b5100b799. Reason for revert: https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/64448/steps/components_unittests Original change's description: > ConstructInferredLabel now takes ServerFieldType* instead of std::vector > > ConstructInferredLabel can just as easily use a simple array of > ServerFieldType in place of an std::vector. Calling the function with a > static value for the parameter results in boilerplate code as well as > unnecessary memory overhead for dynamic allocation. > > Bug: 778353 > Change-Id: Ie2b3f771db3c23528148bf3af4e55a3873d62cca > Reviewed-on: https://chromium-review.googlesource.com/747533 > Commit-Queue: Mohamed Heikal <mheikal@google.com> > Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org> > Reviewed-by: Roger McFarlane <rogerm@chromium.org> > Cr-Commit-Position: refs/heads/master@{#513849} TBR=rouslan@chromium.org,rogerm@chromium.org,mheikal@google.com Change-Id: I8bfdd232f43ed1752de031dfdf9686a90dcbe9e7 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 778353 Reviewed-on: https://chromium-review.googlesource.com/754035 Reviewed-by: Sky Malice <skym@chromium.org> Commit-Queue: Sky Malice <skym@chromium.org> Cr-Commit-Position: refs/heads/master@{#513966} [modify] https://crrev.com/ff2e5ef13eb78fb57880db2a987f900d3e9d61a5/chrome/browser/autofill/android/personal_data_manager_android.cc [modify] https://crrev.com/ff2e5ef13eb78fb57880db2a987f900d3e9d61a5/chrome/browser/ui/views/payments/payment_sheet_view_controller.cc [modify] https://crrev.com/ff2e5ef13eb78fb57880db2a987f900d3e9d61a5/components/autofill/core/browser/autofill_profile.cc [modify] https://crrev.com/ff2e5ef13eb78fb57880db2a987f900d3e9d61a5/components/autofill/core/browser/autofill_profile.h [modify] https://crrev.com/ff2e5ef13eb78fb57880db2a987f900d3e9d61a5/components/payments/core/strings_util.cc
,
Nov 6 2017
,
Nov 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/966b76361c548f58c038efd501e70260b9294e86 commit 966b76361c548f58c038efd501e70260b9294e86 Author: Mohamed Heikal <mheikal@google.com> Date: Tue Nov 07 22:45:09 2017 Reland: ConstructInferredLabel now takes ServerFieldType* instead of std::vector This reverts commit ff2e5ef13eb78fb57880db2a987f900d3e9d61a5. Reason for reland: Fixed bug that caused revert. ConstructInferredLabel can just as easily use a simple array of ServerFieldType in place of an std::vector. Calling the function with a static value for the parameter results in boilerplate code as well as unnecessary memory overhead for dynamic allocation. TBR=agrieve@chromium.org Bug: 778353 Change-Id: Ibddcd8bf18c42079e01b5d90004e89ec292b07b6 Reviewed-on: https://chromium-review.googlesource.com/755573 Reviewed-by: agrieve <agrieve@chromium.org> Commit-Queue: agrieve <agrieve@chromium.org> Cr-Commit-Position: refs/heads/master@{#514633} [modify] https://crrev.com/966b76361c548f58c038efd501e70260b9294e86/chrome/browser/autofill/android/personal_data_manager_android.cc [modify] https://crrev.com/966b76361c548f58c038efd501e70260b9294e86/chrome/browser/ui/views/payments/payment_sheet_view_controller.cc [modify] https://crrev.com/966b76361c548f58c038efd501e70260b9294e86/components/autofill/core/browser/autofill_profile.cc [modify] https://crrev.com/966b76361c548f58c038efd501e70260b9294e86/components/autofill/core/browser/autofill_profile.h [modify] https://crrev.com/966b76361c548f58c038efd501e70260b9294e86/components/payments/core/strings_util.cc
,
Nov 13 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by mheikal@chromium.org
, Nov 1 2017Status: Started (was: Untriaged)