Modified clipboard classes to use string16 instead of std::wstring

Modified clipboard classes to use string16 instead of std::wstring

committed in r10860

[Evan Stade, 2009-02-28 02:58:54]

if this is still a work in progress, ignore me, but I wanted to make a couple
comments before you put too much effort into this. Overall it looks good, just 2
things:

a) The file-related functions should take FilePaths, not string16s. It would
also be good to leave it in its native encoding when packing the data into the
byte vectors.

b) aren't any of the clients of SCW ported to posix? I don't think this patch as
it stands will even compile on posix. (I dunno what platform you're working on.)

http://codereview.chromium.org/28294/diff/1012/15
File base/scoped_clipboard_writer.cc (right):

http://codereview.chromium.org/28294/diff/1012/15#newcode81
Line 81: void ScopedClipboardWriter::WriteFile(const string16& file) {
this should take a filepath

http://codereview.chromium.org/28294/diff/1012/16
File base/scoped_clipboard_writer.h (right):

http://codereview.chromium.org/28294/diff/1012/16#newcode13
Line 13: #include <string>
shouldn't this be base/string16.h?

[dsh, 2009-02-28 04:23:26]

Hi,

I don't have much insight on the clipboard bits, but the filenames should be
FilePaths.

Thanks.

http://codereview.chromium.org/28294/diff/1012/14
File base/clipboard_unittest.cc (right):

http://codereview.chromium.org/28294/diff/1012/14#newcode36
Line 36: clipboard_writer.WriteText(WideToUTF16(L"clear me"));
I'm settling on ASCIIToUTF16 for most literals (obviously not the ones with \u
escape codes and such).  Please do that as well so we're consistent.

http://codereview.chromium.org/28294/diff/1012/14#newcode208
Line 208: string16 file3 = WideToUTF16("/usr/bin/perl");
Looks like you deleted an L.  I don't think this will build on Mac.  (Should be
a FilePath anyway.)

http://codereview.chromium.org/28294/diff/1012/17
File base/string_util.h (right):

