803188 - Provide a way to convert from a file path to a file:// URI (and vice-versa)

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Mihai Sucan [:msucan]]

Today I worked with the OS.File API and I found that I have to use NetUtil/FileUtils to go from a local file path to a file:// URI path.

Given:

  path = OS.Path.join(OS.Constants.Path.tmpDir, "foo.html");

I have to do:

  file = new FileUtils.File(path);
  uri = NetUtil.newURI(file).spec;


On a similar note: I'd like a way to convert chrome:// URIs to file:// (and vice-versa). I needed this in other pieces of code I did some time ago (before OS.File).

Comment 1

[David Teller [:Yoric] - still alive but not very active]

We could add |OS.Path.toFileURI|.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

I don't think that we can do chrome:// [yet], but let us concentrate on file://

The algorithm used in NetUtil is defined at:
- http://dxr.mozilla.org/mozilla-central/netwerk/base/src/nsURLHelperUnix.cpp.html#l14 (for Unix)
- http://dxr.mozilla.org/mozilla-central/netwerk/base/src/nsURLHelperWin.cpp.html#l14 (for Windows)

It should be quite easy to port it to JavaScript, as part of ospath_win.jsm/ospath_unix.jsm.

Comment 3

[WangJun]

Can I get assigned this bug?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(In reply to WangJun from comment #3)
> Can I get assigned this bug?

Certainly.
If you have any question, feel free to contact me.

Comment 5

[(dormant account)]

Why is it bad to use FileUtils/NetUtil in this case? Duplicating chrome:// path resolution is ~impossible since it's implemented with a healthy dose of extensible XPCOM.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #5)
> Why is it bad to use FileUtils/NetUtil in this case?

The main reason is that FileUtils/NetUtil can only be used from the main thread.

Now, there are a few secondary reasons, in particular the fact that having to use two additional modules just to convert to/from a file:// path for use with OS.File, makes for a poor developer experience.

> Duplicating chrome://
> path resolution is ~impossible since it's implemented with a healthy dose of
> extensible XPCOM.

I have no intention to duplicate chrome:// resolution, which is generally unfeasible as you mention, just file://, which should be quite simple.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

WangJun, after your message on IRC, I feel that I need to clarify one thing: the objective of this bug is *not* to wrap the C++ function into something that can be called from JS.

What we want here is to implement the algorithm in pure JavaScript. I can be wrong, but I believe that this can be done in ~10 lines for each conversion for Unix and probably about the same number for Windows. I provided the links to C++ source code so that you could check how the algorithm looks like in C++.

Basically, converting file path to file:// URI:
- use OS.Path.split to divide the path into its components;
- encode each component with built-in JavaScript function |encodeURIComponent|;
- concatenate back everything.

Converting file:// URI to file path:
- remove "file://";
- use String split to divide the uri into its components;
- decode each component with built-in JavaScript function |decodeURIComponent|;
- concatenate back everything.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

WangJun, are you still interested in working on that bug?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

In the absence of reply, resetting the bug. If anyone is interested, please take it :)

Comment 11

[Keng Kiat]

Hi, this is my first time contributing to OSS. Would like to get assigned to this bug.

Thank you!

Comment 12

[David Teller [:Yoric] - still alive but not very active]

With pleasure, kkos. Have fun!
If you have any question, do not hesitate to come to IRC (server irc.mozilla.org, channel #introduction or, equivalently, https://irc.lc/mozilla/introduction/kkos ).

Comment 13

[Keng Kiat]

Attachment: Added functions to both windows and unix that will convert local filre path to URI.

I'm not sure how to write tests. Any advice?

Comment 14

[Benjamin Smedberg]

I'm not sure that this duplication is a good idea, but I believe that this patch should be reviewed by a necko peer in any case, and we should note in both locations that any changes to the code should be duplicated in the other path.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #14)
> I'm not sure that this duplication is a good idea, but I believe that this
> patch should be reviewed by a necko peer in any case, and we should note in
> both locations that any changes to the code should be duplicated in the
> other path.

Not sure. It's not a feature duplication (nsIFile doesn't work on worker threads), it's not part of Necko, and it doesn't deal with chrome:// uris.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Oh, and Necko makes the mistake of believing Microsoft's documentation when it states that paths cannot be longer than MAX_PATH. That's probably something we'll want to avoid with OS.Path/OS.File. But that's another story.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 709014 [details] [diff] [review]
Added functions to both windows and unix that will convert local filre path to URI.

