Implement clipboard for aura and re-enable clipboard_unittests.

Implement clipboard for aura and re-enable clipboard_unittests.

BUG=97845
TEST=ui_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107529

[sadrul, 2011-10-21 19:58:07]

This looks good. Left a few comments.

http://codereview.chromium.org/8364037/diff/3003/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_aura.cc (right):

http://codereview.chromium.org/8364037/diff/3003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aura.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
A general note: I think this clipboard_aura is used only in linux, and not on
windows. If so, perhaps it's a good idea to rename this file to
clipboard_aurax11.cc

http://codereview.chromium.org/8364037/diff/3003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aura.cc:132: size_t data_len_;
Call these custom_.._? (data_data_ sounds weird)

http://codereview.chromium.org/8364037/diff/3003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aura.cc:140: ClipboardData* data = NULL;
Perhaps it's a good time to rename this. 'data' can be a tricky name for a
global variable.

http://codereview.chromium.org/8364037/diff/3003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aura.cc:179: // We need to over write previous data.
Probably best to just delete
overwrite

[varunjain, 2011-10-21 21:03:46]

http://codereview.chromium.org/8364037/diff/3003/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_aura.cc (right):

http://codereview.chromium.org/8364037/diff/3003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aura.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/10/21 19:58:07, sadrul wrote:
> A general note: I think this clipboard_aura is used only in linux, and not on
> windows. If so, perhaps it's a good idea to rename this file to
> clipboard_aurax11.cc

Done.

http://codereview.chromium.org/8364037/diff/3003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aura.cc:132: size_t data_len_;
On 2011/10/21 19:58:07, sadrul wrote:
> Call these custom_.._? (data_data_ sounds weird)

Done.

http://codereview.chromium.org/8364037/diff/3003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aura.cc:140: ClipboardData* data = NULL;
On 2011/10/21 19:58:07, sadrul wrote:
> Perhaps it's a good time to rename this. 'data' can be a tricky name for a
> global variable.

Done.

http://codereview.chromium.org/8364037/diff/3003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aura.cc:179: // We need to over write previous data.
Probably best to just delete
On 2011/10/21 19:58:07, sadrul wrote:
> overwrite

Done.

[oshima, 2011-10-21 21:32:42]

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:26: web_smart_paste_(false) { }
nit: just {}

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:37: 
I prefer to remove new line to group them together. That's easier to read and
common in chrome (i think).

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:122: std::string bookmark_title_;
utf8_

or just use text_ for text() and document somewhere.

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:126: uint8_t* bitmap_data_;
you can use scoped_generic_obj and remove CleanupBitmapData.

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:131: char* custom_data_data_;
scoped_ptr

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:189: }
can we exclude this for AURA?

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:273: byte_counter += 4;
can we use gfx::ConvertBetweenBGRAandRGBA?

[varunjain, 2011-10-24 05:51:52]

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:26: web_smart_paste_(false) { }
On 2011/10/21 21:32:43, oshima wrote:
> nit: just {}

Done.

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:37: 
On 2011/10/21 21:32:43, oshima wrote:
> I prefer to remove new line to group them together. That's easier to read and
> common in chrome (i think).

Done.

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:122: std::string bookmark_title_;
On 2011/10/21 21:32:43, oshima wrote:
> utf8_
> 
> or just use text_ for text() and document somewhere.

I am not sure I understand. are you suggesting to reuse the utf8_text_ field? ht
clipboard should be able to hold both text and bookmarks independently.

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:126: uint8_t* bitmap_data_;
On 2011/10/21 21:32:43, oshima wrote:
> you can use scoped_generic_obj and remove CleanupBitmapData.

actually I can just do scoped_ptr_malloc

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:131: char* custom_data_data_;
On 2011/10/21 21:32:43, oshima wrote:
> scoped_ptr

Done. but with scoped_array

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:189: }
On 2011/10/21 21:32:43, oshima wrote:
> can we exclude this for AURA?

Done.

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:273: byte_counter += 4;
On 2011/10/21 21:32:43, oshima wrote:
> can we use gfx::ConvertBetweenBGRAandRGBA?

I believe that function is not publicly visible. Besides its probably faster
this way since we want an int at the end and that function converts between
char*s