http://codereview.chromium.org/28294/diff/1012/17#newcode599
Line 599: inline std::ostream& operator<<(std::ostream& stream, const string16
&s16) {
Can this go in string16.h?

[Evan Stade, 2009-03-01 01:38:24]

Hi Mark,

please use the code review tool to respond, as emails don't get scraped and
added here.

For the byte-packing, is there some reason you can't use a pair of null bytes as
the terminator? Also, you could probably do without any special byte ordering
code; just pack and unpack string_var.data() byte by byte (not character by
character).

[dsh, 2009-03-02 20:28:52]

Hi Mark,

This looks good except please use the Hack functions noted below wherever they
apply.  The clipboard writing stuff is beyond me.  Better let Evan check that
part.

http://codereview.chromium.org/28294/diff/36/1025
File base/scoped_clipboard_writer.h (right):

http://codereview.chromium.org/28294/diff/36/1025#newcode14
Line 14: #include <vector>
Do we need either string or vector here?  It seems this file will compile
without them.

http://codereview.chromium.org/28294/diff/36/1033
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/28294/diff/36/1033#newcode535
Line 535: *result = UTF16ToWide(text);
Rather than put TODO comments all over, just use UTF16ToWideHack or
WideToUTF16Hack.  This is enough to signal that the conversion should be
temporary.

This also applies to anyplace else in the code where this type of conversion is
made.

[mark.a.lindner, 2009-03-02 20:33:23]

> http://codereview.chromium.org/28294/diff/36/1025
> File base/scoped_clipboard_writer.h (right):
> 
> http://codereview.chromium.org/28294/diff/36/1025#newcode14
> Line 14: #include <vector>
> Do we need either string or vector here?  It seems this file will compile
> without them.

This was added to resolve a lint warning.

[mark.a.lindner, 2009-03-02 20:39:41]

http://codereview.chromium.org/28294/diff/1052/59
File base/scoped_clipboard_writer.cc (right):

http://codereview.chromium.org/28294/diff/1052/59#newcode95
Line 95: FilePath filepath = *iter;
> For the byte-packing, is there some reason you can't use a pair of null bytes
as the terminator? Also, you could probably do without any special byte ordering
code; just pack and unpack string_var.data() byte by byte (not character by
character).

Done.

[dsh, 2009-03-02 20:59:48]

OK, please still fix the Hack comments I made before and the minor curly brace
nit.  After that it LGTM if it LGT Evan.

Thanks.

http://codereview.chromium.org/28294/diff/1052/59
File base/scoped_clipboard_writer.cc (right):

http://codereview.chromium.org/28294/diff/1052/59#newcode102
Line 102: for (const char *ch = data; ch < data_end; ++ch) {
You don't need curly braces unless the condition or the body spans multiple
lines.

http://codereview.chromium.org/28294/diff/1052/59#newcode107
Line 107: for (size_t i = 0; i < sizeof(FilePath::CharType); ++i) {
Ditto.

http://codereview.chromium.org/28294/diff/1052/59#newcode113
Line 113: for (size_t i = 0; i < sizeof(FilePath::CharType); ++i) {
And here.

[mark.a.lindner, 2009-03-02 21:18:12]

On 2009/03/02 20:59:48, dsh wrote:
> OK, please still fix the Hack comments I made before and the minor curly brace
> nit.  After that it LGTM if it LGT Evan.

Fixed.

[Evan Stade, 2009-03-02 21:21:36]

you'll have to update the functions that decode the file paths from the byte
vector. (If you need help on mac side then ping motownavi or playmobil or
someone on IRC.) As it stands this won't pass the unit test on mac or windows.

http://codereview.chromium.org/28294/diff/1052/64
File base/clipboard.h (right):

http://codereview.chromium.org/28294/diff/1052/64#newcode67
Line 67: // CBF_FILES     files        char array representing multiple files.
can you update this comment? maybe indicate that the encoding of the characters
is platform-specific and matches the file paths encoding

http://codereview.chromium.org/28294/diff/1052/64#newcode111
Line 111: void ReadFile(string16* file) const;
these two functions should deal in FilePaths

http://codereview.chromium.org/28294/diff/1052/65
File base/clipboard_mac.mm (right):

http://codereview.chromium.org/28294/diff/1052/65#newcode103
Line 103: void Clipboard::WriteFiles(const char* file_data, size_t file_len) {
need to update this function

http://codereview.chromium.org/28294/diff/1052/62
File base/clipboard_win.cc (right):

http://codereview.chromium.org/28294/diff/1052/62#newcode203
Line 203: string16 wide_bookmark = UTF8ToUTF16(bookmark);
I'd say you should leave this bit as is. It doesn't matter much since wstring ==
string16 in windows but GetUrlWFormatType sounds like it should be called with a
wstring.

Same goes for the rest of the file, where appropriate.

http://codereview.chromium.org/28294/diff/1052/62#newcode353
Line 353: string16 filenames(UTF8ToUTF16(std::string(file_data, file_len)));
need to update this.

[dsh, 2009-03-02 21:26:42]

http://codereview.chromium.org/28294/diff/76/86
File chrome/browser/tab_contents/render_view_context_menu_controller.cc (right):

http://codereview.chromium.org/28294/diff/76/86#newcode71
Line 71: // TODO(mlindner): fix this call and upstream code to use string16
Forgot this one.

http://codereview.chromium.org/28294/diff/76/86#newcode78
Line 78: string16 text16 = WideToUTF16(text);
There's also really no need for an extra variable here.

[brettw, 2009-03-02 21:27:09]

http://codereview.chromium.org/28294/diff/36/1025
File base/scoped_clipboard_writer.h (right):

http://codereview.chromium.org/28294/diff/36/1025#newcode14
Line 14: #include <vector>
This isn't just about lint. Files should include their dependencies, and not
rely on other files bring them in. In this case, this file uses vector, so it
should include <vector>

[mark.a.lindner, 2009-03-02 23:22:02]

On 2009/03/02 21:21:36, estade wrote:
> you'll have to update the functions that decode the file paths from the byte
> vector.

I was wondering about that, but I couldn't find any such functions. Where are
they? I don't see any "ScopedClipboardReader" class anywhere...

[Evan Stade, 2009-03-02 23:47:21]

WriteFiles() in the Clipboard class

[mark.a.lindner, 2009-03-03 00:31:47]

On 2009/03/02 21:21:36, estade wrote:
> you'll have to update the functions that decode the file paths from the byte
> vector. (If you need help on mac side then ping motownavi or playmobil or
> someone on IRC.) As it stands this won't pass the unit test on mac or windows.

I fixed WriteFiles() in clipboard_win.cc.

 
> http://codereview.chromium.org/28294/diff/1052/64
> File base/clipboard.h (right):
> 
> http://codereview.chromium.org/28294/diff/1052/64#newcode67
> Line 67: // CBF_FILES     files        char array representing multiple files.
> can you update this comment? maybe indicate that the encoding of the
characters
> is platform-specific and matches the file paths encoding

Done.


> http://codereview.chromium.org/28294/diff/1052/64#newcode111
> Line 111: void ReadFile(string16* file) const;
> these two functions should deal in FilePaths
> 
> http://codereview.chromium.org/28294/diff/1052/65
> File base/clipboard_mac.mm (right):
> 
> http://codereview.chromium.org/28294/diff/1052/65#newcode103
> Line 103: void Clipboard::WriteFiles(const char* file_data, size_t file_len) {
> need to update this function

I don't think anything needs to change here because on Mac filenames are still
encoded in UTF8 as before (FilePath uses std::string internally on Mac).


> http://codereview.chromium.org/28294/diff/1052/62
> File base/clipboard_win.cc (right):
> 
> http://codereview.chromium.org/28294/diff/1052/62#newcode203
> Line 203: string16 wide_bookmark = UTF8ToUTF16(bookmark);
> I'd say you should leave this bit as is. It doesn't matter much since wstring
==
> string16 in windows but GetUrlWFormatType sounds like it should be called with
a
> wstring.

Done.

 
> Same goes for the rest of the file, where appropriate.
> 
> http://codereview.chromium.org/28294/diff/1052/62#newcode353
> Line 353: string16 filenames(UTF8ToUTF16(std::string(file_data, file_len)));
> need to update this.

Done.

[Evan Stade, 2009-03-03 00:45:04]

OK, just 2 more comments. In the meantime, I'll run this by the trybots for you.

http://codereview.chromium.org/28294/diff/1083/108
File base/clipboard_win.cc (right):

http://codereview.chromium.org/28294/diff/1083/108#newcode358
Line 358: HANDLE hdata = ::GlobalAlloc(GMEM_MOVEABLE, file_len);
shouldn't this still be |bytes|?

http://codereview.chromium.org/28294/diff/1083/108#newcode367
Line 367: memcpy(data + sizeof DROPFILES, file_data, file_len);
I know you didn't change it, but can you make this sizeof(DROPFILES) for
consistency (and because DROPFILES is not a variable)

[mark.a.lindner, 2009-03-03 00:57:18]

http://codereview.chromium.org/28294/diff/1012/16
File base/scoped_clipboard_writer.h (right):

http://codereview.chromium.org/28294/diff/1012/16#newcode13
Line 13: #include <string>
On 2009/02/28 02:58:54, estade wrote:
> shouldn't this be base/string16.h?

I added an include for that; they should both be here.

http://codereview.chromium.org/28294/diff/1083/108
File base/clipboard_win.cc (right):

http://codereview.chromium.org/28294/diff/1083/108#newcode358
Line 358: HANDLE hdata = ::GlobalAlloc(GMEM_MOVEABLE, file_len);
On 2009/03/03 00:45:04, estade wrote:
> shouldn't this still be |bytes|?

Yes, you're right. Fixed.

http://codereview.chromium.org/28294/diff/1083/108#newcode367
Line 367: memcpy(data + sizeof DROPFILES, file_data, file_len);
On 2009/03/03 00:45:04, estade wrote:
> I know you didn't change it, but can you make this sizeof(DROPFILES) for
> consistency (and because DROPFILES is not a variable)

Done.

[Evan Stade, 2009-03-03 01:52:18]

What OS and build command did you use? As I supposed in my first post on this
review, there are tons client callers that are complaining... I'm in the process
of fixing them. But generally you need to build more than just the module you're
touching, and on all 3 platforms if you can.