Clipboard:...

Clipboard:

- Move link construction logic to scoped_clipboard_writer.cc
- Make callers use EscapeForHTML on the anchor text
- Make WriteHyperlink just write html, and not a bookmark as well (only affects one caller, which I updated)
- implement WriteBookmark for gtk

BUG=18034, 18035
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25833

[Evan Martin, 2009-09-09 18:45:09]

I'm not sure I'm a good reviewer for this, since I don't understand the
motivation of the change...

http://codereview.chromium.org/194052/diff/1/4
File base/scoped_clipboard_writer.h (right):

http://codereview.chromium.org/194052/diff/1/4#newcode46
Line 46: void WriteHyperlink(const std::string& anchor_text, const std::string&
url);
What about non-ASCII anchor text?

[Evan Stade, 2009-09-09 18:56:45]

On 2009/09/09 18:45:09, Evan Martin wrote:
> I'm not sure I'm a good reviewer for this, since I don't understand the
> motivation of the change...

motivation is that the text ought to be escaped for the html hyperlink, but not
for the bookmark. Also, as it turns out we were not escaping the text sometimes
so this fixes that. And we can't use the net:: function we want to from within
base, which makes this slightly messier, but when I tried to move clipboard to
app it added too many dependencies.

> 
> http://codereview.chromium.org/194052/diff/1/4
> File base/scoped_clipboard_writer.h (right):
> 
> http://codereview.chromium.org/194052/diff/1/4#newcode46
> Line 46: void WriteHyperlink(const std::string& anchor_text, const
std::string&
> url);
> What about non-ASCII anchor text?

|anchor_text| is assumed to be utf8, although I don't know if any non-ascii
characters will come out of the EscapeForHTML() function anyway.

[Evan Stade, 2009-09-09 18:57:03]

+pkasting

[Peter Kasting, 2009-09-09 19:08:21]

Like Evan I don't really understand what's going on in this change.  Sorry :)

http://codereview.chromium.org/194052/diff/1/6
File base/clipboard_linux.cc (right):

http://codereview.chromium.org/194052/diff/1/6#newcode186
Line 186: // We will also write a hyperlink since callers of WriteBookmark will
also
Nit: This comment is confusing.  I don't know what it means.

http://codereview.chromium.org/194052/diff/1/3
File base/scoped_clipboard_writer.cc (right):

http://codereview.chromium.org/194052/diff/1/3#newcode76
Line 76: // Construct the hyperlink.
Nit: This comment should go above the |html| declaration.

[Evan Stade, 2009-09-09 19:32:16]

So, previously:
- WriteHyperlink wrote an html hyperlink and a bookmark to the clipboard
- WriteHyperlink failed to escape the anchor text for the hyperlink
- WriteHyperlink was only used in one place
- WriteHyperlink was not implemented for Linux

Now:
- WriteHyperlink *just* writes an html hyperlink
- Callers are instructed to escape the anchor text
- Callers also explicitly call WriteBookmark
- The sole caller of WriteHyperlink was updated to call WriteBookmark as well
- One caller of WriteBookmark was updated to also call WriteHyperlink
- WriteHyperlink is implemented for linux
- WriteHyperlink takes a utf8 string because EscapeForHTML doesn't deal in
string16, and because it makes it easier to build the <a href> stuff

http://codereview.chromium.org/194052/diff/1/6
File base/clipboard_linux.cc (right):

http://codereview.chromium.org/194052/diff/1/6#newcode186
Line 186: // We will also write a hyperlink since callers of WriteBookmark will
also
On 2009/09/09 19:08:22, Peter Kasting wrote:
> Nit: This comment is confusing.  I don't know what it means.

removed it, as it is confusing and doesn't really belong here.

http://codereview.chromium.org/194052/diff/1/3
File base/scoped_clipboard_writer.cc (right):

http://codereview.chromium.org/194052/diff/1/3#newcode76
Line 76: // Construct the hyperlink.
On 2009/09/09 19:08:22, Peter Kasting wrote:
> Nit: This comment should go above the |html| declaration.

Done.

[Peter Kasting, 2009-09-09 19:39:37]

OK

[TVL, 2009-09-10 12:01:15]

this appears to be causing leaks on the mac bot -
http://build.chromium.org/buildbot/waterfall/builders/Chromium%20Mac%20(valgr...