Remove HttpRequestHeaders::AddHeadersFromString |
|||
Issue descriptionA lot of code that could be using HttpRequestHeaders::AddHeaders is using HttpRequestHeaders::AddHeadersFromString for no good reason. It's extra work, and doesn't provide any benefits. Almost no consumers need AddHeadersFromString - they almost all know the header name and value as separate string. If any consumers do actually need the string parsing, we can add a heavier weight method to the headers to a map (Or as a full set of headers), and then add them in a loop. This is low priority, but I think it's a good change for code health.
,
Jul 26
Marking as assigned to me, though peary2@ is doing the work here (Can't assign to people who aren't committers, unfortunately).
,
Jul 26
Oh, looks like I can assign it to peary2, actually. Nice!
,
Jul 26
And to fix a bug in the description: We should replace HttpRequestHeaders::AddHeadersFromString calls with HttpRequestHeaders::SetHeader.
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/45b9e0af56c474bf8184963f3e3bf92be019c999 commit 45b9e0af56c474bf8184963f3e3bf92be019c999 Author: Yeol <peary2@gmail.com> Date: Fri Jul 27 02:03:29 2018 Uses SetHeader instead of AddHeadersFromString, for code health. So it replaced AddHeadersFromString with SetHeader in the unit tests. Bug: 862175 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: Ia29ed00a158d1dd37fd0aea28cb71b50320daa8e Reviewed-on: https://chromium-review.googlesource.com/1146411 Reviewed-by: Dominic Battré <battre@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Commit-Queue: Yeol Park <peary2@gmail.com> Cr-Commit-Position: refs/heads/master@{#578518} [modify] https://crrev.com/45b9e0af56c474bf8184963f3e3bf92be019c999/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc [modify] https://crrev.com/45b9e0af56c474bf8184963f3e3bf92be019c999/net/url_request/url_request_unittest.cc [modify] https://crrev.com/45b9e0af56c474bf8184963f3e3bf92be019c999/services/network/url_loader_unittest.cc
,
Aug 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0846cc0cfd193aa0118c6a468631bbe1f482431 commit f0846cc0cfd193aa0118c6a468631bbe1f482431 Author: Yeol <peary2@gmail.com> Date: Wed Aug 01 07:43:25 2018 Uses SetHeader instead of AddHeadersFromString, for code health. So it replaced AddHeadersFromString, AddHeaderFromString with SetHeader in ../chrome. Bug: 862175 Change-Id: Ifec193eaf09ff75907ed4f0b0c7b29a3a724183b Reviewed-on: https://chromium-review.googlesource.com/1154852 Reviewed-by: Marc Treib <treib@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Peter Lee <pkl@chromium.org> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org> Reviewed-by: Luis Hector Chavez <lhchavez@chromium.org> Reviewed-by: Nathan Parker <nparker@chromium.org> Commit-Queue: Yeol Park <peary2@gmail.com> Cr-Commit-Position: refs/heads/master@{#579721} [modify] https://crrev.com/f0846cc0cfd193aa0118c6a468631bbe1f482431/chrome/browser/chromeos/arc/auth/arc_background_auth_code_fetcher.cc [modify] https://crrev.com/f0846cc0cfd193aa0118c6a468631bbe1f482431/chrome/browser/feedback/feedback_uploader_chrome.cc [modify] https://crrev.com/f0846cc0cfd193aa0118c6a468631bbe1f482431/chrome/browser/profiles/profile_downloader.cc [modify] https://crrev.com/f0846cc0cfd193aa0118c6a468631bbe1f482431/chrome/browser/safe_browsing/download_protection/two_phase_uploader.cc [modify] https://crrev.com/f0846cc0cfd193aa0118c6a468631bbe1f482431/chrome/browser/supervised_user/child_accounts/family_info_fetcher.cc [modify] https://crrev.com/f0846cc0cfd193aa0118c6a468631bbe1f482431/chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.cc [modify] https://crrev.com/f0846cc0cfd193aa0118c6a468631bbe1f482431/chrome/browser/supervised_user/experimental/safe_search_url_reporter.cc [modify] https://crrev.com/f0846cc0cfd193aa0118c6a468631bbe1f482431/chrome/browser/supervised_user/supervised_user_constants.cc [modify] https://crrev.com/f0846cc0cfd193aa0118c6a468631bbe1f482431/chrome/browser/ui/desktop_ios_promotion/sms_service.cc
,
Oct 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fe9c6dc48ea20b1bacd9fc095652cf1fa319ce81 commit fe9c6dc48ea20b1bacd9fc095652cf1fa319ce81 Author: Yeol <peary2@gmail.com> Date: Thu Oct 25 01:53:16 2018 Remove HttpRequestHeaders::AddHeadersFromString for code health. So it replaced AddHeadersFromString and AddHeaderFromString with SetHeader in //components. Bug: 862175 Change-Id: Ib5d176ad6121bb30804a72b9bd013fbb868ea213 Reviewed-on: https://chromium-review.googlesource.com/c/1297784 Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Reviewed-by: Ryan Sturm <ryansturm@chromium.org> Reviewed-by: Michael Martis <martis@chromium.org> Commit-Queue: Yeol Park <peary2@gmail.com> Cr-Commit-Position: refs/heads/master@{#602573} [modify] https://crrev.com/fe9c6dc48ea20b1bacd9fc095652cf1fa319ce81/components/autofill/core/browser/payments/payments_client.cc [modify] https://crrev.com/fe9c6dc48ea20b1bacd9fc095652cf1fa319ce81/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc [modify] https://crrev.com/fe9c6dc48ea20b1bacd9fc095652cf1fa319ce81/components/translate/core/browser/translate_url_fetcher.cc |
|||
►
Sign in to add a comment |
|||
Comment 1 by mmenke@chromium.org
, Jul 18