[oshima, 2011-10-24 23:31:26]

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:122: std::string bookmark_title_;
On 2011/10/24 05:51:53, varunjain wrote:
> On 2011/10/21 21:32:43, oshima wrote:
> > utf8_
> > 
> > or just use text_ for text() and document somewhere.
> 
> I am not sure I understand. are you suggesting to reuse the utf8_text_ field?
ht
> clipboard should be able to hold both text and bookmarks independently.

If you use utf8_ for text, then use it for other utf8 strings. Alternatively,
you can document which one is utf8 and which one is not.

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:131: char* custom_data_data_;
On 2011/10/24 05:51:53, varunjain wrote:
> On 2011/10/21 21:32:43, oshima wrote:
> > scoped_ptr
> 
> Done. but with scoped_array
yes, scoped_array

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:268: // RGBA values.
BGRA

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:273: byte_counter += 4;
On 2011/10/24 05:51:53, varunjain wrote:
> On 2011/10/21 21:32:43, oshima wrote:
> > can we use gfx::ConvertBetweenBGRAandRGBA?
> 
> I believe that function is not publicly visible. Besides its probably faster
> this way since we want an int at the end and that function converts between
> char*s

Actually, why we're converting back and forth? Can you add comment on why we're
doing this?

I'm not so convinced about performance either. This way requires to load between
memory and register, while their way doesn't, and 3 copies can be done in
parallel.
I'm not disagreeing with you, but it has to be measured to claim it's faster.

http://codereview.chromium.org/8364037/diff/4003/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8364037/diff/4003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:162: return data->bitmap_data();
!! data->bitmap_data();
(this is common pattern in chrome)

http://codereview.chromium.org/8364037/diff/4003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:164: return data->web_smart_paste();
ditto

http://codereview.chromium.org/8364037/diff/4003/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_unittest.cc (right):

http://codereview.chromium.org/8364037/diff/4003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_unittest.cc:299: // channels stay intact. So I am
turning this on only for aura.
how about other platforms such as mac and win?

[varunjain, 2011-10-25 21:04:40]

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:122: std::string bookmark_title_;
On 2011/10/24 23:31:26, oshima wrote:
> On 2011/10/24 05:51:53, varunjain wrote:
> > On 2011/10/21 21:32:43, oshima wrote:
> > > utf8_
> > > 
> > > or just use text_ for text() and document somewhere.
> > 
> > I am not sure I understand. are you suggesting to reuse the utf8_text_
field?
> ht
> > clipboard should be able to hold both text and bookmarks independently.
> 
> If you use utf8_ for text, then use it for other utf8 strings. Alternatively,
> you can document which one is utf8 and which one is not.

Done.

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:268: // RGBA values.
On 2011/10/24 23:31:26, oshima wrote:
> BGRA

Removed conversion.

http://codereview.chromium.org/8364037/diff/2003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:273: byte_counter += 4;
On 2011/10/24 23:31:26, oshima wrote:
> On 2011/10/24 05:51:53, varunjain wrote:
> > On 2011/10/21 21:32:43, oshima wrote:
> > > can we use gfx::ConvertBetweenBGRAandRGBA?
> > 
> > I believe that function is not publicly visible. Besides its probably faster
> > this way since we want an int at the end and that function converts between
> > char*s
> 
> Actually, why we're converting back and forth? Can you add comment on why
we're
> doing this?
> 
> I'm not so convinced about performance either. This way requires to load
between
> memory and register, while their way doesn't, and 3 copies can be done in
> parallel.
> I'm not disagreeing with you, but it has to be measured to claim it's faster.
> 

Removed conversion.

http://codereview.chromium.org/8364037/diff/4003/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8364037/diff/4003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:162: return data->bitmap_data();
On 2011/10/24 23:31:26, oshima wrote:
> !! data->bitmap_data();
> (this is common pattern in chrome)

Done.

http://codereview.chromium.org/8364037/diff/4003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_aurax11.cc:164: return data->web_smart_paste();
On 2011/10/24 23:31:26, oshima wrote:
> ditto

web smart paste is already a bool

http://codereview.chromium.org/8364037/diff/4003/ui/base/clipboard/clipboard_...
File ui/base/clipboard/clipboard_unittest.cc (right):

