211181 - [FIXr]URI fixup should work with UTF8 strings

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Patch coming up.  This makes the code a little saner in a few places...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126795 [details] [diff] [review]
Proposed patch

smontagu has tested this and it does not regress bug 58866

Comment 3

[Darin Fisher]

Comment on attachment 126795 [details] [diff] [review]
Proposed patch

this looks ok to me.  sr=darin with the following caveat...

as for the PossiblyByteExpandedFileName code... think about it this way.  if
native charset code were zero padded, it would still look like valid UTF-16. 
moreover, a conversion of such to UTF-8 would be well defined.	as such, the
conversion back from UTF-8 to UTF-16 could very well reintroduce characters
that are possibly byte expanded, so i think you should do away with your XXX
comment unless someone can confirm that we never see incorrectly formated
strings like this.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I had simon test, and we do see incorrect strings like that.

I'd like to change the XXX comment to say that the code _should_ be removed,
once bug xxxxx is fixed.  And I guess I should file that bug on intl?  Simon?

Comment 5

[Adam Lock]

Comment on attachment 126795 [details] [diff] [review]
Proposed patch

Aside from the xxx comment, it looks okay to me too.

r=adamlock

Comment 6

[Boris Zbarsky [:bzbarsky]]

The XXX comment I checked in:

        // XXXbz nsICmdLineService doesn't hand back unicode, so in some cases
        // what we have is actually a "utf8" version of a "utf16" string that's
        // actually byte-expanded native-encoding data.  Someone upstream needs
        // to stop using AssignWithConversion and do things correctly.  See bug
        // 58866 for what happens if we remove this
        // PossiblyByteExpandedFileName check.

jshin?  smontagu?  Any ideas on where the perfidious non-converting code lives?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 8

[[e-mail]]

This appears to have killed one of the camino builds:

src/browser/BrowserWindowController.mm: In function `-[BrowserWindowController
goToLocationFromToolbarURLField:]':
src/browser/BrowserWindowController.mm:1175: no matching function for call to
`nsIURIFixup::CreateFixupURI(nsAutoString&, int, nsGetterAddRefs<nsIURI>)'
/builds/tinderbox/SeaMonkey/Darwin_6.6_Depend/mozilla/dist/include/docshell/nsIURIFixup.h:73:
candidates are: virtual nsresult nsIURIFixup::CreateFixupURI(const nsACString&,
unsigned int, nsIURI**)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Camino attempted fix checked in.