expose NS_EscapeURL through nsINetUtil

RESOLVED FIXED in mozilla1.9alpha6

Status

()

Core
Networking
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: aaronr, Assigned: prasad)

Tracking

Trunk
mozilla1.9alpha6
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

12 years ago
Currently nsINetUtil interface has the function escapeString to expose NS_Escape functionality to components and extensions through nsIOService.  The XForms extension (and possibly other extensions) needs access to NS_EscapeURL, so I'd like to add a function called 'escapeURL' to the nsINetUtil interface and implement the work in nsIOService.  We currently access NS_EscapeURL directly, but that won't be possible once --enable-libxul is on by default for the trunk.
(Reporter)

Updated

12 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 1

12 years ago
Created attachment 237289 [details] [diff] [review]
patch

adds interface method that calls directly into NS_EscapeURL
(Reporter)

Updated

12 years ago
Attachment #237289 - Flags: review?(darin)

Comment 2

12 years ago
Comment on attachment 237289 [details] [diff] [review]
patch

The flag values really ought to be defined on this interface as well.  People should not have to #include nsEscape.h in order to get the flag values, or they should not have to write numeric literals in their place.
Attachment #237289 - Flags: review?(darin) → review-
(Reporter)

Comment 3

12 years ago
(In reply to comment #2)
> (From update of attachment 237289 [details] [diff] [review] [edit])
> The flag values really ought to be defined on this interface as well.  People
> should not have to #include nsEscape.h in order to get the flag values, or they
> should not have to write numeric literals in their place.
> 

My bad, I should have thought of this.  I'm guessing that the best way to do this would be to define the values in the interface and then change the flags in nsEscape.h to be based off of the interface flags.  I'll put together a patch that does this.
(Reporter)

Comment 4

12 years ago
Created attachment 238554 [details] [diff] [review]
patch 2

patch to address darin's comment.
Attachment #237289 - Attachment is obsolete: true
Comment on attachment 238554 [details] [diff] [review]
patch 2

biesi is already reviewing a similar change to nsINetUtil... Can we resurrect this one?
Attachment #238554 - Flags: review?(cbiesinger)
I'd really rather not expose _two_ functions to escape strings here, especially if it's unclear how they differ...
(Assignee)

Comment 7

11 years ago
Created attachment 267921 [details] [diff] [review]
Exposes both EscapeURL and UnescapeURL though nsINetUtil

This patch exposes both UnescapeURL and EscapeURL (originally NS_EscapeURL and NS_UnescapeURL from nsEscape.h) through nsINetUtil.

The main difference between EscapeString and EscapeURL will be that EscapeURL is a more specialized function for %XX escaping URLs. This function recognizes various parts of the URL... scheme, username, password, path, filename etc; and lets the user escape any of the parts selectively.  The output is appended to the result string (unlike EscapeString where the output is copied to result string)
Attachment #267921 - Flags: review?(cbiesinger)
(Assignee)

Comment 8

11 years ago
Biesi, more information on why URL escaping is different is discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=61269
OK. I accept the need for escapeURL.

Instead of adding unescapeURL, I'd prefer if you just added a flags parameter to the existing unescape function and made it call NS_UnescapeURL.

What I would like to see in a patch that adds it:
- A comment in nsEscape.h as in aaronr's patch
- Comments about the individual flag values as in prasad's patch
- Combining things like ESCAPE_MINIMAL from the individual flags as in aaronr's patch
- Using (1 << N) for the actual values, so that this is closer to the .h file (e.g. as in how nsIChannel.idl defines its flags)


The implementations don't match the XPCOM semantics, you should truncate the string first (or change it to an inout parameter). However, I think this would be easier to use if the signature looked like:

  ACString escapeURL(in ACString string, in unsigned long flags);

And if it always set ESCAPE_ALWAYS_COPY (and didn't add that flag on the interface). What do you think? I'm not sure that the performance optimization of avoiding the copy sometimes is very valuable here.


Also, don't use ToNewCString to implement this. Instead, pass aStr.BeginReading().
(Assignee)

Comment 10

11 years ago
Created attachment 268061 [details] [diff] [review]
Expose EscapeURL and update UnescapeString to work for URLs

Biesi, I updated the patch as per your comments.

I am not sure if what I did is correct, specially w.r.t truncating the string.  Can you explain me the need from truncating?
Attachment #267921 - Attachment is obsolete: true
Attachment #268061 - Flags: review?(cbiesinger)
Attachment #267921 - Flags: review?(cbiesinger)
(Assignee)

Comment 11

11 years ago
Can Darin or someone else review the patch if Biesi is busy.
Comment on attachment 268061 [details] [diff] [review]
Expose EscapeURL and update UnescapeString to work for URLs

nsINetUtil.idl
+  /** Copy input to result buffer even if escaping is unnecessary */
+  // Did not see situations where NS_EscapeURL is called without esc_AlwaysCopy
+  // const unsigned long ESCAPE_URL_ALWAYS_COPY = 1 << 13;

please remove that

+   * @param aFlags only ESCAPE_URL_ONLY_NONASCII, ESCAPE_URL_SKIP_CONTROL and 
+   *               ESCAPE_URL_ALWAYS_COPY are recognized

perhaps mention that 0 is a valid value too

+%{C++

please don't add C++ blocks to necko IDL files... just update the callers instead

nsIOService.cpp
+  PRBool escaped = NS_EscapeURL(aStr.BeginReading(), aStr.Length(), aFlags, aResult);

You should add esc_AlwaysCopy to the flags here too

With that flag, the scape/unescape functions always return true, so you can just always return NS_OK

r=biesi with those changes. please attach a patch with those changes.
Attachment #268061 - Flags: review?(cbiesinger) → review+
(In reply to comment #10)
> I am not sure if what I did is correct, specially w.r.t truncating the string. 
> Can you explain me the need from truncating?

Yes, it's correct. Thanks.

The need is that XPCOM functions shouldn't rely on any specific value of out parameters and thus overwrite the values there.

(Assignee)

Comment 14

11 years ago
Created attachment 268732 [details] [diff] [review]
Exposes both EscapeURL and UnescapeURL though nsINetUtil - Updated as per Biesi's comments.

Updated the patch according to Biesi's comments above.
Attachment #268061 - Attachment is obsolete: true
Attachment #268732 - Flags: review?(cbiesinger)
Comment on attachment 268732 [details] [diff] [review]
Exposes both EscapeURL and UnescapeURL though nsINetUtil - Updated as per Biesi's comments.

oh... sorry, I missed this in my previous review:

please move escapeString and its flags after toImmutableURI, so that the escape functions are all grouped together
+   * %XX-Escape invalid chars in a URL segment. Has no effect if the
+   * URL segment is already escaped. Otherwise, the escaped URL segment is
+   * appended to |aResult|

this description is wrong. it should be something like:

+   * %XX-Escape invalid chars in a URL segment.
+   *
+   * @param aStr the URL to be escaped
+   * @param aFlags the URL segment type flags
+   *
+   * @return the escaped string (the string itself if escaping did not happen)

+   * @param aFlags only ESCAPE_URL_ONLY_NONASCII, ESCAPE_URL_SKIP_CONTROL and 
+   *               ESCAPE_URL_ALWAYS_COPY are recognized.  If |aFlags| is 0
+   *               the function act as nsEscape::Unescape

ESCAPE_URL_ALWAYS_COPY doesn't exist anymore, so you shouldn't mention it here :)

nsEscape is not a class... just write that if flags is 0, all escape sequences are unescaped.
Attachment #268732 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 16

11 years ago
Created attachment 268969 [details] [diff] [review]
Expose EscapeURL and update UnescapeString to work for URLs

Updated as per the comments above.
Attachment #268732 - Attachment is obsolete: true
Attachment #268969 - Flags: review?(cbiesinger)
Attachment #268969 - Flags: superreview+
Attachment #268969 - Flags: review?(cbiesinger)
Attachment #268969 - Flags: review+
Attachment #238554 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 17

11 years ago
biesi, can you check this in for me... I don't have commit access to CVS.
Whiteboard: [checkin needed]

Updated

11 years ago
Assignee: aaronr → prasad
Status: ASSIGNED → NEW

Comment 18

11 years ago
mozilla/xpcom/io/nsEscape.h                 1.29
mozilla/netwerk/base/public/nsINetUtil.idl  1.6
mozilla/netwerk/base/src/nsIOService.cpp    1.197
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha6

Comment 19

11 years ago
Comment on attachment 238554 [details] [diff] [review]
patch 2

The newer patches obsoleted this patch, right?
Attachment #238554 - Attachment is obsolete: true

Comment 20

11 years ago
Prasad: think you'd have time to write some brief testcases for this?  See the following URL for details:

http://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests
Flags: in-testsuite?
(Reporter)

Updated

11 years ago
Blocks: 348391
unescape testcase added:
mozilla/netwerk/test/unit/test_unescapestring.js 	1.1
You need to log in before you can comment on or make changes to this bug.