Closed
Bug 350932
Opened 18 years ago
Closed 17 years ago
expose NS_EscapeURL through nsINetUtil
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha6
People
(Reporter: aaronr, Assigned: prasad)
References
Details
Attachments
(1 file, 5 obsolete files)
9.93 KB,
patch
|
Biesinger
:
review+
Biesinger
:
superreview+
|
Details | Diff | Splinter Review |
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.
adds interface method that calls directly into NS_EscapeURL
Attachment #237289 -
Flags: review?(darin)
Comment 2•18 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-
(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.
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)
Comment 6•17 years ago
|
||
I'd really rather not expose _two_ functions to escape strings here, especially if it's unclear how they differ...
Assignee | ||
Comment 7•17 years ago
|
||
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•17 years ago
|
||
Biesi, more information on why URL escaping is different is discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=61269
Comment 9•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
Can Darin or someone else review the patch if Biesi is busy.
Comment 12•17 years ago
|
||
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+
Comment 13•17 years ago
|
||
(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•17 years ago
|
||
Updated the patch according to Biesi's comments above.
Attachment #268061 -
Attachment is obsolete: true
Attachment #268732 -
Flags: review?(cbiesinger)
Comment 15•17 years ago
|
||
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•17 years ago
|
||
Updated as per the comments above.
Attachment #268732 -
Attachment is obsolete: true
Attachment #268969 -
Flags: review?(cbiesinger)
Updated•17 years ago
|
Attachment #268969 -
Flags: superreview+
Attachment #268969 -
Flags: review?(cbiesinger)
Attachment #268969 -
Flags: review+
Updated•17 years ago
|
Attachment #238554 -
Flags: review?(cbiesinger) → review-
Assignee | ||
Comment 17•17 years ago
|
||
biesi, can you check this in for me... I don't have commit access to CVS.
Updated•17 years ago
|
Whiteboard: [checkin needed]
Updated•17 years ago
|
Assignee: aaronr → prasad
Status: ASSIGNED → NEW
Comment 18•17 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
Closed: 17 years ago
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha6
Comment 19•17 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•17 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?
Comment 21•17 years ago
|
||
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.
Description
•