Closed
Bug 1271500
Opened 9 years ago
Closed 8 years ago
Binary Key Support
Categories
(Core :: Storage: IndexedDB, defect, P2)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: bevis, Assigned: bevis)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [tw-dom] )
Attachments
(1 file, 1 obsolete file)
13.74 KB,
patch
|
bevis
:
review+
|
Details | Diff | Splinter Review |
Binary Key support is specified in [1]:
"
A key has an associated type which is one of: number, date, string, *binary*, or array.
"
[1] https://w3c.github.io/IndexedDB/#key-construct
Assignee | ||
Comment 1•9 years ago
|
||
More detailed discussion of this feature can be found at:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23332
Assignee | ||
Updated•9 years ago
|
Whiteboard: [tw-dom] btpp-fixlater
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 2•9 years ago
|
||
Mapping fixlater to P2
Priority: -- → P2
Whiteboard: [tw-dom] btpp-fixlater → [tw-dom]
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
This is the last brick of IDB v2 features:
1. The input type could be either ArrayBuffer or the view of ArrayBuffer(TypedArray or DataView)[1].
2. The output type should always be ArrayBuffer according to [2], the discussion in [3] and the implementation in Chromium[4].
3. The String key encoding is adopted with new tag named *eBinary* to support the variable length of the binary data and to ensure that these binary keys are comparable/sortable in the same type and among different types.
Hi Jan,
May I have your review for this approach?
Thanks!
[1] http://w3c.github.io/IndexedDB/#convert-value-to-key
[2] http://w3c.github.io/IndexedDB/#convert-key-to-value
[3] https://www.w3.org/Bugs/Public/show_bug.cgi?id=23332#c11
[4] https://codereview.chromium.org/1087923003
Attachment #8781015 -
Flags: review?(jvarga)
Comment 5•9 years ago
|
||
Comment on attachment 8781015 [details] [diff] [review]
(v1) Patch: Binary Key Support
Review of attachment 8781015 [details] [diff] [review]:
-----------------------------------------------------------------
Looks very good to me, but please update the big comment in Key.cpp to mention how we treat binary keys (just in case someone dares to touch this code in future :)
::: dom/indexedDB/Key.cpp
@@ +296,5 @@
> +
> + if (JS_IsArrayBufferObject(obj)) {
> + uint8_t* bufferData = nullptr;
> + uint32_t bufferLength = 0;
> + bool unused = false;
These three don't have to be initialized here.
@@ +307,5 @@
> +
> + if (JS_IsArrayBufferViewObject(obj)) {
> + uint8_t* bufferData = nullptr;
> + uint32_t bufferLength = 0;
> + bool unused = false;
dtto
@@ +313,5 @@
> + js::GetArrayBufferViewLengthAndData(obj, &bufferLength, &unused, &bufferData);
> + EncodeBinary(bufferData, bufferLength, aTypeOffset);
> +
> + return NS_OK;
> + }
Consider to create a C++ template for this and use it here if it looks better to you.
@@ +668,5 @@
> +Key::DecodeBinary(const unsigned char*& aPos,
> + const unsigned char* aEnd,
> + JSContext* aCx)
> +{
> + NS_ASSERTION(*aPos % eMaxType == eBinary, "Don't call me!");
MOZ_ASSERT() for new code
@@ +671,5 @@
> +{
> + NS_ASSERTION(*aPos % eMaxType == eBinary, "Don't call me!");
> +
> + ++aPos;
> + const unsigned char* buffer = aPos;
Nit: const unsigned char* buffer = aPos++;
@@ +688,5 @@
> + return JS_NewArrayBuffer(aCx, 0);
> + }
> +
> + uint8_t* out = static_cast<uint8_t*>(JS_malloc(aCx, size));
> + if (!out) {
Nit: if (NS_WARN_IF(!out)) {
@@ +718,5 @@
> +
> + aPos = iter + 1;
> +
> + NS_ASSERTION(static_cast<size_t>(pos - out) == size,
> + "Should have written the whole buffer");
MOZ_ASSERT()
::: dom/indexedDB/Key.h
@@ +36,4 @@
> eDate = 0x20,
> eString = 0x30,
> eArray = 0x50,
> + eBinary = 0x40,
move after eString
Attachment #8781015 -
Flags: review?(jvarga) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8781015 [details] [diff] [review]
(v1) Patch: Binary Key Support
Review of attachment 8781015 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/indexedDB/Key.cpp
@@ +313,5 @@
> + js::GetArrayBufferViewLengthAndData(obj, &bufferLength, &unused, &bufferData);
> + EncodeBinary(bufferData, bufferLength, aTypeOffset);
> +
> + return NS_OK;
> + }
Thanks for reminding me to keep codes as compact as possible in mind.
After further comparision, I think the following one will be better than creating a template:
In Key.h:
void
EncodeBinary(JSObject* aObject, bool aIsViewObject, uint8_t aTypeOffset);
In Key::EncodeJSValInternal():
if (JS_IsArrayBufferObject(obj)) {
EncodeBinary(obj, /* aIsViewObject */ false, aTypeOffset);
return NS_OK;
}
if (JS_IsArrayBufferViewObject(obj)) {
EncodeBinary(obj, /* aIsViewObject */ true, aTypeOffset);
return NS_OK;
}
Assignee | ||
Comment 7•9 years ago
|
||
Address suggestions in comment 5.
Attachment #8781015 -
Attachment is obsolete: true
Attachment #8781342 -
Flags: review+
Assignee | ||
Comment 8•9 years ago
|
||
The test result of the patch in comment 7 looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=88319a5aea9c
Keywords: checkin-needed
Comment 9•9 years ago
|
||
Comment on attachment 8781342 [details] [diff] [review]
(v2) Patch: Binary Key Support. r=janv
Review of attachment 8781342 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/indexedDB/Key.cpp
@@ +671,5 @@
> + JSContext* aCx)
> +{
> + MOZ_ASSERT(*aPos % eMaxType == eBinary, "Don't call me!");
> +
> + const unsigned char* buffer = ++aPos;
"++aPos" or "aPos++" ?
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Jan Varga [:janv] from comment #9)
> "++aPos" or "aPos++" ?
"++aPos" is the right one to match the same logic of
> ++aPos;
> + const unsigned char* buffer = aPos;
Sorry for not mentioning this when replying review. :)
Comment 11•9 years ago
|
||
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #10)
> (In reply to Jan Varga [:janv] from comment #9)
> > "++aPos" or "aPos++" ?
> "++aPos" is the right one to match the same logic of
> > ++aPos;
> > + const unsigned char* buffer = aPos;
> Sorry for not mentioning this when replying review. :)
of course, my bad :)
Comment 12•9 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23ba2c37b8ef
Binary Key Support. r=janv
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 14•8 years ago
|
||
Docs updated:
https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Basic_Concepts_Behind_IndexedDB
https://developer.mozilla.org/en-US/docs/Web/API/IDBCursor/key
and
https://developer.mozilla.org/en-US/Firefox/Releases/51#IndexedDB
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•