Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 10103676
or point your web browser to
http://mondrian/10103676
to review the following code:
Change 10103676 by stevebl...@steveblock-gears2 on 2009/02/11 13:19:53 *pending*
Escapes percent symbols in shortcut URLs when they are not already part
of an escape sequence.
Note that we can not use EscapeUrl from url_utils.cc, as this does not
escape percent symbols unless forced. When forced, all percent symbols,
including those already part of an escape sequence are escaped. See
https://bugzilla.mozilla.org/show_bug.cgi?query_format=specific&order=relevance+desc&bug_status=__open__&id=61269
for details.
R=andreip
[email protected]
DELTA=74 (70 added, 4 deleted, 0 changed)
OCL=10103676
B=1433689
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/url_utils.cc#5 edit
... //depot/googleclient/gears/opensource/gears/base/common/url_utils.h#1 edit
... //depot/googleclient/gears/opensource/gears/base/common/url_utils_test.cc#3
edit
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#78 edit
74 delta lines: 70 added, 4 deleted, 0 changed
Also consider running:
g4 lint -c 10103676
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [email protected].
Change 10103676 by stevebl...@steveblock-gears2 on 2009/02/11 13:19:53 *pending*
Escapes percent symbols in shortcut URLs when they are not already part
of an escape sequence.
Note that we can not use EscapeUrl from url_utils.cc, as this does not
escape percent symbols unless forced. When forced, all percent symbols,
including those already part of an escape sequence are escaped. See
https://bugzilla.mozilla.org/show_bug.cgi?query_format=specific&order=relevance+desc&bug_status=__open__&id=61269
for details.
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/url_utils.cc#5 edit
... //depot/googleclient/gears/opensource/gears/base/common/url_utils.h#1 edit
... //depot/googleclient/gears/opensource/gears/base/common/url_utils_test.cc#3
edit
... //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#78 edit
==== //depot/googleclient/gears/opensource/gears/base/common/url_utils.cc#5 -
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/base/common/url_utils.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/url_utils.cc
2009-02-11 13:29:43.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/url_utils.cc
2009-02-11 13:17:04.000000000 +0000
@@ -309,6 +309,25 @@
return result;
}
+std::string16 EscapePercent(const std::string16 &source) {
+ static const std::string16 kHexCharacters(
+ STRING16(L"0123456789abcdefABCDEF"));
+ static const char16 kPercentCharacter = '%';
+ static const char16 kPercentCharacterEscapeCode[] = L"25";
+ std::string16 result;
+ for (int i = 0; i < static_cast<int>(source.length()); ++i) {
+ const char16 *c = source.c_str() + i;
+ result += *c;
+ if (*c == kPercentCharacter &&
+ (i > static_cast<int>(source.length()) - 3 ||
+ !(kHexCharacters.find(*(c+1)) != std::string16::npos &&
+ kHexCharacters.find(*(c+2)) != std::string16::npos))) {
+ result += kPercentCharacterEscapeCode;
+ }
+ }
+ return result;
+}
+
bool ResolveAndNormalize(const char16 *base, const char16 *url,
std::string16 *out) {
assert(url && out);
==== //depot/googleclient/gears/opensource/gears/base/common/url_utils.h#1 -
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/base/common/url_utils.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/url_utils.h 2009-02-11
13:29:43.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/url_utils.h 2009-02-11
13:19:22.000000000 +0000
@@ -66,9 +66,6 @@
std::string UnescapeURL(const std::string& escaped_text);
std::string16 UnescapeURL(const std::string16& escaped_text);
-// ----------------------------------------------------------------------
-// Converts a UTF8 path to a percent encoded "file:///" URL.
-// ----------------------------------------------------------------------
enum EscapeUrlFlags {
ESCAPE_SCHEME = 1,
ESCAPE_USERNAME = 2,
@@ -93,7 +90,14 @@
ESCAPE_COLON = 16384, // forces escape of colon
ESCAPE_SKIPCONTROL = 32768 // skips C0 and DEL from unescaping
};
-std::string UTF8PathToUrl(const std::string &path, bool directory);
std::string EscapeUrl(const std::string &source, unsigned int flags);
+// Escapes percent symbols that are not already part of an escape sequence.
+std::string16 EscapePercent(const std::string16 &source);
+
+// ----------------------------------------------------------------------
+// Converts a UTF8 path to a percent encoded "file:///" URL.
+// ----------------------------------------------------------------------
+std::string UTF8PathToUrl(const std::string &path, bool directory);
+
#endif // GEARS_BASE_COMMON_URL_UTILS_H__
====
//depot/googleclient/gears/opensource/gears/base/common/url_utils_test.cc#3 -
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/base/common/url_utils_test.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/url_utils_test.cc
2009-02-11 13:29:43.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/url_utils_test.cc
2009-02-11 13:08:40.000000000 +0000
@@ -30,11 +30,13 @@
static bool TestUrlUTF8FileToUrl();
static bool TestUrlResolve();
+static bool TestEscapePercent();
bool TestUrlUtils(std::string16 *error) {
bool ok = true;
ok &= TestUrlResolve();
ok &= TestUrlUTF8FileToUrl();
+ ok &= TestEscapePercent();
if (ok) {
LOG(("TestUrlUtilsAll - passed\n"));
} else {
@@ -322,3 +324,40 @@
LOG(("TestUrlUTF8FileToUrl - passed\n"));
return true;
}
+
+static bool TestEscapePercent() {
+#undef TEST_ASSERT
+#define TEST_ASSERT(b,test_name) \
+{ \
+ if (!(b)) { \
+ LOG(("TestEscapePercent: %s - failed (%d)\n", test_name, __LINE__)); \
+ return false; \
+ } \
+}
+ struct URLCase {
+ const char16 *input;
+ const char16 *expected;
+ const char16 *test_name;
+ } cases[] = {
+ {STRING16(L"foo%foo"), STRING16(L"foo%25foo"), STRING16(L"Escape lone %")},
+ {STRING16(L"foo%25foo"), STRING16(L"foo%25foo"),
+ STRING16(L"Already escaped")},
+ {STRING16(L"foo%a1foo"), STRING16(L"foo%a1foo"),
+ STRING16(L"Part of escape sequence")},
+ {STRING16(L"foo%A1foo"), STRING16(L"foo%A1foo"),
+ STRING16(L"Part of uppercase escape sequence")},
+ {STRING16(L"foo%%foo"), STRING16(L"foo%25%25foo"), STRING16(L"Two")},
+ {STRING16(L"foo%foo%xy"), STRING16(L"foo%25foo%25xy"),
+ STRING16(L"Near end")},
+ {STRING16(L"foo%foo%x"), STRING16(L"foo%25foo%25x"),
+ STRING16(L"Very near end")},
+ {STRING16(L"foo%foo%"), STRING16(L"foo%25foo%25"), STRING16(L"At end")}
+ };
+
+ for (unsigned int i = 0; i < ARRAYSIZE(cases); ++i) {
+ std::string16 input(cases[i].input);
+ std::string16 output(EscapePercent(input));
+ TEST_ASSERT(output == cases[i].expected, cases[i].test_name);
+ }
+ return true;
+}
\ No newline at end of file
==== //depot/googleclient/gears/opensource/gears/desktop/desktop.cc#78 -
c:\MyDocs\Gears2/googleclient/gears/opensource/gears/desktop/desktop.cc ====
# action=edit type=text
--- googleclient/gears/opensource/gears/desktop/desktop.cc 2009-02-11
13:29:43.000000000 +0000
+++ googleclient/gears/opensource/gears/desktop/desktop.cc 2009-02-11
12:55:04.000000000 +0000
@@ -160,6 +160,9 @@
return false;
}
+ // Escape percents in the URL.
+ shortcut_info->app_url = EscapePercent(shortcut_info->app_url);
+
// If we ever want to support this see b/1408993, also note that
// IsSameOriginAsUrl() rejects data URLs.
if (IsDataUrl(shortcut_info->app_url.c_str())) {
@@ -834,6 +837,7 @@
}
*url = full_url;
+
return true;
}