Review of attachment 709014 [details] [diff] [review]:
-----------------------------------------------------------------

This may works, but it definitely needs tests.

You can find more information about how to write simple tests here: https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests

I would check that nsIIOService and your code return the same file:// uri for a variety of paths.

Also, I revise my earlier reply to bsmedberg. Benjamin, if you believe that a Necko peer should look at the algorithm, could you suggest someone? I don't think we need a full review from a peer, but having some feedback would be interesting.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Any progress, Keng Kiat?

Comment 19

[Keng Kiat]

Hi David,

Based on the discussion in IRC previously, I'm required to implement the method to convert the file path the the uri format, and vice-versa. While that is done, I have some trouble passing some tests

For example ~/Documents does not translate to file:///Users/username/Documents/. Instead, I got file:///~/Documents/. So I'm trying to find a way to get that parsed.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Ah, good point. I didn't realize that nsIIOService handled "~". It probably shouldn't, so this is ok if you do not handle tests that use symbol "~".

Comment 21

[Keng Kiat]

The other problem is in test case '/test\ test/', my unit test would throw an exception. Currently I'm using FileUtils.File() to compare against my function. I suspect that FileUtils.File() cannot handle the space character. Any suggestions?

Comment 22

[David Teller [:Yoric] - still alive but not very active]

That's surprising. Can you double-check that |FileUtils.File()| fails with files that contain a space in the name?

Btw, don't hesitate to tick "Need more information from [me]", this will ensure that I don't forget replying to your questions.

Comment 23

[Keng Kiat]

Ok, I've found the problem. Apparently FileUtils.File() will fail when i supply a path that doesn't start with a '/'.

However, I'm now facing another problem when testing the conversion from file URI back to file path. Currently, I'm using iOService.newURI() to create a URI object, and comparing my code with URI.spec. However, when testing with this test case: "file:///test%20test/", iOService.newURI() does not decode the URI into its special characters (In this case a space character). I believe that the decoding functionality must be included in this conversion process since file names can contain spaces. Is there a better way to test this?

Comment 24

[Masatoshi Kimura [:emk]]

iOService.newFileURI()?

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Ah, it's a bit tricky indeed.
So, once you have your nsIURI, you will need to convert it to a nsIFileURL, as follows:
   let myFileURL = myURI.QueryInterface(nsIFileURL)

Interface nsIFileURL should contain everything you need:
   https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIFileURL

Comment 26

[Asaf Romano (gone)]