http://codereview.chromium.org/8364037/diff/4003/ui/base/clipboard/clipboard_...
ui/base/clipboard/clipboard_unittest.cc:299: // channels stay intact. So I am
turning this on only for aura.
On 2011/10/24 23:31:26, oshima wrote:
> how about other platforms such as mac and win?

it passes the trybot win_aura. So I think it is ok on win. I will also put it
through other trybots

[oshima, 2011-10-25 21:10:10]

LGTM with nit

http://codereview.chromium.org/8364037/diff/13001/ui/base/clipboard/clipboard...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8364037/diff/13001/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:87: std::string markup_data_;
is this utf8 too?

[varunjain, 2011-10-25 21:23:47]

http://codereview.chromium.org/8364037/diff/13001/ui/base/clipboard/clipboard...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8364037/diff/13001/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:87: std::string markup_data_;
On 2011/10/25 21:10:10, oshima wrote:
> is this utf8 too?

Done.

[varunjain, 2011-10-25 22:39:18]

Ben/Sky.. need owners LGTM


On 2011/10/25 21:23:47, varunjain wrote:
>
http://codereview.chromium.org/8364037/diff/13001/ui/base/clipboard/clipboard...
> File ui/base/clipboard/clipboard_aurax11.cc (right):
> 
>
http://codereview.chromium.org/8364037/diff/13001/ui/base/clipboard/clipboard...
> ui/base/clipboard/clipboard_aurax11.cc:87: std::string markup_data_;
> On 2011/10/25 21:10:10, oshima wrote:
> > is this utf8 too?
> 
> Done.

[sky, 2011-10-25 23:55:35]

