Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
147843008:
libaddressinput - Don't make transient copies of entire JSON rule dictionaries (Closed)
|
Created:
6 years, 10 months ago by Evan Stade Modified:
6 years, 10 months ago Reviewers:
Dan Beam CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionlibaddressinput - Don't make transient copies of entire JSON rule dictionaries
BUG=337679
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248530
Patch Set 1 #Patch Set 2 : more commentary #Patch Set 3 : no obsolete comment #
Total comments: 8
Patch Set 4 : nits #
Messages
Total messages: 24 (0 generated)
(see CountryRulesAggregator for the one place in libaddressinput where GetJsonValueForKey is used, and to see why it's not necessary to update that callsite)
On 2014/01/30 04:42:29, Evan Stade wrote: > (see CountryRulesAggregator for the one place in libaddressinput where > GetJsonValueForKey is used, and to see why it's not necessary to update that > callsite) ping
https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... File third_party/libaddressinput/chromium/cpp/src/util/json.h (right): https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/cpp/src/util/json.h:53: // before invoking this method. The returned object is only gauranteed to be nit: |value| is only guaranteed to be valid as long as |this| is valid. https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... File third_party/libaddressinput/chromium/json.cc (right): https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/json.cc:18: // A base class for Chrome Json objects. Json gets parsed into a nit: s/Json/JSON/g https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/json.cc:93: DCHECK(GetDict()); nit: remove if it'll just explode in the next line... https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/json.cc:99: DCHECK(GetDict()); same
lgtm (w/those nits)
https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... File third_party/libaddressinput/chromium/cpp/src/util/json.h (right): https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/cpp/src/util/json.h:53: // before invoking this method. The returned object is only gauranteed to be On 2014/02/01 01:02:36, Dan Beam wrote: > nit: |value| is only guaranteed to be valid as long as |this| is valid. Done. https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... File third_party/libaddressinput/chromium/json.cc (right): https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/json.cc:18: // A base class for Chrome Json objects. Json gets parsed into a On 2014/02/01 01:02:36, Dan Beam wrote: > nit: s/Json/JSON/g Done. https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/json.cc:93: DCHECK(GetDict()); On 2014/02/01 01:02:36, Dan Beam wrote: > nit: remove if it'll just explode in the next line... Done. https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/json.cc:99: DCHECK(GetDict()); On 2014/02/01 01:02:36, Dan Beam wrote: > same Done.
https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... File third_party/libaddressinput/chromium/cpp/src/util/json.h (right): https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/cpp/src/util/json.h:53: // before invoking this method. The returned object is only gauranteed to be On 2014/02/01 01:02:36, Dan Beam wrote: > nit: |value| is only guaranteed to be valid as long as |this| is valid. Done. https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... File third_party/libaddressinput/chromium/json.cc (right): https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/json.cc:18: // A base class for Chrome Json objects. Json gets parsed into a On 2014/02/01 01:02:36, Dan Beam wrote: > nit: s/Json/JSON/g Done. https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/json.cc:93: DCHECK(GetDict()); On 2014/02/01 01:02:36, Dan Beam wrote: > nit: remove if it'll just explode in the next line... Done. https://codereview.chromium.org/147843008/diff/40001/third_party/libaddressin... third_party/libaddressinput/chromium/json.cc:99: DCHECK(GetDict()); On 2014/02/01 01:02:36, Dan Beam wrote: > same Done.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/147843008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chromedriver_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests, media_unittests, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/147843008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chromedriver_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests, media_unittests, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/147843008/100001
Message was sent while issue was closed.
Change committed as 248530
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