(FWIW, You don't need the extra variable. you could just do myURI.QueryInterface(nsIFileURL); myURI.QueryInterface(nsIFileURL); myURI.WhatEeverFileURIProvices();)

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Any news, Keng Kiat?

Comment 28

[Keng Kiat]

I've done up both methods, and tested on a Unix machine, now I have to test it on a Windows machine...

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Keng Kiat, if you are satisfied with your test suite and if you do not have access to a Windows machine, I can send your patch to one of our Windows unit testing servers for automated testing.

Comment 30

[Keng Kiat]

Attachment: Bug Fixes, tests located in other patch

I have tested this in Unix (Mac). However I am unable to perform test on my Windows VM. I have written tests for both environments. Tests are located in the other patch.

Comment 31

[Keng Kiat]

Attachment: Test cases for the fixes

Comment 32

[David Teller [:Yoric] - still alive but not very active]

You'll find the results of testing for Linux, MacOS and Windows here: https://tbpl.mozilla.org/?tree=Try&rev=a466bf5be4c5

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 746436 [details] [diff] [review]
Bug Fixes, tests located in other patch

Review of attachment 746436 [details] [diff] [review]:
-----------------------------------------------------------------

Good start.
Oh, by the way, you should change the commit message of your patch to something along the lines of
"Bug 803188 - Conversion between file paths and file:/// uris;r=yoric"

::: toolkit/components/osfile/ospath_unix_back.jsm
@@ +135,5 @@
> +     /**
> +      * Return the file:// URI file path of the given local file path.
> +      */
> +     filePathToFileURI: function filePathToFileURI(path) {
> +       

Nit: Please avoid these whitespaces.
(you can find them easily by following the "review" link on bugzilla)

@@ +137,5 @@
> +      */
> +     filePathToFileURI: function filePathToFileURI(path) {
> +       
> +       // Remove trailing slashes
> +       if(path.substr(-1) === '/' && path.length > 1) {

Nit: Please add a whitespace after |if|.

@@ +139,5 @@
> +       
> +       // Remove trailing slashes
> +       if(path.substr(-1) === '/' && path.length > 1) {
> +         path = path.substr(0, path.length - 1);
> +        }

That won't work if you have several trailing slashes.
I seem to remember that |normalize| will do what you want, though.

@@ +141,5 @@
> +       if(path.substr(-1) === '/' && path.length > 1) {
> +         path = path.substr(0, path.length - 1);
> +        }
> +       
> +       var prefix = "file://";

Please use |let| instead of |var| in this file.

@@ +150,5 @@
> +       uri = prefix + uri.replace(/;/g, "%3b")
> +                         .replace(/\?/g, "%3F");
> +
> +       return uri;
> +     },

Please also document this function.

@@ +153,5 @@
> +       return uri;
> +     },
> +     fileURIToFilePath: function fileURIToFilePath(uri) {
> +       
> +       if(uri.indexOf("file://localhost/") === 0) {

Interesting. I had no idea that this should work.

@@ +162,5 @@
> +       
> +       // strip trailing slashes
> +       if(uri.charAt(uri.length - 1) === '/' && uri.length > 1) {
> +         uri = uri.substr(0, uri.length - 1);
> +       }

If possible, I would let |normalize| handle these trailing spaces.

::: toolkit/components/osfile/ospath_win_back.jsm
@@ +246,5 @@
> +       
> +       // Remove trailing slashes
> +       if(path.substr(-1) === '\\' && path.length > 3) {
> +         path = path.substr(0, path.length - 1);
> +       }

Generally, same remarks as Unix version.

::: toolkit/components/osfile/tests/xpcshell/xpcshell.ini
@@ +9,5 @@
> +[test_file_URL_conversion_unix.js]
> +skip-if = os == 'win'
> +
> +[test_file_URL_conversion_win.js]
> +run-if = os == 'win'
\ No newline at end of file

Please move this to the other patch.
Also, please add a newline at the end of the file.

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 746437 [details] [diff] [review]
Test cases for the fixes

Review of attachment 746437 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

::: toolkit/components/osfile/tests/xpcshell/test_file_URL_conversion_unix.js
@@ +3,5 @@
> +  // this is just an example, so we assert that true == true
> +  Components.utils.import("resource://gre/modules/Services.jsm");
> +  Components.utils.import("resource://gre/modules/osfile.jsm");
> +  Components.utils.import("resource://gre/modules/FileUtils.jsm");
> +  

As in the other patch, please avoid whitespaces, please use |let|.

@@ +4,5 @@
> +  Components.utils.import("resource://gre/modules/Services.jsm");
> +  Components.utils.import("resource://gre/modules/osfile.jsm");
> +  Components.utils.import("resource://gre/modules/FileUtils.jsm");
> +  
> +  var iOService = Services.io;

You don't really need that shortcut.

@@ +18,5 @@
> +    '/test\ test'
> +    );
> +  
> +  // Empty array to be used later for fileURIToFilePath tests
> +  uriStringList = new Array();

You should use |[]| instead of |new Array()|.

@@ +21,5 @@
> +  // Empty array to be used later for fileURIToFilePath tests
> +  uriStringList = new Array();
> +  
> +  // Testing of filePathToURI
> +  for(var key in pathStringList) {

Please add a whitespace after |for|.

::: toolkit/components/osfile/tests/xpcshell/test_file_URL_conversion_win.js
@@ +14,5 @@
> +    'C:\\test\\',
> +    'C:\\test\\test\\test',
> +    'C:\\test;+%',
> +    'C:\\test?action=index\\',
> +    'C:\\test\ test'

You should also check with UNC-formatted paths, e.g.
\\C:\a\b\c
\\Server\a\b\c

Comment 35

[David Teller [:Yoric] - still alive but not very active]

I haven't seen any updates to this bug in some time. What's your status? Do you need assistance?

Comment 36

[Keng Kiat]

Attachment: bug803188.patch

Updated changes. There may be an issue with the window's version of normalise. Please see the comments in the code.

Comment 37

[Keng Kiat]

Attachment: bug803188test.patch

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 788671 [details] [diff] [review]
bug803188.patch

Review of attachment 788671 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
r=me with the following changes, if it passes tests

By the way, the files you are patching have changed a little (in particular, they have changed name), so you will need to fix conflicts next time you pull.

::: toolkit/components/osfile/modules/ospath_unix_back.jsm
@@ +151,5 @@
> +     },
> +     /**
> +      * Returns the file:// URI file path of the given local file path.
> +      */
> +     filePathToFileURI: function filePathToFileURI(path) {

Given that the module name is already |Path|, we can probably simplify the name of these functions to |fromFileURI| and |toFileURI|.

@@ +170,5 @@
> +       if (uri.indexOf("file://localhost/") === 0) {
> +         uri = uri.substr(16, uri.length);
> +       } else if (uri.indexOf("file:///") === 0) {
> +         uri = uri.substr(7, uri.length);
> +       }

Rather than using magical constants 7 and 16, I suggest storing "file://localhost/" and "file:///" as constant strings and using their length.

::: toolkit/components/osfile/modules/ospath_win_back.jsm
@@ +300,5 @@
> +       // encodeURI doesn't escape semicolons. So we need to manually
> +       // escape them.
> +       uri = prefix + uri.replace(/;/g, "%3b")
> +                         .replace(/\?/g, "%3F");
> +

Could you add a comment explaining this final "/"? I assume this is for transforming "file:///C:" into "file:///C:/", isn't it?

@@ +324,5 @@
> +                                 .replace(/\//g, "\\"));
> +
> +       // this.normalize() does not remove the trailing slash if the path
> +       // component is a drive letter. eg. 'C:\'' will not get normalized.
> +       if (path.substr(path.length - 2, path.length) === ":\\") {

Simply |if (path.endsWith(":\\") {|

Comment 39

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 788672 [details] [diff] [review]
bug803188test.patch

Review of attachment 788672 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.
Don't forget to rename your patch according to our conventions. The commit message should look like:
"Bug 803188 - Testing conversion between paths and file:// URIs;r=yoric"

Comment 40

[David Teller [:Yoric] - still alive but not very active]

How are things going Keng Kiat? I seem to remember that your work is almost complete, so it would be nice to land it.

Comment 41

[David Teller [:Yoric] - still alive but not very active]

It seems that Keng Kiat is not workig on this anymore. However, the bug is almost ready, if anybody wants to unbitrot and land.

Comment 42

[Dustin J. Mitchell [:dustin] (he/him)]

I have these patches un-bitrotten and squashed into a single patch.  I'll work to apply the review comments then submit for one more round of r?.  I'm a little confused about the renaming suggestions in comment 39, as the patch there already has comment "Bug 803188 - Tests for conversion between file paths and file:/// uris;r=yoric".  Are you suggesting that the patch filename in Bugzilla have that form?  Or is the distinction "Tests for" vs. "Testing"?

Is it OK to put the tests and functionality in the same patch?

Does this change need corresponding documentation in-tree, or does that go into MDN?

Comment 43

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug803188.patch

Here it is at any rate.  I did a little bit more refactoring to do the prefix checks in a loop, and noted the difference between UNIX and Windows as far as stripping the trailing slash of the prefix.

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 824799 [details] [diff] [review]
bug803188.patch

Review of attachment 824799 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the regexp change

::: toolkit/components/osfile/modules/ospath_unix.jsm
@@ +160,5 @@
> +
> +  // encodeURI doesn't escape semicolons. So we need to manually
> +  // escape them.
> +  uri = prefix + uri.replace(/;/g, "%3b")
> +                    .replace(/\?/g, "%3F");

We should probably do the replacement in one pass. Something along the lines of:
  uri.replace(/[;\?]/, match => (match == ":")?"%3b":"%3F)

@@ +182,5 @@
> +
> +  let path = decodeURI(this.normalize(uri));
> +  path = path.replace(/%3b/g, ";")
> +             .replace(/%3F/g, "?");
> +

As above.

Comment 45

[David Teller [:Yoric] - still alive but not very active]

(In reply to Dustin J. Mitchell [:dustin] (I read my bugmail; don't needinfo me) from comment #42)
> I have these patches un-bitrotten and squashed into a single patch.  I'll
> work to apply the review comments then submit for one more round of r?.  I'm
> a little confused about the renaming suggestions in comment 39, as the patch
> there already has comment "Bug 803188 - Tests for conversion between file
> paths and file:/// uris;r=yoric".  Are you suggesting that the patch
> filename in Bugzilla have that form?  Or is the distinction "Tests for" vs.
> "Testing"?

I don't remember why I put that comment.

> Is it OK to put the tests and functionality in the same patch?

For such a small patch, this is ok.

> Does this change need corresponding documentation in-tree, or does that go
> into MDN?

Well, some MDN doc will be needed

Comment 46

[Dustin J. Mitchell [:dustin] (he/him)]

I'm curious why the URI escaping has the funny case it does (%3b vs. %3F).  In fromFileURI, we only match that case, which might cause a problem for opposite cases.  Should I add some tests for case-insensitivity and adjust the code to match, or are %3b and %3F canonical?

Comment 47

[David Teller [:Yoric] - still alive but not very active]

If my memory serves, %3b comes from the reference implementation: http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsURLHelperUnix.cpp#l45 – I'm not sure where %3F comes from, though. I believe that this maps to "?", so it would be interesting if you could find out the issue that prompted the addition of this case to the code by writing the appropriate tests.

Also, you're right, the choice of case is surprising. We probably want to handle both upper and lower case.

Now, I just realized that bug 920015 has landed, which means that we should be able to use the URL object (https://developer.mozilla.org/en-US/docs/Web/API/URL) for robust parsing of URLs. Could you check it out?

Comment 48

[Dustin J. Mitchell [:dustin] (he/him)]

I've refactored the tests to check case sensitivity, and also switched to decodeURIComponent to convert %xx to the corresponding character, rather than using the ternary operator.  That's baking in try right now.

URL sounds great.  How do I get ahold of this URL object in ospath_unix.jsm?

Comment 49

[David Teller [:Yoric] - still alive but not very active]

I believe that |new URL(string)| should work.

Comment 50

[Masatoshi Kimura [:emk]]

But you need |Cu.importGlobalProperties(["URL"]);|. See bug 920015.

Comment 51

[Dustin J. Mitchell [:dustin] (he/him)]

Thanks - I didn't know if that was OK in production code (as opposed to in tests).

Comment 52

[Dustin J. Mitchell [:dustin] (he/him)]

(In reply to Masatoshi Kimura [:emk] from comment #50)
> But you need |Cu.importGlobalProperties(["URL"]);|. See bug 920015.

It looks like this is actually false - this seems to cause weird failures in mochitests on try:

10:09:18     INFO -  JavaScript error: , line 0: uncaught exception: 2147942487
https://tbpl.mozilla.org/php/getParsedLog.php?id=30281071&tree=Try

Comment 53

[Dustin J. Mitchell [:dustin] (he/him)]

I stand corrected - that line is required iff `Components` is defined; when it is not defined, URL is already "automatically" available.

Comment 54

[Dustin J. Mitchell [:dustin] (he/him)]

I'm sorry this hasn't seen any action.  I tripped and fell down the rabbit-hole of trying to get Firefox to build on Windows.  It builds, but tests don't run.

Comment 55

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug803188-r2.patch

I finally got the windows dev environment up and running, and a few iterations of the test suite later, I'm confident that the OSFile interfaces correctly mirror those in Services.io, including all of the relevant funny characters, case differences in uri encoding, and so on.  This also uses URL, as suggested.

Sorry for the delay.  Windows is annoying.

https://tbpl.mozilla.org/?tree=Try&rev=576e2e885059

Comment 56

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8347246 [details] [diff] [review]
bug803188-r2.patch

Review of attachment 8347246 [details] [diff] [review]:
-----------------------------------------------------------------

I believe it's good, I just have a few nits & questions before r+.

::: toolkit/components/osfile/modules/ospath_unix.jsm
@@ +35,5 @@
>    "join",
>    "normalize",
> +  "split",
> +  "toFileURI",
> +  "fromFileURI"

Nit: can you add a comma after "fromfileURI"?
Also, please remove the unnecessary newline.

@@ +169,5 @@
> +};
> +exports.toFileURI = toFileURI;
> +
> +/**
> + * Returns the local file path from a given file URI.

Could you throw if the uri scheme is not "file:"?

::: toolkit/components/osfile/modules/ospath_win.jsm
@@ +46,5 @@
>    "split",
>    "winGetDrive",
> +  "winIsAbsolute",
> +  "toFileURI",
> +  "fromFileURI"

Here, too, could you add a comma?

@@ +316,5 @@
> +
> +/**
> + * Returns the local file path from a given file URI.
> + */
> +let fileURIPrefixes = ["file:///", "file://localhost/"];

I believe that you don't need that array anymore.

@@ +317,5 @@
> +/**
> + * Returns the local file path from a given file URI.
> + */
> +let fileURIPrefixes = ["file:///", "file://localhost/"];
> +let fromFileURI = function fromFileURI(uri) {

Could you throw if the uri scheme is not "file:"?

@@ +325,5 @@
> +
> +  let path = decodeURI(uri);
> +  // decode a few characters where URL's parsing is overzealous
> +  path = path.replace(/%(3b|3f|23)/gi,
> +        match => decodeURIComponent(match));

Why don't we need this on Unix?

::: toolkit/components/osfile/tests/xpcshell/test_file_URL_conversion.js
@@ +1,1 @@
> +function run_test() {

Nit: Please add the public domain stuff at the start of the test.

Comment 57

[Dustin J. Mitchell [:dustin] (he/him)]

(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #56)
> @@ +325,5 @@
> > +
> > +  let path = decodeURI(uri);
> > +  // decode a few characters where URL's parsing is overzealous
> > +  path = path.replace(/%(3b|3f|23)/gi,
> > +        match => decodeURIComponent(match));
> 
> Why don't we need this on Unix?

That's a good question, but this code is emulating the Services.io.newURI/newFileURI functionality, with all its warts and oddities (e.g., differing cases of URI encodings), and according to the tests it's doing so correctly.

Comment 58

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug803188-r3.patch

this should address all of the other comments

Comment 59

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8348862 [details] [diff] [review]
bug803188-r3.patch

Review of attachment 8348862 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks.
Don't forget to change the commit message.

Comment 60

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug803188-r3b.patch

(same patch, just different commit message; r+ carried forward)

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/58698f94b762

Comment 62

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/58698f94b762