Closed Bug 435995 Opened 12 years ago Closed 11 years ago

Implement an nsIVariant object storage can use

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch v1.0 (obsolete) — Splinter Review
Spun out of Bug 435994 for ease of review.

This is a templated class that implements nsIVariant.  Works wonderfully in tests so far, but not sure if reviewers will like it.

This can be useful for more than just the async api once it lands too.
Comment on attachment 322710 [details] [diff] [review]
v1.0

I think this is ready for review.
Attachment #322710 - Flags: review?(vladimir)
Whiteboard: [has patch][needs review vlad]
Tests?  Usage?  I'm not actually sure what this does :)
Tests up the dependency tree.  This is part of the async api (bug 429986) stuff but I'm trying to separate parts to make reviewing easier.
Attachment #322710 - Flags: review?(vladimir) → review?(shaver)
Whiteboard: [has patch][needs review vlad] → [has patch][needs review shaver]
Attached patch v1.1 (obsolete) — Splinter Review
Addresses naming concerns.  Shaver wanted bsmedberg to review this.
Attachment #322710 - Attachment is obsolete: true
Attachment #326805 - Flags: review?(benjamin)
Attachment #322710 - Flags: review?(shaver)
Whiteboard: [has patch][needs review shaver] → [has patch][needs review bsmedberg]
Comment on attachment 326805 [details] [diff] [review]
v1.1

bsmedberg thinks dbaron should review this...
Attachment #326805 - Flags: review?(benjamin) → review?(dbaron)
Whiteboard: [has patch][needs review bsmedberg] → [has patch][needs review dbaron]
Why me?  I've got a bunch of other stuff in my queue that does require my review.


What's the #include <utility> for?  How portable is it?

It seems like you could get rid of a good bit of code bloat by giving mozStorageVariant a non-templatized base class that has the methods that don't vary, and implementing them in a C++ file.

Could you describe what this class is for and how it's used?  Preferably in code comments?
Also, it seems like the template parameter isn't use purely as a type; PRUint8 looks like it's code for "array", which seems at the very least a bit weird.
(In reply to comment #6)
> Why me?  I've got a bunch of other stuff in my queue that does require my
> review.
shaver punted to bsmedberg, who punted to you on it.

> What's the #include <utility> for?  How portable is it?
I'm using std::pair.  As far as I know, it is portable.

> It seems like you could get rid of a good bit of code bloat by giving
> mozStorageVariant a non-templatized base class that has the methods that don't
> vary, and implementing them in a C++ file.
Yeah, that's actually a good idea.  Next version will do that.

> Could you describe what this class is for and how it's used?  Preferably in
> code comments?
Sure, that'll be in the next patch.

(In reply to comment #7)
> Also, it seems like the template parameter isn't use purely as a type; PRUint8
> looks like it's code for "array", which seems at the very least a bit weird.
Good point.  That will be changed to PRUint8[].
Whiteboard: [has patch][needs review dbaron] → [needs new patch]
Target Milestone: --- → mozilla1.9.1a1
Attached patch v1.2 (obsolete) — Splinter Review
Addresses review comments.
Attachment #326805 - Attachment is obsolete: true
Attachment #328381 - Flags: review?(dbaron)
Attachment #326805 - Flags: review?(dbaron)
Whiteboard: [needs new patch] → [has patch][needs review dbaron]
I don't like the name assign in variant_storage_traits<DataType>::assign(aData).
Maybe storage_form or convert or something like that would be better but assign doesn't actually assign anything so it's a bit misleading.
Attachment #328381 - Flags: review?(dbaron)
Whiteboard: [has patch][needs review dbaron] → [needs new patch]
Attached patch v1.3 (obsolete) — Splinter Review
Attachment #328381 - Attachment is obsolete: true
Attachment #328676 - Flags: review?(vladimir)
Attachment #328676 - Flags: review?(tellrob)
Comment on attachment 328676 [details] [diff] [review]
v1.3

be consistent in what you call the return type and parameters types for storage_conversion and the at* functions. To avoid accidentally creating overloaded members, you should probably use the typedef'd names.
Attached patch v1.4 (obsolete) — Splinter Review
Addresses comments as discussed over im.
Attachment #328676 - Attachment is obsolete: true
Attachment #328896 - Flags: review?(vladimir)
Attachment #328896 - Flags: review?(tellrob)
Attachment #328676 - Flags: review?(vladimir)
Attachment #328676 - Flags: review?(tellrob)
Attached patch v1.4Splinter Review
ugh - bug numbers are too similar.  That was the wrong patch.
Attachment #328896 - Attachment is obsolete: true
Attachment #328909 - Flags: review?(vladimir)
Attachment #328909 - Flags: review?(tellrob)
Attachment #328896 - Flags: review?(vladimir)
Attachment #328896 - Flags: review?(tellrob)
Comment on attachment 328909 [details] [diff] [review]
v1.4

I like the new typedefs
Attachment #328909 - Flags: review?(tellrob) → review+
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/3b714908eed1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs new patch]
Comment on attachment 328909 [details] [diff] [review]
v1.4

>+    _result = NS_ConvertUTF16toUTF8(aValue);
CopyUTF16toUTF8(aValue, _result);

>+    nsTArray<PRUint8> data(aBlob.second);
>+    for (int i = 0; i < aBlob.second; i++)
>+      data.InsertElementAt(i, static_cast<const PRUint8 *>(aBlob.first)[i]);
data.AppendElements(static_cast<const PRUint8 *>(aBlob.first), aBlob.second);

>+    // Copy the array
>+    *_result = NS_Alloc(sizeof(PRUint8) * aData.Length());
>+    NS_ENSURE_TRUE(*_result, NS_ERROR_OUT_OF_MEMORY);
>+
>+    PRUint8 *result = static_cast<PRUint8 *>(*_result);
>+    for (PRUint32 i = 0; i < aData.Length(); i++)
>+      result[i] = aData[i];
*_result = nsMemory::Clone(aData.Elements(), aData.Length() * sizeof(PRUint8));
Blocks: 451058
You need to log in before you can comment on or make changes to this bug.