Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(293)

Issue 820673004: json_schema_compiler: Use std::vector<char> for binary values. (Closed)

Created:
5 years, 11 months ago by pneubeck (no reviews)
Modified:
5 years, 11 months ago
CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, mlamouri+watch-notifications_chromium.org, jam, yuzo+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, oshima+watch_chromium.org, dtseng+watch_chromium.org, chromium-apps-reviews_chromium.org, peter+watch_chromium.org, erikwright+watch_chromium.org, stevenjb+watch_chromium.org, hashimoto+watch_chromium.org, davemoore+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@simplify_json_schema
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

json_schema_compiler: Use std::vector<char> for binary values. This change enables ArrayBuffer[] in IDLs / json-schema descriptions of extension APIs. This affects the code generated from these descriptions: The C++ members (and arguments to the ::Create functions) that refer to binary values are now represented by std::vector<char> instead of std::string. All usages are adapted in this CL. While there, this change updates the affected code parts - to use vector_as_array where it makes sense - unnecessary copies are removed where easily possible - and range based assigns ( ::assign(InputIterator first, InputIterator last) or the same with c'tors) are uniformly used. BUG=448383 TBR=stevenjb@chromium.org (chromeos/dbus was reviewed by bartfab@) Committed: https://crrev.com/93871258123f9c02d00dc33ff96d1d4a240fae68 Cr-Commit-Position: refs/heads/master@{#312183}

Patch Set 1 #

Total comments: 13

Patch Set 2 : Addressed feedback #

Total comments: 2

Patch Set 3 : Fix compile error on non-ChromeOS. #

Patch Set 4 : byte -> 8 bit #

Patch Set 5 : Fix generation error (data() -> vector_as_array) #

Patch Set 6 : Fix unit tests: usage of arraysize included \0. Switched to string. #

Patch Set 7 : Fix merge error. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+289 lines, -168 lines) Patch
M base/values.h View 3 chunks +3 lines, -0 lines 0 comments Download
M base/values.cc View 2 chunks +10 lines, -0 lines 0 comments Download
M base/values_unittest.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/extensions/wallpaper_api.cc View 1 2 chunks +5 lines, -1 line 0 comments Download
M chrome/browser/chromeos/extensions/wallpaper_function_base.h View 1 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/chromeos/extensions/wallpaper_function_base.cc View 1 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/extensions/wallpaper_private_api.h View 1 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/chromeos/extensions/wallpaper_private_api.cc View 1 6 chunks +14 lines, -7 lines 0 comments Download
M chrome/browser/chromeos/file_system_provider/operations/read_file.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/file_system_provider/operations/write_file_unittest.cc View 1 2 3 4 5 2 chunks +5 lines, -1 line 0 comments Download
M chrome/browser/copresence/chrome_whispernet_client.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/extensions/api/braille_display_private/braille_controller.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.h View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc View 3 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/extensions/api/braille_display_private/braille_controller_stub.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/extensions/api/braille_display_private/stub_braille_controller.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/braille_display_private/stub_braille_controller.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/copresence/copresence_api.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/extensions/api/copresence/copresence_api_unittest.cc View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/copresence/copresence_translations.cc View 3 chunks +6 lines, -2 lines 0 comments Download
M chrome/browser/extensions/api/copresence_private/copresence_private_api.cc View 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api.cc View 1 2 8 chunks +19 lines, -13 lines 0 comments Download
M chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_crypto_delegate_chromeos.cc View 1 2 chunks +32 lines, -16 lines 0 comments Download
M chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc View 6 chunks +16 lines, -12 lines 0 comments Download
M chrome/browser/image_decoder.h View 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/image_decoder.cc View 1 chunk +10 lines, -0 lines 0 comments Download
M chrome/browser/notifications/notification_conversion_helper.cc View 3 chunks +12 lines, -8 lines 0 comments Download
M chromeos/dbus/fake_shill_third_party_vpn_driver_client.h View 1 chunk +2 lines, -2 lines 0 comments Download
M chromeos/dbus/fake_shill_third_party_vpn_driver_client.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chromeos/dbus/shill_third_party_vpn_driver_client.h View 3 chunks +4 lines, -2 lines 0 comments Download
M chromeos/dbus/shill_third_party_vpn_driver_client.cc View 1 2 3 4 chunks +9 lines, -7 lines 0 comments Download
M chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc View 1 5 chunks +9 lines, -6 lines 0 comments Download
M chromeos/dbus/shill_third_party_vpn_observer.h View 2 chunks +2 lines, -2 lines 0 comments Download
M content/browser/speech/google_streaming_remote_engine_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M content/public/browser/speech_recognition_session_preamble.h View 2 chunks +2 lines, -2 lines 0 comments Download
M extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M extensions/browser/api/bluetooth_socket/bluetooth_socket_event_dispatcher.cc View 1 chunk +1 line, -1 line 0 comments Download
M extensions/browser/api/copresence_endpoints/copresence_endpoints_api.cc View 2 chunks +4 lines, -3 lines 0 comments Download
M extensions/browser/api/hid/hid_api.cc View 3 chunks +5 lines, -4 lines 0 comments Download
M extensions/browser/api/hid/hid_device_manager.cc View 1 1 chunk +2 lines, -3 lines 0 comments Download
M extensions/browser/api/serial/serial_connection.h View 3 chunks +5 lines, -3 lines 0 comments Download
M extensions/browser/api/serial/serial_connection.cc View 5 chunks +8 lines, -5 lines 0 comments Download
M extensions/browser/api/serial/serial_event_dispatcher.h View 2 chunks +2 lines, -1 line 0 comments Download
M extensions/browser/api/serial/serial_event_dispatcher.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M extensions/browser/api/sockets_tcp/tcp_socket_event_dispatcher.h View 1 chunk +2 lines, -0 lines 0 comments Download
M extensions/browser/api/sockets_tcp/tcp_socket_event_dispatcher.cc View 1 chunk +1 line, -1 line 0 comments Download
M extensions/browser/api/sockets_udp/udp_socket_event_dispatcher.h View 1 chunk +2 lines, -0 lines 0 comments Download
M extensions/browser/api/sockets_udp/udp_socket_event_dispatcher.cc View 1 chunk +1 line, -1 line 0 comments Download
M extensions/browser/api/usb/usb_api.cc View 1 3 chunks +3 lines, -15 lines 0 comments Download
M extensions/browser/api/vpn_provider/vpn_service.h View 3 chunks +5 lines, -4 lines 0 comments Download
M extensions/browser/api/vpn_provider/vpn_service.cc View 3 chunks +4 lines, -3 lines 0 comments Download
M tools/json_schema_compiler/cc_generator.py View 1 2 3 4 4 chunks +12 lines, -8 lines 0 comments Download
M tools/json_schema_compiler/cpp_type_generator.py View 1 1 chunk +1 line, -1 line 0 comments Download
M tools/json_schema_compiler/util.h View 1 3 chunks +3 lines, -1 line 0 comments Download
M tools/json_schema_compiler/util.cc View 1 2 3 4 5 6 5 chunks +17 lines, -3 lines 0 comments Download

