Closed Bug 1497513 Opened 2 years ago Closed 2 years ago
Want to make some filter methods scriptable
I'd liked to call some filter methods but they've been marked as [noscript] because of the non-standard string passing convention that they use. Fortunately the problem only prevents passing strings from C++ to JS but passing strings from JS to C++ is not affected so this isn't necessary.
Assignee: nobody → neil
Attachment #9015527 - Flags: review?(acelists)
Comment on attachment 9015527 [details] [diff] [review] Proposed patch Review of attachment 9015527 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure about this stuff, Magnus could know better.
Attachment #9015527 - Flags: review?(acelists) → review?(mkmelin+mozilla)
Or an array? M-C offer arrays in IDL these days, see an example here: https://hg.mozilla.org/mozilla-central/rev/a33fb1d3cda0#l3.19
Good idea. Commas may also be problematic as they could occur in the data. Maybe we could activate the '[array, size_is(headerSize)] in string headers,' arguments (make the headers an array), which somebody seems to have already thought about in the past. Neil, would you be able to do that (remove the single string argument and make it an array) ? And would it then be usable from JS?
It is seen in the patch itself: - // Marking noscript because "headers" is actually a null-separated - // list of headers, which is not scriptable. - [noscript] void MatchHdr(in nsIMsgDBHdr msgHdr, in nsIMsgFolder folder, + void MatchHdr(in nsIMsgDBHdr msgHdr, in nsIMsgFolder folder, in nsIMsgDatabase db, in string headers, -> // [array, size_is(headerSize)] in string headers, in unsigned long headerSize, out boolean result);
Yeah comma separation would not work if it's the complete header. (I read it as the header name, not the complete header line).
The MatchHdr() calls into other functions 10 levels deep (which all would need a change) passing this 'headers' arguments. I'm also not sure whether it only contains header names or values too (or the full msgs header lines), as e.g. in https://dxr.mozilla.org/comm-central/source/comm/mailnews/base/search/src/nsMsgBodyHandler.cpp#146 it still does a lot of parsing of the m_headers string.
Given what acemen said (comment 8), can we land the patch as-is, and make a followup bug to make this nicer with arrays? From what I understand, Neil's patch is a direct improvement, i.e. doesn't regress anything that already works, and makes new things work that didn't work before.
... you can't argue that you can't have nulls in ACStrings, because you can.
Comment on attachment 9016715 [details] [diff] [review] Use ACString I think ACString is a good idea. I've browsed over the patch and it looks sane to me. I think this is an improvement over status quo.
@Neil: Could you please state here what you did to manually test this latest ACString patch and to ensure that it doesn't break anything? Could you please, in addition to manual tests, also push a try run?
Try run is green: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=78219475a2aacc33c6b68865620f46bbe261ff21 Sadly we had an influx of Linux MozMill bustage and telemetry issues, but we're on the way to addressing those.
Not the ideal solution, but it will do for now. I have no idea why Substring was used at three call sites, unless I'm missing something it should be nsDependentCString in all three cases.
Pushed by email@example.com: https://hg.mozilla.org/comm-central/rev/8aebb2ed9307 make some filter methods scriptable, replace string with ACString for 'headers' parameter. r=jorgk
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
I did mess this up :-( - crashes in debug mode with a message that nsDependentCSubstring() should be used. Sorry.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/comm-central/rev/6dda9a7770fc fix incorrect use of nsDependentCString. r=me
(In reply to Jorg K from comment #14) > Try run is green: > https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=78219475a2aacc33c6b68865620f46bbe261ff21 Ah, you found it ;-) I would have posted it but I had a stomach ache. (In reply to Jorg K from comment #15) > Not the ideal solution, but it will do for now. I have no idea why Substring > was used at three call sites, unless I'm missing something it should be > nsDependentCString in all three cases. Those three C++ callers don't pass null-terminated strings (well, the last character of the string is null, but that's not what nsDependentCString is looking for). Substring is slightly less typing and slightly more type-safe than nsDependent(C)Substring.
You'll handle the uplift request here, right? Well, initially I wanted to use nsDependentCString to avoid making copies of strings. I still think nsDependentCSubstring is the better solution here, since Substring(char*, ...) would have promoted the raw string to a "smart" string first, no?
Neil says that both are equivalent after compile, but Substring() is type-safe during compile/development, so strictly better. But for the same reason, it doesn't make much of a difference. We can leave it as-is. Just for the next time.
Attachment #9017786 - Flags: approval-comm-esr60? → approval-comm-esr60+
You need to log in before you can comment on or make changes to this bug.