Binary Key Support

RESOLVED FIXED in Firefox 51

Status

()

P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bevis, Assigned: bevis)

Tracking

({dev-doc-complete})

unspecified
mozilla51
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [tw-dom] )

Attachments

(1 attachment, 1 obsolete attachment)

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
More detailed discussion of this feature can be found at:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23332
Whiteboard: [tw-dom] btpp-fixlater
Status: NEW → ASSIGNED
Mapping fixlater to P2
Priority: -- → P2
Whiteboard: [tw-dom] btpp-fixlater → [tw-dom]
Created attachment 8781015 [details] [diff] [review]
(v1) Patch: Binary Key Support

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

2 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+
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;
    }
Created attachment 8781342 [details] [diff] [review]
(v2) Patch: Binary Key Support. r=janv

Address suggestions in comment 5.
Attachment #8781015 - Attachment is obsolete: true
Attachment #8781342 - Flags: review+
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

2 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++" ?
(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. :)
(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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/23ba2c37b8ef
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.