Messages

Total messages: 26 (9 generated)
pneubeck (no reviews)
5 years, 11 months ago (2015-01-16 16:25:44 UTC) #2
not at google - send to devlin
lgtm, phew, that was quite a slog. really sorry about that :-(
5 years, 11 months ago (2015-01-16 18:17:46 UTC) #3
pneubeck (no reviews)
xiyuan@chromium.org: Please review changes in chrome/browser/copresence/ chrome/browser/chromeos/file_system_provider/operations/ stevenjb@chromium.org: Please review changes in chromeos/dbus/ avi@chromium.org: Please ...
5 years, 11 months ago (2015-01-16 21:05:34 UTC) #5
xiyuan
LGTM chrome/browser/copresence/ chrome/browser/chromeos/file_system_provider/operations/
5 years, 11 months ago (2015-01-16 21:17:28 UTC) #6
Avi (use Gerrit)
What? BinaryValue was string before? Ugh, good to see this. content LGTM https://codereview.chromium.org/820673004/diff/1/base/values_unittest.cc File base/values_unittest.cc ...
5 years, 11 months ago (2015-01-16 21:19:10 UTC) #7
Lei Zhang
On 2015/01/16 21:05:34, pneubeck wrote: > mailto:thestig@chromium.org: Please review changes in > chrome/browser/image_decoder.* > chrome/browser/notifications/notification_conversion_helper.cc ...
5 years, 11 months ago (2015-01-16 21:49:14 UTC) #8
Mark Mentovai
LGTM
5 years, 11 months ago (2015-01-16 22:24:45 UTC) #9
pneubeck (no reviews)
@Bartosz, ptal at the vpn provider code in chromeos/dbus
5 years, 11 months ago (2015-01-19 09:27:09 UTC) #11
bartfab (slow)
https://codereview.chromium.org/820673004/diff/1/chromeos/dbus/shill_third_party_vpn_driver_client.cc File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/820673004/diff/1/chromeos/dbus/shill_third_party_vpn_driver_client.cc#newcode7 chromeos/dbus/shill_third_party_vpn_driver_client.cc:7: #include <string> Nit: Already included by header. https://codereview.chromium.org/820673004/diff/1/chromeos/dbus/shill_third_party_vpn_driver_client.cc#newcode8 chromeos/dbus/shill_third_party_vpn_driver_client.cc:8: ...
5 years, 11 months ago (2015-01-19 10:08:38 UTC) #12
pneubeck (no reviews)
https://codereview.chromium.org/820673004/diff/1/chromeos/dbus/shill_third_party_vpn_driver_client.cc File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/820673004/diff/1/chromeos/dbus/shill_third_party_vpn_driver_client.cc#newcode7 chromeos/dbus/shill_third_party_vpn_driver_client.cc:7: #include <string> On 2015/01/19 10:08:38, bartfab wrote: > Nit: ...
5 years, 11 months ago (2015-01-19 17:13:46 UTC) #14
bartfab (slow)
LGTM with some nits. https://codereview.chromium.org/820673004/diff/1/chromeos/dbus/shill_third_party_vpn_driver_client.cc File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/820673004/diff/1/chromeos/dbus/shill_third_party_vpn_driver_client.cc#newcode7 chromeos/dbus/shill_third_party_vpn_driver_client.cc:7: #include <string> On 2015/01/19 17:13:46, ...
5 years, 11 months ago (2015-01-19 17:57:18 UTC) #15
pneubeck (no reviews)
https://codereview.chromium.org/820673004/diff/1/chromeos/dbus/shill_third_party_vpn_driver_client.cc File chromeos/dbus/shill_third_party_vpn_driver_client.cc (right): https://codereview.chromium.org/820673004/diff/1/chromeos/dbus/shill_third_party_vpn_driver_client.cc#newcode7 chromeos/dbus/shill_third_party_vpn_driver_client.cc:7: #include <string> On 2015/01/19 17:57:17, bartfab wrote: > On ...
5 years, 11 months ago (2015-01-19 18:04:29 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/820673004/80001
5 years, 11 months ago (2015-01-19 18:05:13 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tests_recipe/builds/46577)
5 years, 11 months ago (2015-01-19 18:22:34 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/820673004/140001
5 years, 11 months ago (2015-01-20 10:21:49 UTC) #24
commit-bot: I haz the power
Committed patchset #7 (id:140001)
5 years, 11 months ago (2015-01-20 11:27:09 UTC) #25
commit-bot: I haz the power
5 years, 11 months ago (2015-01-20 11:28:17 UTC) #26
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/93871258123f9c02d00dc33ff96d1d4a240fae68
Cr-Commit-Position: refs/heads/master@{#312183}

Powered by Google App Engine
This is Rietveld 408576698