Don't you also have to clear the types at some point? It seems like if I were to
write some text, then a bookmark both are still available.

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:23: ClipboardData() : bitmap_data_(),
when you wrap, you need to go to next line then indent.

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:52: const gfx::Size bitmap_size() const {
return bitmap_size_; }
const gfx::Size&

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:71: return;
shouldn't this do custom_data_data_.reset() and clear custom_data_form_?

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:83: // Plain text in UTF8 format
End each sentence with a period.

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:203: void Clipboard::ReadHTML(Buffer
buffer, string16* markup, std::string* src_url,
each param on its own line.

http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp#newcode442
ui/ui.gyp:442: 'base/clipboard/clipboard_aurax11.cc',
I think this is handled by a rul in common.gypi.

[varunjain, 2011-10-26 05:41:40]

> Don't you also have to clear the types at some point? It seems like if I were
to
> write some text, then a bookmark both are still available.

I believe the normal usage of clipboard is through scoped_clipboard_writer which
erases everything before subsequent writes (a unit test in clipboard_unittest.cc
tests this)

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
File ui/base/clipboard/clipboard_aurax11.cc (right):

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:23: ClipboardData() : bitmap_data_(),
On 2011/10/25 23:55:35, sky wrote:
> when you wrap, you need to go to next line then indent.

Done.

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:52: const gfx::Size bitmap_size() const {
return bitmap_size_; }
On 2011/10/25 23:55:35, sky wrote:
> const gfx::Size&

Done.

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:71: return;
On 2011/10/25 23:55:35, sky wrote:
> shouldn't this do custom_data_data_.reset() and clear custom_data_form_?

Done.

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:83: // Plain text in UTF8 format
On 2011/10/25 23:55:35, sky wrote:
> End each sentence with a period.

Done.

http://codereview.chromium.org/8364037/diff/12002/ui/base/clipboard/clipboard...
ui/base/clipboard/clipboard_aurax11.cc:203: void Clipboard::ReadHTML(Buffer
buffer, string16* markup, std::string* src_url,
On 2011/10/25 23:55:35, sky wrote:
> each param on its own line.

Done.

http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp#newcode442
ui/ui.gyp:442: 'base/clipboard/clipboard_aurax11.cc',
On 2011/10/25 23:55:35, sky wrote:
> I think this is handled by a rul in common.gypi.

are you referring to build/common.gypi? the exclude there is only for
use_aura=0. This one is for OS=win since we want to use clipboard_win for win.

[sky, 2011-10-26 14:01:33]

http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp#newcode442
ui/ui.gyp:442: 'base/clipboard/clipboard_aurax11.cc',
On 2011/10/26 05:41:40, varunjain wrote:
> On 2011/10/25 23:55:35, sky wrote:
> > I think this is handled by a rul in common.gypi.
> 
> are you referring to build/common.gypi? the exclude there is only for
> use_aura=0. This one is for OS=win since we want to use clipboard_win for win.

Good point. I think you should add a rule that automatically includes aurax11
when use_aura==1 and use_x11==1

[varunjain, 2011-10-26 17:56:34]

http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp#newcode442
ui/ui.gyp:442: 'base/clipboard/clipboard_aurax11.cc',
On 2011/10/26 14:01:33, sky wrote:
> On 2011/10/26 05:41:40, varunjain wrote:
> > On 2011/10/25 23:55:35, sky wrote:
> > > I think this is handled by a rul in common.gypi.
> > 
> > are you referring to build/common.gypi? the exclude there is only for
> > use_aura=0. This one is for OS=win since we want to use clipboard_win for
win.
> 
> Good point. I think you should add a rule that automatically includes aurax11
> when use_aura==1 and use_x11==1

Done.

[sky, 2011-10-26 21:07:09]

On Wed, Oct 26, 2011 at 10:56 AM,  <varunjain@chromium.org> wrote:
>
> http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp
> File ui/ui.gyp (right):
>
> http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp#newcode442
> ui/ui.gyp:442: 'base/clipboard/clipboard_aurax11.cc',
> On 2011/10/26 14:01:33, sky wrote:
>>
>> On 2011/10/26 05:41:40, varunjain wrote:
>> > On 2011/10/25 23:55:35, sky wrote:
>> > > I think this is handled by a rul in common.gypi.
>> >
>> > are you referring to build/common.gypi? the exclude there is only
>
> for
>>
>> > use_aura=0. This one is for OS=win since we want to use
>
> clipboard_win for win.
>
>> Good point. I think you should add a rule that automatically includes
>
> aurax11
>>
>> when use_aura==1 and use_x11==1
>
> Done.

Did you upload a new patch?

  -Scott

[varunjain, 2011-10-26 21:09:46]

On 2011/10/26 21:07:09, sky wrote:
> On Wed, Oct 26, 2011 at 10:56 AM,  <mailto:varunjain@chromium.org> wrote:
> >
> > http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp
> > File ui/ui.gyp (right):
> >
> > http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp#newcode442
> > ui/ui.gyp:442: 'base/clipboard/clipboard_aurax11.cc',
> > On 2011/10/26 14:01:33, sky wrote:
> >>
> >> On 2011/10/26 05:41:40, varunjain wrote:
> >> > On 2011/10/25 23:55:35, sky wrote:
> >> > > I think this is handled by a rul in common.gypi.
> >> >
> >> > are you referring to build/common.gypi? the exclude there is only
> >
> > for
> >>
> >> > use_aura=0. This one is for OS=win since we want to use
> >
> > clipboard_win for win.
> >
> >> Good point. I think you should add a rule that automatically includes
> >
> > aurax11
> >>
> >> when use_aura==1 and use_x11==1
> >
> > Done.
> 
> Did you upload a new patch?
> 
>   -Scott

yep.. patch #10 has the change you suggested

[varunjain, 2011-10-27 03:13:42]

On 2011/10/26 14:01:33, sky wrote:
> http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp
> File ui/ui.gyp (right):
> 
> http://codereview.chromium.org/8364037/diff/12002/ui/ui.gyp#newcode442
> ui/ui.gyp:442: 'base/clipboard/clipboard_aurax11.cc',
> On 2011/10/26 05:41:40, varunjain wrote:
> > On 2011/10/25 23:55:35, sky wrote:
> > > I think this is handled by a rul in common.gypi.
> > 
> > are you referring to build/common.gypi? the exclude there is only for
> > use_aura=0. This one is for OS=win since we want to use clipboard_win for
win.
> 
> Good point. I think you should add a rule that automatically includes aurax11
> when use_aura==1 and use_x11==1

Done. PTAL

[sky, 2011-10-27 03:17:43]

LGTM

[commit-bot: I haz the power, 2011-10-27 03:18:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/8364037/22001

[commit-bot: I haz the power, 2011-10-27 04:19:08]

Change committed as 107529