IndexedDB: Map IDBValue.bits directly to std::string / WebData |
||
Issue descriptionAs part of https://crrev.com/c/1316444, |IDBValue.bits| became type array<uint8>. This fixed a type conversion issue found during IDB onion souping. A downside of using type array<uint8> is that we receive a std::vector or WTF::Vector from Mojo and then convert to std::string or WebData. If we could go directly from the Mojo data view to std::string or WebData, we could eliminate the intermediate vector. Option 1 - follow the Blob pattern, they have a span and then a backing store for the span, so we could have a std::string and std::span<uint8_t> field Option 2 - make IDBValue bits a struct field with definition struct Data { array<uint8> bits }, and map that to a std::string
,
Nov 6
Why not just switch to using vector<uint8_t> throughout instead of "abusing" strings to store binary data? Anyway I don't quite understand option 1, what "blob pattern" is that referring to? Note there is also mojo_base.mojom.ReadOnlyBuffer which you maps to base::span<const uint8_t>, so you can pass a std::string into it and convert it back to a std::string on the other hand without any extra copies or extra typemaps/struct traits. Of course still with some minor extra amount of code.
,
Nov 6
And re comment #1, why is that code converting to a vector to begin with? You can just get the ArrayDataView for the binary data and create a string from that directly...
,
Nov 6
leveldb requires you read using a std::string: https://cs.chromium.org/chromium/src/third_party/leveldatabase/src/include/leveldb/db.h?q=db.h+f:leveldatabase&dr=CSs&l=88 and is generally written to use std::string as buffers.
,
Nov 6
nit: technically leveldb only requires a std::string when you don't use an iterator to read. Otherwise it just seems to use Slice (i.e. something very similar to a span). And would it really be that controversial to add an overload for Get that does operate on a std::vector<uint8_t>? It seems like it would be very easy to do since the conversion from Slice to std::string is already made very late in the stack (i.e. the value is represented as a Slice most of the way through anyway)
,
Nov 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c07c4f39ed016e7b30d1e707486acd4985eb9c7a commit c07c4f39ed016e7b30d1e707486acd4985eb9c7a Author: Chase Phillips <cmp@chromium.org> Date: Mon Nov 12 22:05:55 2018 IndexedDB: Use ArrayDataView to convert IDBKey binary to std::string As part of this change, also update the IndexedDBKey constructors to support std::move(). Bug: 902498 Bug: 717812 Change-Id: Ifd94d01a525850c6293fbfaf28624d3614a419a6 Reviewed-on: https://chromium-review.googlesource.com/c/1321164 Commit-Queue: Chase Phillips <cmp@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/master@{#607347} [modify] https://crrev.com/c07c4f39ed016e7b30d1e707486acd4985eb9c7a/content/browser/indexed_db/indexed_db_database.cc [modify] https://crrev.com/c07c4f39ed016e7b30d1e707486acd4985eb9c7a/content/browser/indexed_db/indexed_db_leveldb_coding.cc [modify] https://crrev.com/c07c4f39ed016e7b30d1e707486acd4985eb9c7a/content/browser/indexed_db/indexed_db_leveldb_coding_unittest.cc [modify] https://crrev.com/c07c4f39ed016e7b30d1e707486acd4985eb9c7a/content/renderer/indexed_db/indexed_db_key_builders.cc [modify] https://crrev.com/c07c4f39ed016e7b30d1e707486acd4985eb9c7a/third_party/blink/common/indexeddb/indexeddb_key.cc [modify] https://crrev.com/c07c4f39ed016e7b30d1e707486acd4985eb9c7a/third_party/blink/common/indexeddb/indexeddb_key_unittest.cc [modify] https://crrev.com/c07c4f39ed016e7b30d1e707486acd4985eb9c7a/third_party/blink/common/indexeddb/indexeddb_mojom_traits.cc [modify] https://crrev.com/c07c4f39ed016e7b30d1e707486acd4985eb9c7a/third_party/blink/public/common/indexeddb/indexeddb_key.h
,
Nov 13
mek@, dmurph@: Regarding Comment 5 and Comment 6 -- if you open a bug against me I'll try to add the overload we need to LevelDB.
,
Nov 14
@pwnall: Filed https://github.com/google/leveldb/issues/636. |
||
►
Sign in to add a comment |
||
Comment 1 by c...@chromium.org
, Nov 6