Review of attachment 9062711 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK to me. Note that the mailnews/compose hasn't bee reformatted to Google style yet. That will be done in the next few days. See bug 1546364. ::: mailnews/compose/src/nsMsgCompUtils.cpp @@ +921,4 @@ > > bool needEscape; > nsCString dupParm; > + nsCString charset("UTF-8"); We could hard-code that in the two/three spots where this is used. @@ +927,2 @@ > needEscape = true; > + dupParm = parmValue; Maybe better: dupParam.Assign(parmValue) ? @@ -960,4 @@ > } > > - if (dupParm.IsEmpty()) > - return nullptr; Should we leave this in? It could be called with a null string. @@ +1014,4 @@ > } > + // *end is now a '%'. > + // Check if the following UTF-8 octet is a continuation. > + while(end > start + 3 && (*(end + 1) == '8' || *(end + 1) == '9' `while (space) (end - 3 > start`, for consistency, no? Above we have `end - 1` and `end - 2`. @@ +1014,5 @@ > } > + // *end is now a '%'. > + // Check if the following UTF-8 octet is a continuation. > + while(end > start + 3 && (*(end + 1) == '8' || *(end + 1) == '9' > + || *(end + 1) == 'A' || *(end + 1) == 'B')) { Hmm, can this be written as ... && '8 <= (*end + 1) && (*end + 1) <= 'B')?
Bug 1532668 Comment 13 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Review of attachment 9062711 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK to me. Note that the mailnews/compose hasn't bee reformatted to Google style yet. That will be done in the next few days. See bug 1546364. ::: mailnews/compose/src/nsMsgCompUtils.cpp @@ +921,4 @@ > > bool needEscape; > nsCString dupParm; > + nsCString charset("UTF-8"); We could hard-code that in the two/three spots where this is used. @@ +927,2 @@ > needEscape = true; > + dupParm = parmValue; Maybe better: dupParam.Assign(parmValue) ? @@ -960,4 @@ > } > > - if (dupParm.IsEmpty()) > - return nullptr; Should we leave this in? It could be called with a null string. @@ +1014,4 @@ > } > + // *end is now a '%'. > + // Check if the following UTF-8 octet is a continuation. > + while(end > start + 3 && (*(end + 1) == '8' || *(end + 1) == '9' `while (space) (end - 3 > start`, for consistency, no? Above we have `end - 1` and `end - 2`. @@ +1014,5 @@ > } > + // *end is now a '%'. > + // Check if the following UTF-8 octet is a continuation. > + while(end > start + 3 && (*(end + 1) == '8' || *(end + 1) == '9' > + || *(end + 1) == 'A' || *(end + 1) == 'B')) { Hmm, can this be written as ... && '8' <= (*end + 1) && (*end + 1) <= 'B')?
Review of attachment 9062711 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK to me. Note that the mailnews/compose hasn't bee reformatted to Google style yet. That will be done in the next few days. See bug 1546364. ::: mailnews/compose/src/nsMsgCompUtils.cpp @@ +921,4 @@ > > bool needEscape; > nsCString dupParm; > + nsCString charset("UTF-8"); We could hard-code that in the two/three spots where this is used. @@ +927,2 @@ > needEscape = true; > + dupParm = parmValue; Maybe better: dupParam.Assign(parmValue) ? @@ -960,4 @@ > } > > - if (dupParm.IsEmpty()) > - return nullptr; Should we leave this in? It could be called with a null string. @@ +1014,4 @@ > } > + // *end is now a '%'. > + // Check if the following UTF-8 octet is a continuation. > + while(end > start + 3 && (*(end + 1) == '8' || *(end + 1) == '9' `while (space) (end - 3 > start`, for consistency, no? Above we have `end - 1` and `end - 2`. @@ +1014,5 @@ > } > + // *end is now a '%'. > + // Check if the following UTF-8 octet is a continuation. > + while(end > start + 3 && (*(end + 1) == '8' || *(end + 1) == '9' > + || *(end + 1) == 'A' || *(end + 1) == 'B')) { Hmm, can this be written as ... && '8' <= *(end + 1) && *(end + 1) <= 'B')? EDIT: Fixed last code just above.