Closed
Bug 796850
Opened 12 years ago
Closed 11 years ago
Implement ByteString in the WebIDL bindings
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: Ms2ger, Assigned: jkitch)
References
(Blocks 1 open bug, )
Details
(Keywords: doc-bug-filed, Whiteboard: [mentor=bz][lang=python][lang=c++])
Attachments
(5 files, 14 obsolete files)
1.68 KB,
text/plain
|
Details | |
16.31 KB,
patch
|
Details | Diff | Splinter Review | |
25.83 KB,
patch
|
Details | Diff | Splinter Review | |
14.41 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
28.72 KB,
patch
|
Details | Diff | Splinter Review |
Andrea, do you happen to be interested? :)
Updated•12 years ago
|
Assignee: nobody → amarchesini
Updated•12 years ago
|
Comment 1•12 years ago
|
||
I am interested... but I cannot work on this right now.
Assignee: amarchesini → nobody
Updated•11 years ago
|
Whiteboard: [mentor=bz][lang=python][lang=c++]
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → jkitch.bug
Assignee | ||
Comment 2•11 years ago
|
||
My work so far. Support for bytestrings has been added to the webidl parser and code generator. Compiles, but it needs additional testing before I am confident of correctness.
Assignee | ||
Comment 3•11 years ago
|
||
Now includes tests. Is this a reasonable approach? isString() is currently kept as syntatic sugar and tests against both isDOMString() and isByteString(). Should this be removed? It may be a little ambiguous given the type of a DOMString is "String"
Attachment #745629 -
Attachment is obsolete: true
Attachment #747968 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 4•11 years ago
|
||
I'm less confident about the correctness of this patch. It compiles, parses the Test*.webidl files and appears to generate code, but is otherwise lacking in tests. (In particular, the "Throw a TypeError if char value > 255" behaviour is in need of testing). Can you suggest a way to perform runtime tests?
Attachment #747979 -
Flags: feedback?(bzbarsky)
Comment 5•11 years ago
|
||
Comment on attachment 747968 [details] [diff] [review] part 1. WebIDL parser changes to support ByteString Are there still consumers of isString when this is all landed? I guess it would at least be used in isDistinguishableFrom, and its use there is sensible. I don't think the IDLValue.coerceToType changes are needed or desired. Per spec there is no way to specify constant bytestrings or to set default values for bytestring stuff, right? If the latter _is_ possible, I see no indication that having values over 255 would be disallowed, nonsensical as they would be. Please raise spec issues as needed. >+ harness.check(len(members), 2, "Should be one production") Two productions. >+ #now check we haven't broken DOMStrings in the process. Space after '#'. >+ const ByteString = "hello" const ByteString foo = "hello"; no? Check which exception you're getting, please. And for the arg with default value test, I guess we should see what we decide the spec says? Thanks for doing this!
Attachment #747968 -
Flags: feedback?(bzbarsky) → feedback+
Comment 6•11 years ago
|
||
Comment on attachment 747979 [details] [diff] [review] part 2: Code Generation changes & tests > Can you suggest a way to perform runtime tests? We don't have a way to do that with WebIDL right now short of implementing an actual interface using the functionality and then writing mochitests for it.... So we'd just need to use this thing in XHR as specced and add some tests. I'm not sure we need the mozilla::dom::ByteString class; It's really a performance optimization, and I would assume nothing really performance-sensitive uses ByteString. I would also get rid of the Optional specialization. Just use Optional<nsCString> to represent optional ByteString values. We can't do that for DOMString because dependent strings are not nsString, but for ByteString we can't use dependent strings anyway. Similar for FakeDependentCString, which is even worse because we have nothing to depend on! We can just use an nsCString, I would think. The version of ConvertJSValueToString that does ByteString stuff can be way simpler: there are no different stringification behaviors involved except to the extent that a nullable type is used, but then both undefined and null become null. >+ result.SetData(ToNewCString(nsAutoString(chars)), len); That's a leak right there. What I suggest doing instead for ByteString is something more like what XPCConvert::JSData2Native does for the ACString type. See http://hg.mozilla.org/mozilla-central/file/08be63954b6b/js/xpconnect/src/XPCConvert.cpp#l691 except your nsCString will already be on the stack instead of needing to heap-allocate it. >+ # XXX Suspect this needs work It can be simplified a good bit: Just always use nsCString as the decltype. And if we use Optional<nsCString> for optionals we don't need special-case Optional handling here either: just return True for the fourth element of the return-value tuple, and the Optional bits will be done automatically. And as I said above, we don't have different stringification behaviors here, modulo nullable, and the behavior for null and undefined is always the same. >+ elif arg.type.isByteString(): >+ # XPConnect string-to-JS conversion wants to mutate the string. It won't; see below. >+MSG_DEF(MSG_INVALID_BYTESTRING, 0, "Cannot convert to ByteString with character exceeding 255.") Maybe "Cannot convert string to ByteString because the character at index {0} has value {1} which is greater than 255" and pass in the values? ByteStringToJsval should just call NonVoidByteStringToJsval when not null. And both can always take a const string. The implementation of NonVoidByteStringToJsval should probably just do something similar to what http://hg.mozilla.org/mozilla-central/file/08be63954b6b/js/xpconnect/src/XPCConvert.cpp#l285 does and be out of line. There's no point calling into NonVoidStringToJsval with all its complexity. I don't think we need the changes to nsrootidl or xpidl.py at all here. Just take those out. Sorry ByteString isn't more like DOMString... but it's really not. :(
Attachment #747979 -
Flags: feedback?(bzbarsky) → feedback+
Reporter | ||
Updated•11 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #747968 -
Attachment is obsolete: true
Attachment #757094 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•11 years ago
|
||
Suggested changes have been made, with the exception of removing the Optional<nsACString> specialisation. I was unable to get the code to compile without it (type casting issues).
Attachment #747979 -
Attachment is obsolete: true
Attachment #757095 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•11 years ago
|
||
This includes a behaviour change. Previously the changed strings were interpreted as UTF8 encoded, rather than the raw 8-bit format of ByteString. I've done a limited (linux64) try run, and fixed the test where this was assumed.
Attachment #757096 -
Flags: review?(bzbarsky)
Comment 10•11 years ago
|
||
Comment on attachment 757094 [details] [diff] [review] part 1. WebIDL parser changes to support ByteString v2 >+ if self.type.isByteString(): >+ raise WebIDLError("ByteStrings cannot have a default value", >+ [self.location]) Why is this needed? I would have expected coerceToType() to fail on this situation anyway.... > dom/imptests/WebIDLParser.js I don't think we want to change these files, since they're imported from another repository; they should be fixed there instead. Sorry I missed that the first time. Ms2ger, can you confirm? r=me with those issues dealt with.
Attachment #757094 -
Flags: review?(bzbarsky)
Attachment #757094 -
Flags: review?(Ms2ger)
Attachment #757094 -
Flags: review+
Comment 11•11 years ago
|
||
Comment on attachment 757095 [details] [diff] [review] part 2: Code Generation changes & tests v2 Was there a reason that using nsCString for the ByteString type did not work such that you ended up having to use nsACString? Again, if we just used nsCString we wouldn't need the extra Optional specialization and we could leverage more of the "normal" (as opposed to DOMString) Optional handling? There's also no need to put NonVoidByteStringToJsval and ByteStringToJsval under xpc stuff. Just put them in BindingUtils.h/cpp.
Attachment #757095 -
Flags: review?(bzbarsky) → review-
Comment 12•11 years ago
|
||
I'd like to get part 2 sorted through before worrying about part 3, though I suspect that even if the bindings use nsCString the callee can still use nsACString for both in and out params....
Reporter | ||
Comment 13•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #10) > Comment on attachment 757094 [details] [diff] [review] > part 1. WebIDL parser changes to support ByteString v2 > > > dom/imptests/WebIDLParser.js > > I don't think we want to change these files, since they're imported from > another repository; they should be fixed there instead. Sorry I missed that > the first time. Ms2ger, can you confirm? Indeed. Upstream is <https://github.com/w3c/testharness.js/>; would be great if you could create a pull request there.
Reporter | ||
Comment 14•11 years ago
|
||
Comment on attachment 757094 [details] [diff] [review] part 1. WebIDL parser changes to support ByteString v2 r- for the dom/imptests parts.
Attachment #757094 -
Flags: review?(Ms2ger) → review-
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #11) > Comment on attachment 757095 [details] [diff] [review] > part 2: Code Generation changes & tests v2 > > Was there a reason that using nsCString for the ByteString type did not work > such that you ended up having to use nsACString? Again, if we just used > nsCString we wouldn't need the extra Optional specialization and we could > leverage more of the "normal" (as opposed to DOMString) Optional handling? I wasn't able to get it to compile using nsCString and default Optional. The error message, and the generated code that produces the error message is attached.
Flags: needinfo?(bzbarsky)
Comment 16•11 years ago
|
||
> The error message, and the generated code that produces the error message is attached.
Thanks. The callee (in TestBindingHeader.h) should be declared as taking an Optional<nsCString> but isn't, is what that error is saying.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 18•11 years ago
|
||
Corrections have been made
Attachment #757094 -
Attachment is obsolete: true
Assignee | ||
Comment 19•11 years ago
|
||
The suggested change fixed the problems. Default Optional and nsCString version is attached.
Attachment #757095 -
Attachment is obsolete: true
Attachment #759661 -
Flags: review?(bzbarsky)
Flags: needinfo?(jkitch.bug)
Assignee | ||
Comment 20•11 years ago
|
||
The changes to part 2 didn't appear to require any changes to part 3. The updated version simply uses a new GUID for nsIXMLHttpRequest.
Attachment #757096 -
Attachment is obsolete: true
Attachment #757096 -
Flags: review?(bzbarsky)
Attachment #759663 -
Flags: review?(bzbarsky)
Comment 21•11 years ago
|
||
Note: you'll need to merge part 1 to the changes from bug 829248.
Comment 22•11 years ago
|
||
Comment on attachment 759661 [details] [diff] [review] part 2: Code Generation changes & tests v3 >+++ b/dom/bindings/BindingUtils.cpp >+NonVoidByteStringToJsval(JSContext *cx, const nsACString &str, JS::Value *rval) Might be better to use "JS::MutableHandle<JS::Value> rval" for the last argument, now that bug 877281 is fixed (though you'll need to update to pull in that fix). More on this below. >+ *rval = JS_GetEmptyStringValue(cx); And this would be rval.set(JS_GetEmptyStringValue(cx); >+ *rval = STRING_TO_JSVAL(jsStr); And rval.set(JS::StringValue(jsStr)); >+++ b/dom/bindings/BindingUtils.h >+ConvertJSValueToString(JSContext* cx, const JS::Value& v, JS::Value* pval, JS::Handle<JS::Value> v, JS::MutableHandle<JS::Value> pval Also, I don't expect this to be super-performance-sensitive, so might be better to not inline it. Also, maybe this should be ConvertJSValueToByteString? >+ if (nullable && (v.isNull() || v.isUndefined())) { if (nullable && v.isNullOrUndefined()) { >+ pval->setString(s); // Root the new string. pval.set(JS::StringValue(s)); // Root the new string >+ for (unsigned int i = 0; i < length; i++) { Probably better to make 'i' be of type size_t. >+ sprintf(index, "%d",i); Space before ",i", please. Also, worth commenting that even with a 64-bit size_t 20 chars is enough? And definitely using snprintf to make sure that if not enough for some odd reason bad things don't happen. >+ sprintf(badChar,"%d", chars[i]); Similar here: space after "badChar," and please us snprintf. >+ result.SetLength(uint32_t(length)); >+ if (result.Length() != uint32_t(length)) { >+ return false; >+ } Hmm. We need to make sure we have space for the null-terminator too... So what should probably happen here is making sure length is < UINT32_MAX, then SetCapacity(length + 1), writing into the string, putting a null char at result.BeginWriting()[length], and then SetLength(length). >+ JS_EncodeStringToBuffer(cx, s, result.BeginWriting(), length); uint32_t(length) for the last arg, since that's how much length you know you have to work with. >+ * Note, the ownership of the string buffer may be moved from str to rval. >+ * If that happens, str will point to an empty string after this call. Please remove those two lines; they aren't true for ByteString. >+inline bool ByteStringToJsval(JSContext *cx, const nsACString &str, JS::Value *rval) Again, MutableHandle. >+++ b/dom/bindings/Codegen.py >+ if type.isByteString(): >+ if type.nullable(): >+ nullable = "true" >+ else: >+ nullable = "false" nullable = toStringBool(type.nullable()) >+ "if (!ConvertJSValueToString(cx, ${val}, ${valPtr}, %s, %s)) {\n" Please replace ${valPtr} with ${mutableVal}. >+ "}" % (nullable,varName, exceptionCodeIndented.define())) Space after "nullable," >+ # We have to make a copy, because our jsval may well not >+ # live as long as our string needs to. Not relevant, since we already made a copy. You shouldn't need any differences here between the isMember case and not, and shouldn't need the getConversionCode bits. You should be able to just do: conversionCode = ( "if (!ConvertJSValueToString(cx, ${val}, ${mutableVal}, %s, ${declName})) {\n" "%s\n" "}" % (nullable, varName, exceptionCodeIndented.define())) return JSToNativeConversionInfo( conversionCode, declType=CGGeneric("nsCString"), dealWithOptional=isOptional) >+ return (wrapAndSetPtr("ByteStringToJsval(cx, %s, ${jsvalPtr})" % result), False) That should now be ${jsvalHandle} instead of ${jsvalPtr} if ByteStringToJsval takes a MutableHandle. (Yes, I know the DOMString case hasn't gotten fixed yet...) >+ return (wrapAndSetPtr("NonVoidByteStringToJsval(cx, %s, ${jsvalPtr})" % result), False) And here. >+++ b/dom/bindings/test/TestBindingHeader.h > static > already_AddRefed<TestInterface> >+ Constructor(const GlobalObject&, const nsACString&, ErrorResult&); This is unused. Please drop it. Thank you for adding tests! r=me with the above fixed, though I'd appreciate an interdiff showing the changes.
Attachment #759661 -
Flags: review?(bzbarsky) → review+
Comment 23•11 years ago
|
||
Comment on attachment 759663 [details] [diff] [review] part 3: Convert XMLHttpRequest to use ByteString v2 >+++ b/content/base/src/nsXMLHttpRequest.h >+ aRv = SetRequestHeader(aHeader,aValue); Space after ',', please. >+++ b/dom/bindings/test/TestBindingHeader.h >+++ b/dom/bindings/test/TestCodeGen.webidl >+++ b/dom/bindings/test/TestExampleGen.webidl >+++ b/dom/bindings/test/TestJSImplGen.webidl These changes should be in part 2, right? Also, in TestJSImplGen, please put receiveByteStringSequence() after the commented-out passStringSequence()? r=me with that. This is awesome. :)
Attachment #759663 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 24•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #22) > >+ *rval = STRING_TO_JSVAL(jsStr); > > And rval.set(JS::StringValue(jsStr)); rval.setString(jsStr); please.
Assignee | ||
Comment 25•11 years ago
|
||
Suggested changes have been made. jsvalHandle wasn't defined, so you might want to check that my implementation is what was intended.
Attachment #760194 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 27•11 years ago
|
||
Interdiff changes folded in.
Attachment #759661 -
Attachment is obsolete: true
Assignee | ||
Comment 28•11 years ago
|
||
Suggested changes have been made.
Attachment #759663 -
Attachment is obsolete: true
Reporter | ||
Comment 29•11 years ago
|
||
Comment on attachment 760194 [details] [diff] [review] interdiff of code generation changes Review of attachment 760194 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +3901,5 @@ > 'result' : "%s[%s]" % (result, index), > 'successCode': "break;", > 'jsvalRef': "tmp", > 'jsvalPtr': "tmp.address()", > + 'jsvalHandle': "JS::MutableHandle<JS::Value>::fromMarkedLocation(tmp.address())", You should just be "&tmp" @@ +7250,5 @@ > } > }""" > > + templateValues = {'jsvalRef': '*vp.address()', 'jsvalPtr': 'vp.address()', > + 'jsvalHandle' : 'JS::MutableHandle<JS::Value>::fromMarkedLocation(vp.address())', &vp @@ +7337,5 @@ > def getBody(self): > successCode = ("*present = found;\n" > "return true;") > templateValues = {'jsvalRef': '*vp.address()', 'jsvalPtr': 'vp.address()', > + 'jsvalHandle': 'JS::MutableHandle<JS::Value>::fromMarkedLocation(vp.address())', &vp @@ +7961,5 @@ > "}\n" > "break;" % propDef), > 'jsvalRef': "temp", > 'jsvalPtr': "temp.address()", > + 'jsvalHandle': "JS::MutableHandle<JS::Value>::fromMarkedLocation(temp.address())", &temp @@ +9576,5 @@ > 'result' : result, > 'successCode' : "continue;" if arg.variadic else "break;", > 'jsvalRef' : "argv[%s]" % jsvalIndex, > 'jsvalPtr' : "&argv[%s]" % jsvalIndex, > + 'jsvalHandle' : "JS::MutableHandle<JS::Value>::fromMarkedLocation(&argv[%s])" % jsvalIndex, argv.handleAt(%s) should work, I think
Assignee | ||
Comment 30•11 years ago
|
||
jsvalHandle definitions have been corrected
Attachment #760197 -
Attachment is obsolete: true
Comment 31•11 years ago
|
||
> jsvalHandle wasn't defined Which revision are you basing this on? It should be defined if your tree has the fix for bug 877281 (note that you need to pull inbound for that; it hasn't gotten merged to m-c yet).
Assignee | ||
Comment 32•11 years ago
|
||
My code is based off Mozilla-Central. I didn't realise it hadn't landed yet. A revised patch will be posted once it does.
Assignee | ||
Updated•11 years ago
|
Attachment #760194 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 34•11 years ago
|
||
Now uses the changes from bug 877281
Attachment #760194 -
Attachment is obsolete: true
Attachment #760348 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 35•11 years ago
|
||
Sorry, the last one was missing some changes.
Attachment #760348 -
Attachment is obsolete: true
Attachment #760348 -
Flags: feedback?(bzbarsky)
Attachment #760349 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 36•11 years ago
|
||
Interdiff folded into a slightly modified version of attachment 759663 [details] [diff] [review] (changed only enough for the patch to apply cleanly. Changes to make it compile were in the interdiff).
Attachment #760207 -
Attachment is obsolete: true
Comment 37•11 years ago
|
||
Comment on attachment 760349 [details] [diff] [review] rebased interdiff of code generation changes >+++ b/dom/bindings/BindingUtils.cpp >+// pval must not be null and must point to a rooted JS::Value This comment should go; MutableHandle<Value> tells us all we need to know about pval. >+ConvertJSValueToByteString(JSContext* cx, JS::Handle<JS::Value> v, >+ JS::MutableHandle<JS::Value> pval, bool nullable, >+ nsACString& result) Please fix the indent. >+ _snprintf(index, 21, "%d", i); I forgot that Windows does weird naming here, right. What you want is: PR_snprintf(index, sizeof(index), "%d", i); and perhaps a static assert that sizeof(size_t) <= 8. >+ // A UTF-16 character is up to 32 bits long. Actually, a jschar is 16 bits in size, for what it's worth. >+ JS_EncodeStringToBuffer(cx, s, result.BeginWriting(), uint32_t(length)); >+ result.SetLength(uint32_t(length)); Shouldn't need the casts here, since we made sure it's in the right range, right? >+++ b/dom/bindings/BindingUtils.h >+ConvertJSValueToByteString(JSContext* cx, JS::Handle<JS::Value> v, >+ JS::MutableHandle<JS::Value> pval, bool nullable, >+ nsACString& result); Please fix the indent here too. >+ rval.set(JSVAL_NULL); I _think_ rval.setNull() should work here. >+++ b/dom/bindings/Codegen.py >+ # ByteString arguments cannot have a default value. assert defaultValue is None r=me with those fixed; this looks fantastic!
Attachment #760349 -
Flags: review+
Attachment #760349 -
Flags: feedback?(bzbarsky)
Attachment #760349 -
Flags: feedback+
Assignee | ||
Comment 38•11 years ago
|
||
Attachment #760350 -
Attachment is obsolete: true
Comment 39•11 years ago
|
||
James, it looks like this is ready to go, right? Do you want me to push the last patches here to try?
Flags: needinfo?(jkitch.bug)
Assignee | ||
Comment 40•11 years ago
|
||
Yes, it should be ready. I can do the push myself, but I won't have access to the appropriate computer for another 7 hours. Otherwise how exhaustive should the try run be?
Flags: needinfo?(jkitch.bug)
Comment 41•11 years ago
|
||
Another 7 hours is not a problem, so I'll leave that to you. ;) Doing a debug run on one platform is probably enough, I would think...
Assignee | ||
Comment 42•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=7fb933609efe Green, other than a crash that appears to be a known problem.
Comment 43•11 years ago
|
||
Yep, looks good. Can you push to inbound yourself, or do you want me to?
Flags: needinfo?(jkitch.bug)
Assignee | ||
Comment 44•11 years ago
|
||
Could you please do it? I don't have that level of commit access.
Flags: needinfo?(jkitch.bug)
Comment 45•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/864acd590539 https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb618e6b57c https://hg.mozilla.org/integration/mozilla-inbound/rev/708f6cd81acb Thank you again for the patches!
Flags: in-testsuite+
Target Milestone: --- → mozilla24
Comment 46•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/864acd590539 https://hg.mozilla.org/mozilla-central/rev/2cb618e6b57c https://hg.mozilla.org/mozilla-central/rev/708f6cd81acb
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 47•11 years ago
|
||
Follow up for documentation: https://bugzilla.mozilla.org/show_bug.cgi?id=885275
Keywords: dev-doc-needed → doc-bug-filed
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•