Closed Bug 1271500 Opened 4 years ago Closed 4 years ago

Binary Key Support

Categories

(Core :: Storage: IndexedDB, defect, P2)

defect

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)

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]
Attached patch (v1) Patch: Binary Key Support (obsolete) — Splinter Review
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 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;
    }
Address suggestions in comment 5.
Attachment #8781015 - Attachment is obsolete: true
Attachment #8781342 - Flags: review+
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 :)
https://hg.mozilla.org/mozilla-central/rev/23ba2c37b8ef
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.