Simplify Ampersand escaping logic in RenderViewContextMenu.

Simplify Ampersand escaping logic in RenderViewContextMenu.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185312

[Yoyo Zhou, 2013-02-19 18:34:01]

https://codereview.chromium.org/12294022/diff/1/chrome/browser/tab_contents/r...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/12294022/diff/1/chrome/browser/tab_contents/r...
chrome/browser/tab_contents/render_view_context_menu.cc:392:
ReplaceChars(printable_selection_text, ASCIIToUTF16("&").c_str(),
Try L"&" and L"&&" here.

[Yoyo Zhou, 2013-02-19 18:34:46]

And please append "in RenderViewContextMenu." to your CL description.

[Joe Thomas, 2013-02-19 19:17:50]

https://codereview.chromium.org/12294022/diff/1/chrome/browser/tab_contents/r...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/12294022/diff/1/chrome/browser/tab_contents/r...
chrome/browser/tab_contents/render_view_context_menu.cc:392:
ReplaceChars(printable_selection_text, ASCIIToUTF16("&").c_str(),
On 2013/02/19 18:34:01, Yoyo Zhou wrote:
> Try L"&" and L"&&" here.

This works only on platforms like windows where wchar_t is 2 bytes
(http://code.google.com/searchframe#OAMlx_jo-ck/src/base/string16.h&type=cs&l=35).

We get type conversion error on other platforms

[Joe Thomas, 2013-02-19 19:29:00]

Thanks for the review. Please take another look.

On 2013/02/19 18:34:46, Yoyo Zhou wrote:
> And please append "in RenderViewContextMenu." to your CL description.

Done.

[Joe Thomas, 2013-02-19 21:32:11]

https://codereview.chromium.org/12294022/diff/1/chrome/browser/tab_contents/r...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/12294022/diff/1/chrome/browser/tab_contents/r...
chrome/browser/tab_contents/render_view_context_menu.cc:392:
ReplaceChars(printable_selection_text, ASCIIToUTF16("&").c_str(),
On 2013/02/19 19:17:50, Joe Thomas wrote:
> On 2013/02/19 18:34:01, Yoyo Zhou wrote:
> > Try L"&" and L"&&" here.
> 
> This works only on platforms like windows where wchar_t is 2 bytes
>
(http://code.google.com/searchframe#OAMlx_jo-ck/src/base/string16.h&type=cs&l=35).
> 
> We get type conversion error on other platforms

Looks like my above explanation is not fully clear. Trying to explain it in a
better way.

ReplaceChars's second argument takes char16.  wchar_t takes 2 bytes on WINDOWS
but 4 bytes on POSIX platforms. And in WINDOWS wchar_t is typedef'ed to char16
(http://code.google.com/searchframe#OAMlx_jo-ck/src/base/string16.h&type=cs&l=35)

So using Unicode string directly compiles only on Windows but not on other
platforms as we lack suitable type conversion for unicode string to char16.

[Yoyo Zhou, 2013-02-19 21:57:02]

https://chromiumcodereview.appspot.com/12294022/diff/1/chrome/browser/tab_con...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://chromiumcodereview.appspot.com/12294022/diff/1/chrome/browser/tab_con...
chrome/browser/tab_contents/render_view_context_menu.cc:392:
ReplaceChars(printable_selection_text, ASCIIToUTF16("&").c_str(),
On 2013/02/19 19:17:50, Joe Thomas wrote:
> On 2013/02/19 18:34:01, Yoyo Zhou wrote:
> > Try L"&" and L"&&" here.
> 
> This works only on platforms like windows where wchar_t is 2 bytes
>
(http://code.google.com/searchframe#OAMlx_jo-ck/src/base/string16.h&type=cs&l=35).
> 
> We get type conversion error on other platforms

Bummer. I guess C++11 initializer lists would be really useful here if we could
use them inline.

The viable alternative I see is defining a helper function EscapeAmpersands, in
which you define

const char16 ampersand[] = {'&', 0};

What do you think about that?

[Joe Thomas, 2013-02-19 22:39:05]

https://codereview.chromium.org/12294022/diff/1/chrome/browser/tab_contents/r...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/12294022/diff/1/chrome/browser/tab_contents/r...
chrome/browser/tab_contents/render_view_context_menu.cc:392:
ReplaceChars(printable_selection_text, ASCIIToUTF16("&").c_str(),
On 2013/02/19 21:57:02, Yoyo Zhou wrote:
> On 2013/02/19 19:17:50, Joe Thomas wrote:
> > On 2013/02/19 18:34:01, Yoyo Zhou wrote:
> > > Try L"&" and L"&&" here.
> > 
> > This works only on platforms like windows where wchar_t is 2 bytes
> >
>
(http://code.google.com/searchframe#OAMlx_jo-ck/src/base/string16.h&type=cs&l=35).
> > 
> > We get type conversion error on other platforms
> 
> Bummer. I guess C++11 initializer lists would be really useful here if we
could
> use them inline.
> 
> The viable alternative I see is defining a helper function EscapeAmpersands,
in
> which you define
> 
> const char16 ampersand[] = {'&', 0};
> 
> What do you think about that?

That can be done. Patch updated.

[Yoyo Zhou, 2013-02-19 22:42:25]

LGTM

https://codereview.chromium.org/12294022/diff/8001/chrome/browser/tab_content...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/12294022/diff/8001/chrome/browser/tab_content...
chrome/browser/tab_contents/render_view_context_menu.cc:234: // Escape "&" as
"&&".
Move this comment above the function declaration.

[Joe Thomas, 2013-02-19 22:48:05]

https://codereview.chromium.org/12294022/diff/8001/chrome/browser/tab_content...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/12294022/diff/8001/chrome/browser/tab_content...
chrome/browser/tab_contents/render_view_context_menu.cc:234: // Escape "&" as
"&&".
On 2013/02/19 22:42:25, Yoyo Zhou wrote:
> Move this comment above the function declaration.

Done.

[Joe Thomas, 2013-02-21 17:57:54]

Avi@

friendly ping for OWNERS review.

[Avi (use Gerrit), 2013-02-28 15:43:52]

lgtm

[commit-bot: I haz the power, 2013-02-28 18:02:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/12294022/1002

[commit-bot: I haz the power, 2013-02-28 20:26:19]

Change committed as 185312