Closed
Bug 972593
Opened 10 years ago
Closed 10 years ago
Convert nsStreamConverterService::mAdjacencyList to a more modern hashtable
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(2 files, 3 obsolete files)
13.11 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
929 bytes,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
This looks slightly tricky, but maybe only because I don't really understand how the strings should be created or copied around.
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ecd2e68bc23c
Assignee: nobody → continuation
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
I need to look over this a bit more to convince myself all the string stuff I do is right.
Assignee | ||
Comment 5•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4d406b9ca8ec
Attachment #8392991 -
Attachment is obsolete: true
Attachment #8392992 -
Attachment is obsolete: true
Attachment #8409434 -
Flags: review?(mcmanus)
Comment 6•10 years ago
|
||
Comment on attachment 8409434 [details] [diff] [review] Convert nsStreamConverterService hashtables to something modern. Review of attachment 8409434 [details] [diff] [review]: ----------------------------------------------------------------- sorry about the lag here..
Attachment #8409434 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 7•10 years ago
|
||
No problem, there's not a huge hurry on these removals. Thanks for the review! https://hg.mozilla.org/integration/mozilla-inbound/rev/ac6c395d9364
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ac6c395d9364
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 10•10 years ago
|
||
Comment on attachment 8409434 [details] [diff] [review] Convert nsStreamConverterService hashtables to something modern. Review of attachment 8409434 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/streamconv/src/nsStreamConverterService.cpp @@ +271,5 @@ > nsIAtom* curVertexAtom = data2->ObjectAt(i); > nsAutoString curVertexStr; > curVertexAtom->ToString(curVertexStr); > + nsCString *curVertex = nullptr; > + CopyUTF16toUTF8(curVertexStr, *curVertex); Somehow, I suspect this is very, very, very, very wrong.
Assignee | ||
Comment 11•10 years ago
|
||
Thanks for pointing out the error, Joshua! I suppose it isn't worth doing a try run, given that the current code will crash every time it is run.
Attachment #8416310 -
Flags: review?(mcmanus)
Updated•10 years ago
|
Comment 12•10 years ago
|
||
Here's an alternative part 2 if you want it. It makes use of the ToUTF8String function of nsIAtom to reduce the LoC a bit. I've tested locally that it fixes bug 1004658.
Attachment #8416382 -
Flags: review?(mcmanus)
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8416382 -
Attachment is patch: true
Comment 13•10 years ago
|
||
please decide which patch you want and set 1 review flag.
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8416310 [details] [diff] [review] part 2 - Don't deref an always-null pointer in nsStreamConverterService::FindConverter. The other one is better.
Attachment #8416310 -
Attachment is obsolete: true
Attachment #8416310 -
Flags: review?(mcmanus)
Comment 15•10 years ago
|
||
Comment on attachment 8416382 [details] [diff] [review] Alternative part 2 Review of attachment 8416382 [details] [diff] [review]: ----------------------------------------------------------------- sorry i didn't notice this the first time around. thanks for the fix
Attachment #8416382 -
Flags: review?(mcmanus) → review+
Comment 16•10 years ago
|
||
Thanks for the review, landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/865805d83d5a
You need to log in
before you can comment on or make changes to this bug.
Description
•