Lubomir Sedlacik wrote:
Hello,
I'd like to ask for a code review of the suggested fix for:
6802008 webrev generates relative URI for workspace names containing
colon which confuse browser
It was useful to view the html source for the good and bad examples.
Firefox was smart about translating on a rollover of the link.
This looks good, though there is sufficient use of (for example)
wpatch_url="$(print $WNAME.patch | url_encode)"
print "<a href=\"$wpatch_url\">$WNAME.patch</a></td></tr>"
...that it seems you could have written encode_url() to take an
argument, and then used
print "<a href=\"$(encode_url $WNAME.patch)\">$WNAME.patch</a></td></tr>"
But that's not a strong objection.
Is there any reason that the percent character is missing from the list
of reserved chars in the comment preceding encode_url()?
--Mark
Note: while e.g., onnv gate is not buildable from a directory tree
containing colon due to variable transformations in Makefiles, webrev is
used for other gates where this is perfectly acceptable. I regularly
contribute to two such gates. Also, there are other reserved characters
in the URI scheme which are valid characters in file names, e.g., "+",
"=", "," or "(" and ")" which should be percent-encoded.
The fix itself is more generic and implements a filter function, which
is then used in every place where relative URLs are generated by webrev
and encodes all the reserved characters.
The webrev for the change is available at:
http://cr.opensolaris.org/~ls117037/6802008/
An example of a broken URI in webrev generated before the fix:
http://cr.opensolaris.org/~ls117037/6802008_broken/
An example of a correct URI, generated with the fixed webrev:
http://cr.opensolaris.org/~ls117037/6802008_fixed/
Thanks.
regards,
_______________________________________________
tools-discuss mailing list
tools-discuss@opensolaris.org
