fixupURISpec returns an nsString, but this results in us allocating said string three times (once in function, once by generating a temporary for returning, and once for the return variable). Ouch. We should just make it an out param. Also, all the calls to GetString end up copying. We really want to just call GetSharedString and store it in the dependent string ourselves. Less string copying will make this a wee bit faster.
In fact, profiling shows that calling GetString (because it calls Assign) accounts for ~1% of the time in the function! Yikes!
Created attachment 389023 [details] [diff] [review] v1.0
Comment on attachment 389023 [details] [diff] [review] v1.0 >- nsString fixedSpec; >- if (IsUTF8(unescapedSpec)) >- CopyUTF8toUTF16(unescapedSpec, fixedSpec); >+ NS_ASSERTION(_fixedSpec.IsEmpty(), >+ "Passing a non-empty string as an out parameter!"); >+ if (!IsUTF8(unescapedSpec)) >+ CopyUTF8toUTF16(unescapedSpec, _fixedSpec); way to invert your logic, shawn :) r=dwitte with that fixed.
Attachment #389023 - Flags: review?(dwitte) → review+
Looks like I just forgot to qrefresh that logic error. Fixed locally.
Comment on attachment 389023 [details] [diff] [review] v1.0 r=me w/ dwitte's change.
Attachment #389023 - Flags: review?(dietrich) → review+
This landed, but was backed out once I built in a debug build and hit assertions. The trivial fix passed tests (I'll attach that shortly), so this is going to require more work. I'll do that /after/ I fix all the blockers for bug 455555.
Created attachment 390061 [details] [diff] [review] v2.0 Like I said - this doesn't work right.
Attachment #389023 - Attachment is obsolete: true
All of the dependent string stuff is actually a bad idea - violates the SQLite API. Sadfaces.
Created attachment 392326 [details] [diff] [review] v3.0 This passes tests and uses the string API to reduce some coping and malloc's when possible.
Attachment #392326 - Flags: review? → review?(dietrich)
Comment on attachment 392326 [details] [diff] [review] v3.0 r=me
Attachment #392326 - Flags: review?(dietrich) → review+
Whiteboard: [needs review dietrich] → [can land]
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → mozilla1.9.2b1
You need to log in before you can comment on or make changes to this bug.