Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2891453004:
Avoid unnecessary registry/plist reads (hundreds of them) by caching the brand code. (Closed)
DescriptionAvoid unnecessary registry/plist reads (hundreds of them) by caching the brand code.
BUG=723072
Review-Url: https://codereview.chromium.org/2891453004
Cr-Commit-Position: refs/heads/master@{#472622}
Committed: https://chromium.googlesource.com/chromium/src/+/4c703a924fc505015240e1ee859c6a9340d4be2e
Patch Set 1 #Patch Set 2 : Update the comment location. #Patch Set 3 : Minor change. #
Total comments: 2
Patch Set 4 : Changed the initialization pattern for the brand. #Patch Set 5 : Minor formatting (space added). #
Total comments: 1
Messages
Total messages: 22 (7 generated)
Description was changed from ========== Avoid unnecessary registry/plist reads (hundreds of them) by caching the brand code. BUG=723072 ========== to ========== Avoid unnecessary registry/plist reads (hundreds of them) by caching the brand code. BUG=723072 ==========
borisv@chromium.org changed reviewers: + grt@chromium.org, pkasting@chromium.org
https://codereview.chromium.org/2891453004/diff/40001/chrome/browser/search_e... File chrome/browser/search_engines/ui_thread_search_terms_data.cc (right): https://codereview.chromium.org/2891453004/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/ui_thread_search_terms_data.cc:79: static std::string* brand = NULL; the idiomatic way i've seen to do this sort of one-time initialization is: static std::string* brand = []() { auto brand = new std::string(); if (!google_brand::GetBrand(brand)) brand->clear(); return brand; }();
borisv@chromium.org changed reviewers: + borisv@chromium.org
Thanks, Greg. PTAL. https://codereview.chromium.org/2891453004/diff/40001/chrome/browser/search_e... File chrome/browser/search_engines/ui_thread_search_terms_data.cc (right): https://codereview.chromium.org/2891453004/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/ui_thread_search_terms_data.cc:79: static std::string* brand = NULL; On 2017/05/17 08:45:43, grt (UTC plus 2) wrote: > the idiomatic way i've seen to do this sort of one-time initialization is: > static std::string* brand = []() { > auto brand = new std::string(); > if (!google_brand::GetBrand(brand)) > brand->clear(); > return brand; > }(); Sure. I've seen both and I just used the old-fashioned way (as here: https://cs.chromium.org/chromium/src/url/gurl.cc?rcl=733abfce87eb65cf777c6c12...) Changed now. Technically, I don't need to clear on failure based on the current implementation of google_brand::GetBrand, but given that this is now a one-time call, I will throw it there.
LGTM, but FWIW, I found the way you did it before Greg's suggested significantly more readable. Another route might be to change GetBrand() to return an optional<std::string>, then you could probably init here using CR_DEFINE_STATIC_LOCAL for even more clarity. But that may be more effort than this is worth.
On 2017/05/17 22:12:45, Peter Kasting wrote: > LGTM, but FWIW, I found the way you did it before Greg's suggested significantly > more readable. > > Another route might be to change GetBrand() to return an optional<std::string>, > then you could probably init here using CR_DEFINE_STATIC_LOCAL for even more > clarity. But that may be more effort than this is worth. Thank you, Peter. I will keep it with the lambda to avoid checking of brand for NULL on every call.
The CQ bit was checked by borisv@chromium.org
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": 80001, "attempt_start_ts": 1495062639476340,
"parent_rev": "64d2c8a9884ff2ba33dd8dd1d42037b555a5c2c3", "commit_rev":
"4c703a924fc505015240e1ee859c6a9340d4be2e"}
Message was sent while issue was closed.
Description was changed from ========== Avoid unnecessary registry/plist reads (hundreds of them) by caching the brand code. BUG=723072 ========== to ========== Avoid unnecessary registry/plist reads (hundreds of them) by caching the brand code. BUG=723072 Review-Url: https://codereview.chromium.org/2891453004 Cr-Commit-Position: refs/heads/master@{#472622} Committed: https://chromium.googlesource.com/chromium/src/+/4c703a924fc505015240e1ee859c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4c703a924fc505015240e1ee859c...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2891453004/diff/80001/chrome/browser/search_e... File chrome/browser/search_engines/ui_thread_search_terms_data.cc (right): https://codereview.chromium.org/2891453004/diff/80001/chrome/browser/search_e... chrome/browser/search_engines/ui_thread_search_terms_data.cc:80: auto extracted = new std::string(); This broke official builds. It needs to be auto*.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2893483004/ by thestig@chromium.org. The reason for reverting is: Broke official builds due to chromium-style compiler error..
Message was sent while issue was closed.
On 2017/05/18 01:52:50, Lei Zhang wrote: > https://codereview.chromium.org/2891453004/diff/80001/chrome/browser/search_e... > File chrome/browser/search_engines/ui_thread_search_terms_data.cc (right): > > https://codereview.chromium.org/2891453004/diff/80001/chrome/browser/search_e... > chrome/browser/search_engines/ui_thread_search_terms_data.cc:80: auto extracted > = new std::string(); > This broke official builds. It needs to be auto*. Then how did it pass through the CQ?
Message was sent while issue was closed.
On 2017/05/18 01:57:33, Boris Vidolov wrote: > On 2017/05/18 01:52:50, Lei Zhang wrote: > > > https://codereview.chromium.org/2891453004/diff/80001/chrome/browser/search_e... > > File chrome/browser/search_engines/ui_thread_search_terms_data.cc (right): > > > > > https://codereview.chromium.org/2891453004/diff/80001/chrome/browser/search_e... > > chrome/browser/search_engines/ui_thread_search_terms_data.cc:80: auto > extracted > > = new std::string(); > > This broke official builds. It needs to be auto*. > > Then how did it pass through the CQ? CQ doesn't have complete coverage of the main waterfall. There are no official builds.
Message was sent while issue was closed.
On 2017/05/18 02:03:25, Lei Zhang wrote: > On 2017/05/18 01:57:33, Boris Vidolov wrote: > > On 2017/05/18 01:52:50, Lei Zhang wrote: > > > > > > https://codereview.chromium.org/2891453004/diff/80001/chrome/browser/search_e... > > > File chrome/browser/search_engines/ui_thread_search_terms_data.cc (right): > > > > > > > > > https://codereview.chromium.org/2891453004/diff/80001/chrome/browser/search_e... > > > chrome/browser/search_engines/ui_thread_search_terms_data.cc:80: auto > > extracted > > > = new std::string(); > > > This broke official builds. It needs to be auto*. > > > > Then how did it pass through the CQ? > > CQ doesn't have complete coverage of the main waterfall. There are no official > builds. Well, the Windows official build succeeded on my machine. I wasn't aware that the compiler is different on ChromeOS. Sorry about that.
Message was sent while issue was closed.
On 2017/05/18 02:04:58, borisv wrote: > Well, the Windows official build succeeded on my machine. I wasn't aware that > the compiler is different on ChromeOS. Sorry about that. I forget if Windows is using Clang by default or not. If it's not using Clang, then there's no Clang plugin to enforce Chromium style. Or maybe it's using Clang but the Clang plugins are not available on Windows? In any case, I'm just reverting to get the tree green. Please try submitting your CL again with auto*.
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) confirmed this CL at revision 472622 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2017/05/18 02:11:08, findit-for-me wrote: > Findit (https://goo.gl/kROfz5) confirmed this CL at revision 472622 as the > culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... Thank you, Lei. I reproduced the failure on my Mac. I will re-send the change for review tomorrow morning. |
