Clear the clipboard closing Incognito window

Part I: clear primary and selection clipboards if data came from content area.

BUG=171974
TBR=asargent,sky

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

[vasilii, 2013-01-24 18:29:01]

Hi,

Could you review the change set? It comprises
1) Support of Incognito marker in ClipboardMessageFilter
2) Support of Incognito marker in GTK clipboard
3) Interactive ui test
4) Support of clipboard writes notifications

[dcheng, 2013-01-24 20:11:19]

Before I review the technical aspects... I have a larger question. Do we really
want this feature?

I've weakened my stance on this in the past, but looking at how I use Chrome, a
patch like this would be disruptive.

Personal anecdote: my online credit card account is linked to a separate Gmail
account. Every time I want to log in to my account, I do the following:
1) Go to mybank.com and start logging in. They email a one time security code to
me@gmail.com.
2) I login to me@gmail.com in an incognito window. Find the email from My Bank
and copy the security code into the clipboard. I then *close* the incognito
window (after all, why would I keep it open? I already have the information I
need in the clipboard)
3) Finish the login process to mybank.com.

If we start clearing the clipboard when we close the incognito profile, it adds
a few steps to my login process, as I now have to switch windows multiple times
between 2 and 3.

Now, I realize that this is just my use case. But I really think that copying
content to the clipboard when you're incognito mode is just one of those times
when you're explicitly moving data outside the incognito session (setting
bookmarks, saving pages, and downloading links are several other examples of
things that explicitly move data outside the incognito garden). As a result, I'm
unconvinced that users in general expect this behavior and that landing a patch
that adds this behavior would be unintuitive.

[battre, 2013-01-24 21:13:36]

On 2013/01/24 20:11:19, dcheng wrote:
> Before I review the technical aspects... I have a larger question. Do we
really
> want this feature?
> 
> I've weakened my stance on this in the past, but looking at how I use Chrome,
a
> patch like this would be disruptive.
> 
> Personal anecdote: my online credit card account is linked to a separate Gmail
> account. Every time I want to log in to my account, I do the following:
> 1) Go to http://mybank.com and start logging in. They email a one time
security code to
> mailto:me@gmail.com.
> 2) I login to mailto:me@gmail.com in an incognito window. Find the email from
My Bank
> and copy the security code into the clipboard. I then *close* the incognito
> window (after all, why would I keep it open? I already have the information I
> need in the clipboard)
> 3) Finish the login process to http://mybank.com.
> 
> If we start clearing the clipboard when we close the incognito profile, it
adds
> a few steps to my login process, as I now have to switch windows multiple
times
> between 2 and 3.
> 
> Now, I realize that this is just my use case. But I really think that copying
> content to the clipboard when you're incognito mode is just one of those times
> when you're explicitly moving data outside the incognito session (setting
> bookmarks, saving pages, and downloading links are several other examples of
> things that explicitly move data outside the incognito garden). As a result,
I'm
> unconvinced that users in general expect this behavior and that landing a
patch
> that adds this behavior would be unintuitive.

I see your usecase. Nevertheless, I think that from a privacy perspective the
risk of retaining sensitive data beyond an incognito session is bigger. The
reason is that copy&paste is a sensible operation within incongito mode (copy
the URL from one tab to a new one) and this information can easily be forgotten
in the clipboard. I feel pretty strongly, that we should err on the side of
deleting too much. I discussed the new strategy with Jeffrey Chang some time ago
(~1/2 year) and he agreed that we should change the behavior. To be frank, we
did not discuss your usecase, though.

Best regards,
Dominic

[dcheng, 2013-01-24 23:36:56]

On 2013/01/24 21:13:36, battre wrote:
> On 2013/01/24 20:11:19, dcheng wrote:
> > Before I review the technical aspects... I have a larger question. Do we
> really
> > want this feature?
> > 
> > I've weakened my stance on this in the past, but looking at how I use
Chrome,
> a
> > patch like this would be disruptive.
> > 
> > Personal anecdote: my online credit card account is linked to a separate
Gmail
> > account. Every time I want to log in to my account, I do the following:
> > 1) Go to http://mybank.com and start logging in. They email a one time
> security code to
> > mailto:me@gmail.com.
> > 2) I login to mailto:me@gmail.com in an incognito window. Find the email
from
> My Bank
> > and copy the security code into the clipboard. I then *close* the incognito
> > window (after all, why would I keep it open? I already have the information
I
> > need in the clipboard)
> > 3) Finish the login process to http://mybank.com.
> > 
> > If we start clearing the clipboard when we close the incognito profile, it
> adds
> > a few steps to my login process, as I now have to switch windows multiple
> times
> > between 2 and 3.
> > 
> > Now, I realize that this is just my use case. But I really think that
copying
> > content to the clipboard when you're incognito mode is just one of those
times
> > when you're explicitly moving data outside the incognito session (setting
> > bookmarks, saving pages, and downloading links are several other examples of
> > things that explicitly move data outside the incognito garden). As a result,
> I'm
> > unconvinced that users in general expect this behavior and that landing a
> patch
> > that adds this behavior would be unintuitive.
> 
> I see your usecase. Nevertheless, I think that from a privacy perspective the
> risk of retaining sensitive data beyond an incognito session is bigger. The
> reason is that copy&paste is a sensible operation within incongito mode (copy
> the URL from one tab to a new one) and this information can easily be
forgotten
> in the clipboard. I feel pretty strongly, that we should err on the side of
> deleting too much. I discussed the new strategy with Jeffrey Chang some time
ago
> (~1/2 year) and he agreed that we should change the behavior. To be frank, we
> did not discuss your usecase, though.
> 
> Best regards,
> Dominic

I understand your concern. It just seems to me that this behavior violates
normal behavior of the clipboard; it normally changes in response to copy and
cut operations, not closing a window.

You do mention a good point in http://crbug.com/123464 that the other operations
I mentioned were typically to interact with external applications; the problem
is while copy and paste does make sense just within Chrome, it often makes just
as much sense outside of Chrome or between Chrome windows, and there's no way to
distinguish between the two cases.

Now let's say this patch landed and users came to expect this behavior. Even
then they'd still be surprised when the clipboard mysteriously *isn't* cleared
sometimes. A simple example of when this would occur is if someone right-clicked
on a YT flash video and used the context menu to copy the video URL. It just
seems that while adding this behavior has good intentions, it just ends up being
too complicated (and flaky and unpredictable from a user standpoint).

[battre, 2013-01-25 09:43:17]

On 2013/01/24 23:36:56, dcheng wrote:
> On 2013/01/24 21:13:36, battre wrote:
> > On 2013/01/24 20:11:19, dcheng wrote:
> > > Before I review the technical aspects... I have a larger question. Do we
> > really
> > > want this feature?
> > > 
> > > I've weakened my stance on this in the past, but looking at how I use
> Chrome,
> > a
> > > patch like this would be disruptive.
> > > 
> > > Personal anecdote: my online credit card account is linked to a separate
> Gmail
> > > account. Every time I want to log in to my account, I do the following:
> > > 1) Go to http://mybank.com and start logging in. They email a one time
> > security code to
> > > mailto:me@gmail.com.
> > > 2) I login to mailto:me@gmail.com in an incognito window. Find the email
> from
> > My Bank
> > > and copy the security code into the clipboard. I then *close* the
incognito
> > > window (after all, why would I keep it open? I already have the
information
> I
> > > need in the clipboard)
> > > 3) Finish the login process to http://mybank.com.
> > > 
> > > If we start clearing the clipboard when we close the incognito profile, it
> > adds
> > > a few steps to my login process, as I now have to switch windows multiple
> > times
> > > between 2 and 3.
> > > 
> > > Now, I realize that this is just my use case. But I really think that
> copying
> > > content to the clipboard when you're incognito mode is just one of those
> times
> > > when you're explicitly moving data outside the incognito session (setting
> > > bookmarks, saving pages, and downloading links are several other examples
of
> > > things that explicitly move data outside the incognito garden). As a
result,
> > I'm
> > > unconvinced that users in general expect this behavior and that landing a
> > patch
> > > that adds this behavior would be unintuitive.
> > 
> > I see your usecase. Nevertheless, I think that from a privacy perspective
the
> > risk of retaining sensitive data beyond an incognito session is bigger. The
> > reason is that copy&paste is a sensible operation within incongito mode
(copy
> > the URL from one tab to a new one) and this information can easily be
> forgotten
> > in the clipboard. I feel pretty strongly, that we should err on the side of
> > deleting too much. I discussed the new strategy with Jeffrey Chang some time
> ago
> > (~1/2 year) and he agreed that we should change the behavior. To be frank,
we
> > did not discuss your usecase, though.
> > 
> > Best regards,
> > Dominic
> 
> I understand your concern. It just seems to me that this behavior violates
> normal behavior of the clipboard; it normally changes in response to copy and
> cut operations, not closing a window.
> 
> You do mention a good point in http://crbug.com/123464 that the other
operations
> I mentioned were typically to interact with external applications; the problem
> is while copy and paste does make sense just within Chrome, it often makes
just
> as much sense outside of Chrome or between Chrome windows, and there's no way
to
> distinguish between the two cases.
> 
> Now let's say this patch landed and users came to expect this behavior. Even
> then they'd still be surprised when the clipboard mysteriously *isn't* cleared
> sometimes. A simple example of when this would occur is if someone
right-clicked
> on a YT flash video and used the context menu to copy the video URL. It just
> seems that while adding this behavior has good intentions, it just ends up
being
> too complicated (and flaky and unpredictable from a user standpoint).

Vasilii pointed me to this class:
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/pepper/p...

Are you sure that we cannot handle clipboard content from YT flash videos given
that we now use Flapper?

Best regards,
Dominic

[dcheng, 2013-01-25 20:06:19]

On 2013/01/25 09:43:17, battre wrote:
> On 2013/01/24 23:36:56, dcheng wrote:
> > On 2013/01/24 21:13:36, battre wrote:
> > > On 2013/01/24 20:11:19, dcheng wrote:
> > > > Before I review the technical aspects... I have a larger question. Do we
> > > really
> > > > want this feature?
> > > > 
> > > > I've weakened my stance on this in the past, but looking at how I use
> > Chrome,
> > > a
> > > > patch like this would be disruptive.
> > > > 
> > > > Personal anecdote: my online credit card account is linked to a separate
> > Gmail
> > > > account. Every time I want to log in to my account, I do the following:
> > > > 1) Go to http://mybank.com and start logging in. They email a one time
> > > security code to
> > > > mailto:me@gmail.com.
> > > > 2) I login to mailto:me@gmail.com in an incognito window. Find the email
> > from
> > > My Bank
> > > > and copy the security code into the clipboard. I then *close* the
> incognito
> > > > window (after all, why would I keep it open? I already have the
> information
> > I
> > > > need in the clipboard)
> > > > 3) Finish the login process to http://mybank.com.
> > > > 
> > > > If we start clearing the clipboard when we close the incognito profile,
it
> > > adds
> > > > a few steps to my login process, as I now have to switch windows
multiple
> > > times
> > > > between 2 and 3.
> > > > 
> > > > Now, I realize that this is just my use case. But I really think that
> > copying
> > > > content to the clipboard when you're incognito mode is just one of those
> > times
> > > > when you're explicitly moving data outside the incognito session
(setting
> > > > bookmarks, saving pages, and downloading links are several other
examples
> of
> > > > things that explicitly move data outside the incognito garden). As a
> result,
> > > I'm
> > > > unconvinced that users in general expect this behavior and that landing
a
> > > patch
> > > > that adds this behavior would be unintuitive.
> > > 
> > > I see your usecase. Nevertheless, I think that from a privacy perspective
> the
> > > risk of retaining sensitive data beyond an incognito session is bigger.
The
> > > reason is that copy&paste is a sensible operation within incongito mode
> (copy
> > > the URL from one tab to a new one) and this information can easily be
> > forgotten
> > > in the clipboard. I feel pretty strongly, that we should err on the side
of
> > > deleting too much. I discussed the new strategy with Jeffrey Chang some
time
> > ago
> > > (~1/2 year) and he agreed that we should change the behavior. To be frank,
> we
> > > did not discuss your usecase, though.
> > > 
> > > Best regards,
> > > Dominic
> > 
> > I understand your concern. It just seems to me that this behavior violates
> > normal behavior of the clipboard; it normally changes in response to copy
and
> > cut operations, not closing a window.
> > 
> > You do mention a good point in http://crbug.com/123464 that the other
> operations
> > I mentioned were typically to interact with external applications; the
problem
> > is while copy and paste does make sense just within Chrome, it often makes
> just
> > as much sense outside of Chrome or between Chrome windows, and there's no
way
> to
> > distinguish between the two cases.
> > 
> > Now let's say this patch landed and users came to expect this behavior. Even
> > then they'd still be surprised when the clipboard mysteriously *isn't*
cleared
> > sometimes. A simple example of when this would occur is if someone
> right-clicked
> > on a YT flash video and used the context menu to copy the video URL. It just
> > seems that while adding this behavior has good intentions, it just ends up
> being
> > too complicated (and flaky and unpredictable from a user standpoint).
> 
> Vasilii pointed me to this class:
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/pepper/p...
> 
> Are you sure that we cannot handle clipboard content from YT flash videos
given
> that we now use Flapper?
> 
> Best regards,
> Dominic

We probably could. We could probably even update the PDF plugin to respect this
as well. But I'm still not convinced it's a good idea to special case the
operating system clipboard to behave differently from every other app.

We don't impose incognito behavior on favorites (a concept internal to Chrome)
so it doesn't make sense (to me) to impose incognito behavior on the clipboard.
Given that the clipboard is meant as a place to store information that's being
moved between working contexts, it also doesn't make sense to clear the
clipboard when the source context (e.g. incognito window) goes away on the
chance that the user's destination context was that same incognito window.

Maybe we should fix the favorites (or maybe it's by design, I don't know). But I
think that changing clipboard behavior here would be more confusing than
helpful.

[dcheng, 2013-01-25 21:09:27]

On 2013/01/25 20:06:19, dcheng wrote:
> On 2013/01/25 09:43:17, battre wrote:
> > On 2013/01/24 23:36:56, dcheng wrote:
> > > On 2013/01/24 21:13:36, battre wrote:
> > > > On 2013/01/24 20:11:19, dcheng wrote:
> > > > > Before I review the technical aspects... I have a larger question. Do
we
> > > > really
> > > > > want this feature?
> > > > > 
> > > > > I've weakened my stance on this in the past, but looking at how I use
> > > Chrome,
> > > > a
> > > > > patch like this would be disruptive.
> > > > > 
> > > > > Personal anecdote: my online credit card account is linked to a
separate
> > > Gmail
> > > > > account. Every time I want to log in to my account, I do the
following:
> > > > > 1) Go to http://mybank.com and start logging in. They email a one time
> > > > security code to
> > > > > mailto:me@gmail.com.
> > > > > 2) I login to mailto:me@gmail.com in an incognito window. Find the
email
> > > from
> > > > My Bank
> > > > > and copy the security code into the clipboard. I then *close* the
> > incognito
> > > > > window (after all, why would I keep it open? I already have the
> > information
> > > I
> > > > > need in the clipboard)
> > > > > 3) Finish the login process to http://mybank.com.
> > > > > 
> > > > > If we start clearing the clipboard when we close the incognito
profile,
> it
> > > > adds
> > > > > a few steps to my login process, as I now have to switch windows
> multiple
> > > > times
> > > > > between 2 and 3.
> > > > > 
> > > > > Now, I realize that this is just my use case. But I really think that
> > > copying
> > > > > content to the clipboard when you're incognito mode is just one of
those
> > > times
> > > > > when you're explicitly moving data outside the incognito session
> (setting
> > > > > bookmarks, saving pages, and downloading links are several other
> examples
> > of
> > > > > things that explicitly move data outside the incognito garden). As a
> > result,
> > > > I'm
> > > > > unconvinced that users in general expect this behavior and that
landing
> a
> > > > patch
> > > > > that adds this behavior would be unintuitive.
> > > > 
> > > > I see your usecase. Nevertheless, I think that from a privacy
perspective
> > the
> > > > risk of retaining sensitive data beyond an incognito session is bigger.
> The
> > > > reason is that copy&paste is a sensible operation within incongito mode
> > (copy
> > > > the URL from one tab to a new one) and this information can easily be
> > > forgotten
> > > > in the clipboard. I feel pretty strongly, that we should err on the side
> of
> > > > deleting too much. I discussed the new strategy with Jeffrey Chang some
> time
> > > ago
> > > > (~1/2 year) and he agreed that we should change the behavior. To be
frank,
> > we
> > > > did not discuss your usecase, though.
> > > > 
> > > > Best regards,
> > > > Dominic
> > > 
> > > I understand your concern. It just seems to me that this behavior violates
> > > normal behavior of the clipboard; it normally changes in response to copy
> and
> > > cut operations, not closing a window.
> > > 
> > > You do mention a good point in http://crbug.com/123464 that the other
> > operations
> > > I mentioned were typically to interact with external applications; the
> problem
> > > is while copy and paste does make sense just within Chrome, it often makes
> > just
> > > as much sense outside of Chrome or between Chrome windows, and there's no
> way
> > to
> > > distinguish between the two cases.
> > > 
> > > Now let's say this patch landed and users came to expect this behavior.
Even
> > > then they'd still be surprised when the clipboard mysteriously *isn't*
> cleared
> > > sometimes. A simple example of when this would occur is if someone
> > right-clicked
> > > on a YT flash video and used the context menu to copy the video URL. It
just
> > > seems that while adding this behavior has good intentions, it just ends up
> > being
> > > too complicated (and flaky and unpredictable from a user standpoint).
> > 
> > Vasilii pointed me to this class:
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/pepper/p...
> > 
> > Are you sure that we cannot handle clipboard content from YT flash videos
> given
> > that we now use Flapper?
> > 
> > Best regards,
> > Dominic
> 
> We probably could. We could probably even update the PDF plugin to respect
this
> as well. But I'm still not convinced it's a good idea to special case the
> operating system clipboard to behave differently from every other app.
> 
> We don't impose incognito behavior on favorites (a concept internal to Chrome)
> so it doesn't make sense (to me) to impose incognito behavior on the
clipboard.
> Given that the clipboard is meant as a place to store information that's being
> moved between working contexts, it also doesn't make sense to clear the
> clipboard when the source context (e.g. incognito window) goes away on the
> chance that the user's destination context was that same incognito window.
> 
> Maybe we should fix the favorites (or maybe it's by design, I don't know). But
I
> think that changing clipboard behavior here would be more confusing than
> helpful.

Someone else suggested expiring the content in the clipboard on a timeout
(1min-5min?) after closing the incognito window. What do you think of that idea?
It would definitely make things more complicated (and if Chrome is exiting, then
you can't wait for the time out and will just have to destroy it right away).

[dcheng, 2013-01-31 22:25:56]

https://codereview.chromium.org/12041078/diff/21001/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.h (right):

https://codereview.chromium.org/12041078/diff/21001/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.h:35:
base::SharedMemoryHandle bitmap_handle);
Is this the right place to add the clipboard hook? This will catch copy/cut from
WebKit, but what about code in Chrome that calls the other Clipboard::Write*
methods directly? Do we have a plan to address those?

https://codereview.chromium.org/12041078/diff/21001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/12041078/diff/21001/content/public/browser/co...
content/public/browser/content_browser_client.h:72: class
WebUIControllerFactory;
Are these two lines supposed to be part of this patch?

https://codereview.chromium.org/12041078/diff/21001/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/21001/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:184: virtual void OnWriteObjects(Buffer buffer) =
0;
I feel a little uncertain about having a ClipboardObserver that isn't really a
clipboard observer (it only triggers in one very specific circumstance)

Would it be better to write a UI test util that detects when the clipboard
changes? Win and Linux both have support for receiving notifications when the
clipboard changes; Mac can be implemented by polling.

[battre, 2013-02-01 09:36:39]

https://codereview.chromium.org/12041078/diff/21001/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/21001/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:184: virtual void OnWriteObjects(Buffer buffer) =
0;
On 2013/01/31 22:25:56, dcheng wrote:
> I feel a little uncertain about having a ClipboardObserver that isn't really a
> clipboard observer (it only triggers in one very specific circumstance)
> 
> Would it be better to write a UI test util that detects when the clipboard
> changes? Win and Linux both have support for receiving notifications when the
> clipboard changes; Mac can be implemented by polling.

I think the polling on Mac would be very awkward. In particular it would make
the tests slow and potentially flaky (polling is highly discouraged in testing
by Pawel). Also we don't need to do the polling unless we are in unit tests.

Given that we need this observer only for unit testing, my proposal would be the
following:

Rename ClipboardObserver to ClipboardObserverForTesting and add a comment
// This observer notices and reports only write operations though class
Clipboard.
// Modifications from other applications in the operating system are not
reported.

WDYT?

[Jói, 2013-02-01 11:31:20]

API-wise, this looks relatively clean to me, although you shouldn't mention
incognito in the comment on the new ContentBrowserClient method (see my comment
below).

Looping in jam@ who wants to know about any API changes.

Cheers,
Jói

https://codereview.chromium.org/12041078/diff/21001/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.h (right):

https://codereview.chromium.org/12041078/diff/21001/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.h:33: void
OnWriteObjectsAsync(ui::Clipboard::ObjectMap objects);
Are you sure you want to pass by value here?

https://codereview.chromium.org/12041078/diff/21001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/12041078/diff/21001/content/public/browser/co...
content/public/browser/content_browser_client.h:451: // OffTheRecordProfile.
This documentation is for the abstract embedder callback named
BeforeWriteToClipboard. Thus it is not appropriate to document what the main
embedder (Chrome) does on the callback, but rather you should document the
contract for this callback.

Something like "Called before writing clipboard contents, this lets the embedder
modify the contents of Clipboard::ObjectMap."

https://codereview.chromium.org/12041078/diff/21001/content/public/browser/co...
content/public/browser/content_browser_client.h:453: const
ClipboardWriteCallbackParams& params) {}
Why a struct rather than just passing two separate parameters to this method?

[jam, 2013-02-01 17:08:16]

it looks like this can be entirely developed inside content, since
BrowserContext has IsOffTheRecord.

https://codereview.chromium.org/12041078/diff/11008/content/public/browser/cl...
File content/public/browser/clipboard_write_callback_params.h (right):

https://codereview.chromium.org/12041078/diff/11008/content/public/browser/cl...
content/public/browser/clipboard_write_callback_params.h:1: #ifndef
CONTENT_PUBLIC_BROWSER_CLIPBOARD_WRITE_CALLBACK_PARAMS_H_
agree with joi, don't add this and just use two parameters in the callback

[vasilii, 2013-02-01 17:25:08]

Daniel, Joi, John, I addressed your comments.

John, technically this feature could be implemented in 'content' layer. But
BrowserContext::IsOffTheRecord() is deprecated and whole 'Incognito' concept
belongs to chrome layer. Considering these facts we decided to stay in 'chrome'
layer.

https://codereview.chromium.org/12041078/diff/21001/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.h (right):

https://codereview.chromium.org/12041078/diff/21001/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.h:33: void
OnWriteObjectsAsync(ui::Clipboard::ObjectMap objects);
On 2013/02/01 11:31:20, Jói wrote:
> Are you sure you want to pass by value here?
I need a modifiable |objects|. So I removed all explicit copies from
implementation. I think it worth to delegate this job to compiler.

https://codereview.chromium.org/12041078/diff/21001/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.h:35:
base::SharedMemoryHandle bitmap_handle);
On 2013/01/31 22:25:56, dcheng wrote:
> Is this the right place to add the clipboard hook? This will catch copy/cut
from
> WebKit, but what about code in Chrome that calls the other Clipboard::Write*
> methods directly? Do we have a plan to address those?
You are right - this CL handles copy from WebKit only. Other places listed in
Issue 171974 are to be dealt with separately. I can't add the hook to the
clipboard, there is no way to get corresponding BrowserContext* .

https://codereview.chromium.org/12041078/diff/21001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/12041078/diff/21001/content/public/browser/co...
content/public/browser/content_browser_client.h:72: class
WebUIControllerFactory;
On 2013/01/31 22:25:56, dcheng wrote:
> Are these two lines supposed to be part of this patch?
No, this was improper manual merge.

https://codereview.chromium.org/12041078/diff/21001/content/public/browser/co...
content/public/browser/content_browser_client.h:451: // OffTheRecordProfile.
On 2013/02/01 11:31:20, Jói wrote:
> This documentation is for the abstract embedder callback named
> BeforeWriteToClipboard. Thus it is not appropriate to document what the main
> embedder (Chrome) does on the callback, but rather you should document the
> contract for this callback.
> 
> Something like "Called before writing clipboard contents, this lets the
embedder
> modify the contents of Clipboard::ObjectMap."

Done.

https://codereview.chromium.org/12041078/diff/21001/content/public/browser/co...
content/public/browser/content_browser_client.h:453: const
ClipboardWriteCallbackParams& params) {}
On 2013/02/01 11:31:20, Jói wrote:
> Why a struct rather than just passing two separate parameters to this method?
Clipboard::ObjectMap is a typedef. I didn't want to add a new header to this
file and wrapped Clipboard stuff.

https://codereview.chromium.org/12041078/diff/11008/content/public/browser/cl...
File content/public/browser/clipboard_write_callback_params.h (right):

https://codereview.chromium.org/12041078/diff/11008/content/public/browser/cl...
content/public/browser/clipboard_write_callback_params.h:1: #ifndef
CONTENT_PUBLIC_BROWSER_CLIPBOARD_WRITE_CALLBACK_PARAMS_H_
On 2013/02/01 17:08:16, jam wrote:
> agree with joi, don't add this and just use two parameters in the callback
Do you agree to introduce a dependence on ui/base/clipboard.h (see my reply to
joi's comment)?

[jam, 2013-02-01 17:35:29]

On 2013/02/01 17:25:08, vasilii wrote:
> Daniel, Joi, John, I addressed your comments.
> 
> John, technically this feature could be implemented in 'content' layer. But
> BrowserContext::IsOffTheRecord() is deprecated and whole 'Incognito' concept
> belongs to chrome layer. Considering these facts we decided to stay in
'chrome'
> layer.

yeah, initially we tried to move it out. but it's very widespread particularly
because of all the html5 storage features that live in src/wekbit. technically,
while incognito is a chrome feature, i think what we have is that content knows
about "non-persistent" BrowserContexts.

so given that that code already knows about it, and there are no plans to remove
it, it seems to belong there.

[vasilii, 2013-02-04 15:27:28]

John, I moved the feature to 'content'.

[jam, 2013-02-04 16:01:59]

https://codereview.chromium.org/12041078/diff/29001/chrome/browser/clipboard/...
File chrome/browser/clipboard/clipboard_interactive_uitest.cc (right):

https://codereview.chromium.org/12041078/diff/29001/chrome/browser/clipboard/...
chrome/browser/clipboard/clipboard_interactive_uitest.cc:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
since this feature is implemented in content, the test needs to be there as
well. see ContentBrowserTest::CreateOffTheRecordBrowser

https://codereview.chromium.org/12041078/diff/29001/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/12041078/diff/29001/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:161: if
(content::IsThisIncognitoMarkerInClipboard(this))
this should be handled inside the content layer. one way to do this is to make a
SupportsUserData::Data derived class inside content\browser that is added to
BrowserContext. Then its destructor can do this check.

https://codereview.chromium.org/12041078/diff/29001/content/public/browser/in...
File content/public/browser/incognito_marker.cc (right):

https://codereview.chromium.org/12041078/diff/29001/content/public/browser/in...
content/public/browser/incognito_marker.cc:6: #include <cstddef>
nit: per style guide, should be a blank line before this line, and another blank
line before browser_context.h include

https://codereview.chromium.org/12041078/diff/29001/content/public/browser/in...
content/public/browser/incognito_marker.cc:24: // Such data must be erased upon
exit from incognito mode.
nit: in this file refer to "off the record" instead of incognito, and also
"browser context" instead of "profile"

[dcheng, 2013-02-04 18:54:18]

On 2013/02/01 09:36:39, battre wrote:
>
https://codereview.chromium.org/12041078/diff/21001/ui/base/clipboard/clipboa...
> File ui/base/clipboard/clipboard.h (right):
> 
>
https://codereview.chromium.org/12041078/diff/21001/ui/base/clipboard/clipboa...
> ui/base/clipboard/clipboard.h:184: virtual void OnWriteObjects(Buffer buffer)
=
> 0;
> On 2013/01/31 22:25:56, dcheng wrote:
> > I feel a little uncertain about having a ClipboardObserver that isn't really
a
> > clipboard observer (it only triggers in one very specific circumstance)
> > 
> > Would it be better to write a UI test util that detects when the clipboard
> > changes? Win and Linux both have support for receiving notifications when
the
> > clipboard changes; Mac can be implemented by polling.
> 
> I think the polling on Mac would be very awkward. In particular it would make
> the tests slow and potentially flaky (polling is highly discouraged in testing
> by Pawel). Also we don't need to do the polling unless we are in unit tests.
> 
> Given that we need this observer only for unit testing, my proposal would be
the
> following:
> 
> Rename ClipboardObserver to ClipboardObserverForTesting and add a comment
> // This observer notices and reports only write operations though class
> Clipboard.
> // Modifications from other applications in the operating system are not
> reported.
> 
> WDYT?

That'd be a little better. It'd be nice if it could be optimized out completely
in a release build, but I guess that's not possible in this case.

However, I have a suggestion concerning the overall direction of this CL that I
think would make it simpler. Why not just update all the Clipboard::Write()
functions to take a intptr_t-sized "source tag"? This tag would happen to
contain the same data as the incognito marker proposed in this patch contains;
the difference is we just always write it.

On incognito profile destruction, we can just read the source tag (if any) and
clear if appropriate. In my opinion, this has the advantage of being
"fail-safe"; callers have to think about what tag they're going to pass in,
rather than having to remember to add an incongito marker explicitly. WDYT?

[vasilii, 2013-02-05 18:48:14]

John:
I moved everything to content. It seems I can't enforce a call to
OffTheRecordProfile destructor. So I had to split the test into 2 phases.

Daniel:
I agree, I implemented your solution.

[jam, 2013-02-06 04:56:36]

https://codereview.chromium.org/12041078/diff/45043/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/12041078/diff/45043/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.cc:24: // Notifies
ContentBrowserClient and writes objects to the clipborad.
nit: update comment and remove content_*client.h includes

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
File content/public/browser/off_the_record_marker.cc (right):

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
content/public/browser/off_the_record_marker.cc:20: ui::Clipboard::SourceTag
source_tag=
nit: space before '='

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
File content/public/browser/off_the_record_marker.h (right):

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
content/public/browser/off_the_record_marker.h:21: CONTENT_EXPORT
ui::Clipboard::SourceTag GetMarkerForOffTheRecordContext(
nit: don't need to export anymore

also, this file doesn't belong in public anymore.

you can even move the implementation into clipboard_message_filter.cc since it's
not used anywhere else

[vasilii, 2013-02-06 11:59:15]

https://codereview.chromium.org/12041078/diff/45043/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/12041078/diff/45043/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.cc:24: // Notifies
ContentBrowserClient and writes objects to the clipborad.
On 2013/02/06 04:56:36, jam wrote:
> nit: update comment and remove content_*client.h includes 

Done.

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
File content/public/browser/off_the_record_marker.cc (right):

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
content/public/browser/off_the_record_marker.cc:20: ui::Clipboard::SourceTag
source_tag=
On 2013/02/06 04:56:36, jam wrote:
> nit: space before '='

Done.

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
File content/public/browser/off_the_record_marker.h (right):

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
content/public/browser/off_the_record_marker.h:21: CONTENT_EXPORT
ui::Clipboard::SourceTag GetMarkerForOffTheRecordContext(
On 2013/02/06 04:56:36, jam wrote:
> nit: don't need to export anymore
> 
> also, this file doesn't belong in public anymore.
> 
> you can even move the implementation into clipboard_message_filter.cc since
it's
> not used anywhere else
I can't. clipboard_message_filter is not the only place where coping from
incognito occurs. An omnibox writes to the clipboard and should pass the same
SourceTag. The omnibox lives in chrome layer.
It will be a separate CL.

[dcheng, 2013-02-06 22:31:40]

https://codereview.chromium.org/12041078/diff/50023/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/12041078/diff/50023/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.cc:24: void
WriteObjectsWrapper(ui::Clipboard* clipboard,
Nit: No indent inside namespace.

https://codereview.chromium.org/12041078/diff/50023/content/public/browser/of...
File content/public/browser/off_the_record_marker.cc (right):

https://codereview.chromium.org/12041078/diff/50023/content/public/browser/of...
content/public/browser/off_the_record_marker.cc:12: const char*
kClipboardDestroyerKey = "ClipboardDestroyerKey";
Nit: const char kClipboardDestroyerKey[]

https://codereview.chromium.org/12041078/diff/50023/content/public/browser/of...
content/public/browser/off_the_record_marker.cc:51: }
Nit: newline here to mirror newline at beginning of namespace block.

https://codereview.chromium.org/12041078/diff/50023/content/public/browser/of...
File content/public/browser/off_the_record_marker.h (right):

https://codereview.chromium.org/12041078/diff/50023/content/public/browser/of...
content/public/browser/off_the_record_marker.h:22: BrowserContext* context);
Nit: match newline at beginning of namespace block.

https://codereview.chromium.org/12041078/diff/50023/content/test/clipboard_br...
File content/test/clipboard_browser_test.cc (right):

https://codereview.chromium.org/12041078/diff/50023/content/test/clipboard_br...
content/test/clipboard_browser_test.cc:18: private
ui::Clipboard::ClipboardObserverForTesting {
Google style is to use public inheritance.

https://codereview.chromium.org/12041078/diff/50023/content/test/clipboard_br...
content/test/clipboard_browser_test.cc:35: message_loop_runner_ = new
content::MessageLoopRunner;
Do we need to use a MessageLoopRunner here? Would it be possible to use
base::WaitableEvent instead?

https://codereview.chromium.org/12041078/diff/50023/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/12041078/diff/50023/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.cc:99: char bytes[kSourceTagSize];
Nit: uint8 instead of char for byte strings.

https://codereview.chromium.org/12041078/diff/50023/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard_chromeos.cc (right):

https://codereview.chromium.org/12041078/diff/50023/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_chromeos.cc:590: Clipboard::SourceTag
Clipboard::ReadSourceTag() const {
Can we just use ReadData() and WriteData() like the other clipboard
implementations? If we did that, could we move the implemention into
clipboard.cc directly and make minimal changes to the rest of the clipboard_*.cc
files?

That way, we can also move the SourceTag2Binary helpers into clipboard.cc as
well.

[jam, 2013-02-06 22:36:50]

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
File content/public/browser/off_the_record_marker.h (right):

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
content/public/browser/off_the_record_marker.h:21: CONTENT_EXPORT
ui::Clipboard::SourceTag GetMarkerForOffTheRecordContext(
On 2013/02/06 11:59:15, vasilii wrote:
> On 2013/02/06 04:56:36, jam wrote:
> > nit: don't need to export anymore
> > 
> > also, this file doesn't belong in public anymore.
> > 
> > you can even move the implementation into clipboard_message_filter.cc since
> it's
> > not used anywhere else
> I can't. clipboard_message_filter is not the only place where coping from
> incognito occurs. An omnibox writes to the clipboard and should pass the same
> SourceTag. The omnibox lives in chrome layer.
> It will be a separate CL.

i see. do you have a pointer to that cl (if it's already made)?

move this static method to the end of the static methods on BrowserContext
interface, then you can get rid of this header. move the implementation then to
browser_context.cc

[vasilii, 2013-02-07 17:30:05]

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
File content/public/browser/off_the_record_marker.h (right):

https://codereview.chromium.org/12041078/diff/45043/content/public/browser/of...
content/public/browser/off_the_record_marker.h:21: CONTENT_EXPORT
ui::Clipboard::SourceTag GetMarkerForOffTheRecordContext(
On 2013/02/06 22:36:50, jam wrote:
> On 2013/02/06 11:59:15, vasilii wrote:
> > On 2013/02/06 04:56:36, jam wrote:
> > > nit: don't need to export anymore
> > > 
> > > also, this file doesn't belong in public anymore.
> > > 
> > > you can even move the implementation into clipboard_message_filter.cc
since
> > it's
> > > not used anywhere else
> > I can't. clipboard_message_filter is not the only place where coping from
> > incognito occurs. An omnibox writes to the clipboard and should pass the
same
> > SourceTag. The omnibox lives in chrome layer.
> > It will be a separate CL.
> 
> i see. do you have a pointer to that cl (if it's already made)?
> 
> move this static method to the end of the static methods on BrowserContext
> interface, then you can get rid of this header. move the implementation then
to
> browser_context.cc
Omnibox part isn't implemented at the moment.
I moved everything. I didn't include 'clipboard.h' into header because it breaks
compilation of apps/app_restore_service.cc

https://codereview.chromium.org/12041078/diff/50023/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/12041078/diff/50023/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.cc:24: void
WriteObjectsWrapper(ui::Clipboard* clipboard,
On 2013/02/06 22:31:40, dcheng wrote:
> Nit: No indent inside namespace.

Done.

https://codereview.chromium.org/12041078/diff/50023/content/public/browser/of...
File content/public/browser/off_the_record_marker.cc (right):

https://codereview.chromium.org/12041078/diff/50023/content/public/browser/of...
content/public/browser/off_the_record_marker.cc:12: const char*
kClipboardDestroyerKey = "ClipboardDestroyerKey";
On 2013/02/06 22:31:40, dcheng wrote:
> Nit: const char kClipboardDestroyerKey[]

Done.

https://codereview.chromium.org/12041078/diff/50023/content/public/browser/of...
content/public/browser/off_the_record_marker.cc:51: }
On 2013/02/06 22:31:40, dcheng wrote:
> Nit: newline here to mirror newline at beginning of namespace block.

Done.

https://codereview.chromium.org/12041078/diff/50023/content/public/browser/of...
File content/public/browser/off_the_record_marker.h (right):

https://codereview.chromium.org/12041078/diff/50023/content/public/browser/of...
content/public/browser/off_the_record_marker.h:22: BrowserContext* context);
On 2013/02/06 22:31:40, dcheng wrote:
> Nit: match newline at beginning of namespace block.

Done.

https://codereview.chromium.org/12041078/diff/50023/content/test/clipboard_br...
File content/test/clipboard_browser_test.cc (right):

https://codereview.chromium.org/12041078/diff/50023/content/test/clipboard_br...
content/test/clipboard_browser_test.cc:18: private
ui::Clipboard::ClipboardObserverForTesting {
On 2013/02/06 22:31:40, dcheng wrote:
> Google style is to use public inheritance.

Done.

https://codereview.chromium.org/12041078/diff/50023/content/test/clipboard_br...
content/test/clipboard_browser_test.cc:35: message_loop_runner_ = new
content::MessageLoopRunner;
On 2013/02/06 22:31:40, dcheng wrote:
> Do we need to use a MessageLoopRunner here? Would it be possible to use
> base::WaitableEvent instead?
No, copying happens on UI thread and we must dispatch messages to make it
possible. That is, OnWriteObjects() should be called on UI thread. Should I use
ThreadChecker?

https://codereview.chromium.org/12041078/diff/50023/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/12041078/diff/50023/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.cc:99: char bytes[kSourceTagSize];
On 2013/02/06 22:31:40, dcheng wrote:
> Nit: uint8 instead of char for byte strings.

Done.

https://codereview.chromium.org/12041078/diff/50023/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard_chromeos.cc (right):

https://codereview.chromium.org/12041078/diff/50023/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_chromeos.cc:590: Clipboard::SourceTag
Clipboard::ReadSourceTag() const {
On 2013/02/06 22:31:40, dcheng wrote:
> Can we just use ReadData() and WriteData() like the other clipboard
> implementations? If we did that, could we move the implemention into
> clipboard.cc directly and make minimal changes to the rest of the
clipboard_*.cc
> files?
> 
> That way, we can also move the SourceTag2Binary helpers into clipboard.cc as
> well.
Unfortunately no. ClipboardData has an explicit member for each supported type.
If I use WriteData() it can overwrite user's custom data.
SourceTag2Binary helpers are already in clipboard.cc.

[jam, 2013-02-07 21:52:17]

content lgtm with nits

https://codereview.chromium.org/12041078/diff/51004/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.h (right):

https://codereview.chromium.org/12041078/diff/51004/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.h:70: // this class
notifies ContentBrowserClient and passes |browser_context_|.
nit: don't document what the code does with this (someone can easily find this
by looking at the cc file).

https://codereview.chromium.org/12041078/diff/51004/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/12041078/diff/51004/content/public/browser/br...
content/public/browser/browser_context.h:102: // destruction it will be flushed.
nit: this comment is a bit verbose. can you shrink it down a bit?

also, the return type should be a "ui::Clipboard::SourceTag" (then you wouldn't
need to say that in the comment) so that it matches the cc file.

i.e. Returns a tag, for OffTheRecord contexts only, that identifies the contents
of the clipboard as such.

https://codereview.chromium.org/12041078/diff/51004/content/test/clipboard_br...
File content/test/clipboard_browser_test.cc (right):

https://codereview.chromium.org/12041078/diff/51004/content/test/clipboard_br...
content/test/clipboard_browser_test.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
move this test to content/browser/renderer_host/
also just call it clipboard_browsertest.cc (most browsertests don't have
underscore in the middle)

https://codereview.chromium.org/12041078/diff/51004/content/test/clipboard_br...
content/test/clipboard_browser_test.cc:35: message_loop_runner_ = new
content::MessageLoopRunner;
nit: no content::

[dcheng, 2013-02-08 05:42:19]

ui/base/clipboard LGTM with nits.

https://codereview.chromium.org/12041078/diff/50023/content/test/clipboard_br...
File content/test/clipboard_browser_test.cc (right):

https://codereview.chromium.org/12041078/diff/50023/content/test/clipboard_br...
content/test/clipboard_browser_test.cc:35: message_loop_runner_ = new
content::MessageLoopRunner;
On 2013/02/07 17:30:05, vasilii wrote:
> On 2013/02/06 22:31:40, dcheng wrote:
> > Do we need to use a MessageLoopRunner here? Would it be possible to use
> > base::WaitableEvent instead?
> No, copying happens on UI thread and we must dispatch messages to make it
> possible. That is, OnWriteObjects() should be called on UI thread. Should I
use
> ThreadChecker?

I see, I didn't realize that it was important to dispatch messages as well. This
is fine then.

https://codereview.chromium.org/12041078/diff/50023/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard_chromeos.cc (right):

https://codereview.chromium.org/12041078/diff/50023/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_chromeos.cc:590: Clipboard::SourceTag
Clipboard::ReadSourceTag() const {
On 2013/02/07 17:30:05, vasilii wrote:
> On 2013/02/06 22:31:40, dcheng wrote:
> > Can we just use ReadData() and WriteData() like the other clipboard
> > implementations? If we did that, could we move the implemention into
> > clipboard.cc directly and make minimal changes to the rest of the
> clipboard_*.cc
> > files?
> > 
> > That way, we can also move the SourceTag2Binary helpers into clipboard.cc as
> > well.
> Unfortunately no. ClipboardData has an explicit member for each supported
type.
> If I use WriteData() it can overwrite user's custom data.
> SourceTag2Binary helpers are already in clipboard.cc.

... yikes. That's a bug. This is fine for now, but mind filing a cleanup bug for
this? You can assign it to me.

(As for the helpers, I meant moving them into an unnamed namespace into
clipboard.cc)

https://codereview.chromium.org/12041078/diff/51004/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/12041078/diff/51004/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.cc:24: void
WriteObjectsWrapper(ui::Clipboard* clipboard,
Hmm... WriteObjectsWrapper and WriteObjectsHelper could probably use a better
name now that we have two of them, but I don't have any good suggestions...

https://codereview.chromium.org/12041078/diff/51004/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/51004/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:178: static SourceTag Binary2SourceTag(const
std::string& serialization);
Could you add a TODO here that eventually all public Write* methods should
require a SourceTag argument?

https://codereview.chromium.org/12041078/diff/51004/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:370: static const FormatType&
GetSourceTagFormatType();
Move this to lines 300-316 so it's with all the other FormatType functions.

[vasilii, 2013-02-08 09:27:36]

John:
I need you to answer my comment in browser_context.h

Daniel:
Your comments are addressed.

Joi:
Do you want to review?

Dominic:
I'm looking forward to your comments.

https://codereview.chromium.org/12041078/diff/51004/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/12041078/diff/51004/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.cc:24: void
WriteObjectsWrapper(ui::Clipboard* clipboard,
On 2013/02/08 05:42:19, dcheng wrote:
> Hmm... WriteObjectsWrapper and WriteObjectsHelper could probably use a better
> name now that we have two of them, but I don't have any good suggestions...
What about WriteObjectsOnUIThread?

https://codereview.chromium.org/12041078/diff/51004/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.h (right):

https://codereview.chromium.org/12041078/diff/51004/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.h:70: // this class
notifies ContentBrowserClient and passes |browser_context_|.
On 2013/02/07 21:52:17, jam wrote:
> nit: don't document what the code does with this (someone can easily find this
> by looking at the cc file).

Done.

https://codereview.chromium.org/12041078/diff/51004/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/12041078/diff/51004/content/public/browser/br...
content/public/browser/browser_context.h:102: // destruction it will be flushed.
On 2013/02/07 21:52:17, jam wrote:
> nit: this comment is a bit verbose. can you shrink it down a bit?
> 
> also, the return type should be a "ui::Clipboard::SourceTag" (then you
wouldn't
> need to say that in the comment) so that it matches the cc file.
> 
> i.e. Returns a tag, for OffTheRecord contexts only, that identifies the
contents
> of the clipboard as such.
If I include clipboard.h, apps/app_restore_service.cc compilation breaks. This
is because a command doesn't include /usr/include/gtk-2.0 directory and gtk.h
can't be found. How should I circumvent that?

https://codereview.chromium.org/12041078/diff/51004/content/test/clipboard_br...
File content/test/clipboard_browser_test.cc (right):

https://codereview.chromium.org/12041078/diff/51004/content/test/clipboard_br...
content/test/clipboard_browser_test.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/02/07 21:52:17, jam wrote:
> move this test to content/browser/renderer_host/
> also just call it clipboard_browsertest.cc (most browsertests don't have
> underscore in the middle)

Done.

https://codereview.chromium.org/12041078/diff/51004/content/test/clipboard_br...
content/test/clipboard_browser_test.cc:35: message_loop_runner_ = new
content::MessageLoopRunner;
On 2013/02/07 21:52:17, jam wrote:
> nit: no content::

Done.

https://codereview.chromium.org/12041078/diff/51004/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/51004/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:178: static SourceTag Binary2SourceTag(const
std::string& serialization);
On 2013/02/08 05:42:19, dcheng wrote:
> Could you add a TODO here that eventually all public Write* methods should
> require a SourceTag argument?
There is only one public method - WriteObjects(). Everyone uses
ScopedClipboardWriter with bunch of Write* methods. But I have modified it
already.

https://codereview.chromium.org/12041078/diff/51004/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:370: static const FormatType&
GetSourceTagFormatType();
On 2013/02/08 05:42:19, dcheng wrote:
> Move this to lines 300-316 so it's with all the other FormatType functions.

Done.

[Jói, 2013-02-08 10:37:32]

LGTM with nits.

https://codereview.chromium.org/12041078/diff/52040/content/browser/renderer_...
File content/browser/renderer_host/clipboard_browsertest.cc (right):

https://codereview.chromium.org/12041078/diff/52040/content/browser/renderer_...
content/browser/renderer_host/clipboard_browsertest.cc:16: namespace {
nit: I think we would typically include a blank line after this?

https://codereview.chromium.org/12041078/diff/52040/content/browser/renderer_...
content/browser/renderer_host/clipboard_browsertest.cc:102: } // namespace
content
nit: Typically there is a blank line before this.
nit: Two spaces before //

https://codereview.chromium.org/12041078/diff/52040/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/12041078/diff/52040/content/public/browser/br...
content/public/browser/browser_context.h:100: static void*
GetMarkerForOffTheRecordContext(BrowserContext* context);
void* -> Clipboard::SourceTag (?)

https://codereview.chromium.org/12041078/diff/52040/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/12041078/diff/52040/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.cc:97: union SourceTag2BinaryHelper {
Suggest documenting that a union is used as a pickling trick. I initially
mistook this for a struct and had a hard time figuring out the code below until
I realized my error.

[vasilii, 2013-02-08 12:37:12]

Joi:
I tweaked the code.

https://codereview.chromium.org/12041078/diff/52040/content/browser/renderer_...
File content/browser/renderer_host/clipboard_browsertest.cc (right):

https://codereview.chromium.org/12041078/diff/52040/content/browser/renderer_...
content/browser/renderer_host/clipboard_browsertest.cc:16: namespace {
On 2013/02/08 10:37:32, Jói wrote:
> nit: I think we would typically include a blank line after this?

Done.

https://codereview.chromium.org/12041078/diff/52040/content/browser/renderer_...
content/browser/renderer_host/clipboard_browsertest.cc:102: } // namespace
content
On 2013/02/08 10:37:32, Jói wrote:
> nit: Typically there is a blank line before this.
> nit: Two spaces before //

Done.

https://codereview.chromium.org/12041078/diff/52040/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/12041078/diff/52040/content/public/browser/br...
content/public/browser/browser_context.h:100: static void*
GetMarkerForOffTheRecordContext(BrowserContext* context);
On 2013/02/08 10:37:32, Jói wrote:
> void* -> Clipboard::SourceTag (?)
This is my pending question to John. Introducing clipboard.h header breaks
compilation. I can't forward-declare typedef.

https://codereview.chromium.org/12041078/diff/52040/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/12041078/diff/52040/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.cc:97: union SourceTag2BinaryHelper {
On 2013/02/08 10:37:32, Jói wrote:
> Suggest documenting that a union is used as a pickling trick. I initially
> mistook this for a struct and had a hard time figuring out the code below
until
> I realized my error.

Done.

[Jói, 2013-02-08 12:40:06]

LGTM, defer to John for the final review.

On Fri, Feb 8, 2013 at 12:37 PM,  <vasilii@chromium.org> wrote:
> Joi:
> I tweaked the code.
>
>
>
>
https://codereview.chromium.org/12041078/diff/52040/content/browser/renderer_...
> File content/browser/renderer_host/clipboard_browsertest.cc (right):
>
>
https://codereview.chromium.org/12041078/diff/52040/content/browser/renderer_...
> content/browser/renderer_host/clipboard_browsertest.cc:16: namespace {
> On 2013/02/08 10:37:32, Jói wrote:
>>
>> nit: I think we would typically include a blank line after this?
>
>
> Done.
>
>
>
https://codereview.chromium.org/12041078/diff/52040/content/browser/renderer_...
> content/browser/renderer_host/clipboard_browsertest.cc:102: } //
> namespace content
> On 2013/02/08 10:37:32, Jói wrote:
>>
>> nit: Typically there is a blank line before this.
>> nit: Two spaces before //
>
>
> Done.
>
>
>
https://codereview.chromium.org/12041078/diff/52040/content/public/browser/br...
> File content/public/browser/browser_context.h (right):
>
>
https://codereview.chromium.org/12041078/diff/52040/content/public/browser/br...
> content/public/browser/browser_context.h:100: static void*
> GetMarkerForOffTheRecordContext(BrowserContext* context);
> On 2013/02/08 10:37:32, Jói wrote:
>>
>> void* -> Clipboard::SourceTag (?)
>
> This is my pending question to John. Introducing clipboard.h header
> breaks compilation. I can't forward-declare typedef.
>
>
>
https://codereview.chromium.org/12041078/diff/52040/ui/base/clipboard/clipboa...
> File ui/base/clipboard/clipboard.cc (right):
>
>
https://codereview.chromium.org/12041078/diff/52040/ui/base/clipboard/clipboa...
> ui/base/clipboard/clipboard.cc:97: union SourceTag2BinaryHelper {
> On 2013/02/08 10:37:32, Jói wrote:
>>
>> Suggest documenting that a union is used as a pickling trick. I
>
> initially
>>
>> mistook this for a struct and had a hard time figuring out the code
>
> below until
>>
>> I realized my error.
>
>
> Done.
>
> https://codereview.chromium.org/12041078/

[dcheng, 2013-02-08 21:33:21]

https://codereview.chromium.org/12041078/diff/40040/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/40040/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:139: CBF_SOURCE_TAG, // Signifies the source of
data.
Btw, I think this line is no longer needed.

[jam, 2013-02-08 22:26:21]

https://codereview.chromium.org/12041078/diff/51004/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/12041078/diff/51004/content/public/browser/br...
content/public/browser/browser_context.h:102: // destruction it will be flushed.
On 2013/02/08 09:27:36, vasilii wrote:
> On 2013/02/07 21:52:17, jam wrote:
> > nit: this comment is a bit verbose. can you shrink it down a bit?
> > 
> > also, the return type should be a "ui::Clipboard::SourceTag" (then you
> wouldn't
> > need to say that in the comment) so that it matches the cc file.
> > 
> > i.e. Returns a tag, for OffTheRecord contexts only, that identifies the
> contents
> > of the clipboard as such.
> If I include clipboard.h, apps/app_restore_service.cc compilation breaks. This
> is because a command doesn't include /usr/include/gtk-2.0 directory and gtk.h
> can't be found. How should I circumvent that?

i'm not familiar with gtk. you can ask erg

[Elliot Glaysher, 2013-02-09 00:22:21]

https://codereview.chromium.org/12041078/diff/51004/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/12041078/diff/51004/content/public/browser/br...
content/public/browser/browser_context.h:102: // destruction it will be flushed.
On 2013/02/08 09:27:36, vasilii wrote:
> On 2013/02/07 21:52:17, jam wrote:
> > nit: this comment is a bit verbose. can you shrink it down a bit?
> > 
> > also, the return type should be a "ui::Clipboard::SourceTag" (then you
> wouldn't
> > need to say that in the comment) so that it matches the cc file.
> > 
> > i.e. Returns a tag, for OffTheRecord contexts only, that identifies the
> contents
> > of the clipboard as such.
> If I include clipboard.h, apps/app_restore_service.cc compilation breaks. This
> is because a command doesn't include /usr/include/gtk-2.0 directory and gtk.h
> can't be found. How should I circumvent that?

The only thing I can think of is that you're doing a linux_aura build. We don't
put gtk in the build path in most of linux_aura except for the very small
coordined off library that can access it. (We don't use ClipboardGtk there,
either.)

[vasilii, 2013-02-11 18:19:45]

Dominic asked to add support for selection clipboard to this CL. Please review
additional changes.
Dominic:
everything

Daniel:
major changes are in clipboard_gtk.cc and clipboard_aurax11.cc. Other files
changed slightly.

John and Joi:
browser_context.cc, render_widget_host_view_gtk.cc,
render_widget_host_view_aura.cc

Elliot:
A new test in incognito_selection_clipboard_uitest.cc. I can't implement it in
content layer as there is no way to destroy OfftheRecordProfile without
abandoning the process.

https://codereview.chromium.org/12041078/diff/40040/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/40040/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:139: CBF_SOURCE_TAG, // Signifies the source of
data.
On 2013/02/08 21:33:21, dcheng wrote:
> Btw, I think this line is no longer needed.

Done.

[Elliot Glaysher, 2013-02-11 18:28:53]

lgtm

[jam, 2013-02-11 21:28:53]

https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
File content/public/test/clipboard_write_observer.h (right):

https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
content/public/test/clipboard_write_observer.h:17: class ClipboardWriteObserver
:
i try to keep the number of files in content/public/test to a minimum, since the
more there are the harder it is to find the frequently used ones. with the other
suggestion about using a callback instead of an interface, this should go away.

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:189: class ClipboardObserverForTesting {
observer interfaces for one callback is too heavyweight imo. better to have a
method that takes a callback, and since it's only used for testing, call it as
such. i.e.

void AddWriteObjectsCallbackForTesting(const base::Callback<...)

[dcheng, 2013-02-11 22:58:21]

+tony as well, since he added the GTK bits in question.

https://codereview.chromium.org/12041078/diff/47048/chrome/browser/profiles/i...
File chrome/browser/profiles/incognito_selection_clipboard_uitest.cc (right):

https://codereview.chromium.org/12041078/diff/47048/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_selection_clipboard_uitest.cc:16: #if
defined(OS_MACOSX)
Do we need this if this file is only used for selection copy? AFAIK, a separate
selection clipboard only exists on Linux. Would it make sense to make this test
Linux only?

https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
File content/public/test/clipboard_write_observer.h (right):

https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
content/public/test/clipboard_write_observer.h:20: explicit
ClipboardWriteObserver(ui::Clipboard* clipboard,
explicit is only required for single-argument ctors.

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:412: // Reads raw data from the specified
clipboard with the given format type.
I think in the long run, I'd prefer to see ReadData() just take a Buffer
argument to match the other Read* functions.

Is this something you'd be willing to clean up in a follow up patch?

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard_gtk.cc (right):

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_gtk.cc:263: SetGtkClipboard(buffer, tag);
I'd prefer not to pass tag explicitly here. It's already in clipboard_data_
right?

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_gtk.cc:274: clipboard_data_ = new TargetMap;
I think this might be cleaner if you pull this logic up into WriteObjectsImpl.
It's a little icky to mess with clipboard_data_ here, even though it's
technically "OK", and that way, we won't have to plumb tag-specific code in
here. I'm thinking something like:

void Clipboard::WriteObjectsImpl(...) {
  // Current implementation here...
  SetGtkClipboard(buffer);
  if (/* clipboard has text and is the standard buffer */) {
    ObjectMap selection_objects;
    selection_objects.insert(/* iterator that points to text entry */);
    WriteObjectsImpl(BUFFER_SELECTION, selection_objects, tag);
  }
}

WDYT?

[battre, 2013-02-12 12:50:59]

I have just some nits.

https://codereview.chromium.org/12041078/diff/47048/chrome/browser/profiles/i...
File chrome/browser/profiles/incognito_selection_clipboard_uitest.cc (right):

https://codereview.chromium.org/12041078/diff/47048/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_selection_clipboard_uitest.cc:69: // the
selection clipboard after profile destruction.
Add comment that this is only executed on Linux because it is the only system
with a selection clipboard?

https://codereview.chromium.org/12041078/diff/47048/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_selection_clipboard_uitest.cc:111:
content::ClipboardWriteObserver clipboard_observer(
nit: indentation

https://codereview.chromium.org/12041078/diff/47048/content/browser/browser_c...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/12041078/diff/47048/content/browser/browser_c...
content/browser/browser_context.cc:107: }
nit: new lien before private:

https://codereview.chromium.org/12041078/diff/47048/content/browser/browser_c...
content/browser/browser_context.cc:110: ui::Clipboard::Buffer buffer) {
nit: indentation

https://codereview.chromium.org/12041078/diff/47048/content/browser/browser_c...
content/browser/browser_context.cc:121: kClipboardDestroyerKey))
nit: +4 spaces indentation

https://codereview.chromium.org/12041078/diff/47048/content/browser/browser_c...
content/browser/browser_context.cc:316: return
ui::Clipboard::SourceTag(clipboard_destroyer);
How about adding a function

ui::Clipboard::SourceTag OffTheRecordClipboardDestroyer::GetAsSourceTag() {
  return ui::Clipboard::SourceTag(this);
}

This way the knowledge about the use of source tags is more restricted to the
context of the OTRClipboardDestroyer.

https://codereview.chromium.org/12041078/diff/47048/content/browser/renderer_...
File content/browser/renderer_host/clipboard_browsertest.cc (right):

https://codereview.chromium.org/12041078/diff/47048/content/browser/renderer_...
content/browser/renderer_host/clipboard_browsertest.cc:42: // Select and copy
web-page content
nit: . at end of line. Generally I think it looks nice if you have a newline
before a comment.

https://codereview.chromium.org/12041078/diff/47048/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/12041078/diff/47048/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.cc:33: }
nit:
}  // namespace

https://codereview.chromium.org/12041078/diff/47048/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.cc:43: BrowserContext*
browser_context) {
nit: indentation

https://codereview.chromium.org/12041078/diff/47048/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/12041078/diff/47048/content/public/browser/br...
content/public/browser/browser_context.h:100: static void*
GetMarkerForOffTheRecordContext(BrowserContext* context);
I think we want to return a Clipboard::SourceTag here.

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard_gtk.cc (right):

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_gtk.cc:182: // inside upon failure.
I think the entire comment can be simplified to
// Sets the content of |clipboard| and takes ownership of |clipboard_data|.

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_gtk.cc:190: iter != clipboard_data->end(); ++iter,
++i) {
nit: +1 space

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_gtk.cc:269: if (GtkClipboardSetWithData(clipboard,
clipboard_data_))
please add {}

[vasilii, 2013-02-12 15:58:31]

https://codereview.chromium.org/12041078/diff/47048/chrome/browser/profiles/i...
File chrome/browser/profiles/incognito_selection_clipboard_uitest.cc (right):

https://codereview.chromium.org/12041078/diff/47048/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_selection_clipboard_uitest.cc:16: #if
defined(OS_MACOSX)
On 2013/02/11 22:58:21, dcheng wrote:
> Do we need this if this file is only used for selection copy? AFAIK, a
separate
> selection clipboard only exists on Linux. Would it make sense to make this
test
> Linux only?

Done.

https://codereview.chromium.org/12041078/diff/47048/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_selection_clipboard_uitest.cc:69: // the
selection clipboard after profile destruction.
On 2013/02/12 12:50:59, battre wrote:
> Add comment that this is only executed on Linux because it is the only system
> with a selection clipboard?

Done.

https://codereview.chromium.org/12041078/diff/47048/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_selection_clipboard_uitest.cc:111:
content::ClipboardWriteObserver clipboard_observer(
On 2013/02/12 12:50:59, battre wrote:
> nit: indentation

Done.

https://codereview.chromium.org/12041078/diff/47048/content/browser/browser_c...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/12041078/diff/47048/content/browser/browser_c...
content/browser/browser_context.cc:107: }
On 2013/02/12 12:50:59, battre wrote:
> nit: new lien before private:

Done.

https://codereview.chromium.org/12041078/diff/47048/content/browser/browser_c...
content/browser/browser_context.cc:110: ui::Clipboard::Buffer buffer) {
On 2013/02/12 12:50:59, battre wrote:
> nit: indentation

Done.

https://codereview.chromium.org/12041078/diff/47048/content/browser/browser_c...
content/browser/browser_context.cc:121: kClipboardDestroyerKey))
On 2013/02/12 12:50:59, battre wrote:
> nit: +4 spaces indentation

Done.

https://codereview.chromium.org/12041078/diff/47048/content/browser/browser_c...
content/browser/browser_context.cc:316: return
ui::Clipboard::SourceTag(clipboard_destroyer);
On 2013/02/12 12:50:59, battre wrote:
> How about adding a function
> 
> ui::Clipboard::SourceTag OffTheRecordClipboardDestroyer::GetAsSourceTag() {
>   return ui::Clipboard::SourceTag(this);
> }
> 
> This way the knowledge about the use of source tags is more restricted to the
> context of the OTRClipboardDestroyer.

Done.

https://codereview.chromium.org/12041078/diff/47048/content/browser/renderer_...
File content/browser/renderer_host/clipboard_browsertest.cc (right):

https://codereview.chromium.org/12041078/diff/47048/content/browser/renderer_...
content/browser/renderer_host/clipboard_browsertest.cc:42: // Select and copy
web-page content
On 2013/02/12 12:50:59, battre wrote:
> nit: . at end of line. Generally I think it looks nice if you have a newline
> before a comment.

Done.

https://codereview.chromium.org/12041078/diff/47048/content/browser/renderer_...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://codereview.chromium.org/12041078/diff/47048/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.cc:33: }
On 2013/02/12 12:50:59, battre wrote:
> nit:
> }  // namespace

Done.

https://codereview.chromium.org/12041078/diff/47048/content/browser/renderer_...
content/browser/renderer_host/clipboard_message_filter.cc:43: BrowserContext*
browser_context) {
On 2013/02/12 12:50:59, battre wrote:
> nit: indentation

Done.

https://codereview.chromium.org/12041078/diff/47048/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/12041078/diff/47048/content/public/browser/br...
content/public/browser/browser_context.h:100: static void*
GetMarkerForOffTheRecordContext(BrowserContext* context);
On 2013/02/12 12:50:59, battre wrote:
> I think we want to return a Clipboard::SourceTag here.
I agree, but apps/app_restore_service.cc fails to compile if I include
clipboard.h. It can't find gdk/gdk.h header.

https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
File content/public/test/clipboard_write_observer.h (right):

https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
content/public/test/clipboard_write_observer.h:17: class ClipboardWriteObserver
:
On 2013/02/11 21:28:53, jam wrote:
> i try to keep the number of files in content/public/test to a minimum, since
the
> more there are the harder it is to find the frequently used ones. with the
other
> suggestion about using a callback instead of an interface, this should go
away.
I don't understand how callback can help me. This class is shared between
chrome/browser/profiles/incognito_selection_clipboard_uitest.cc and
content/browser/renderer_host/clipboard_browsertest.cc. Where can I put this
code?

https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
content/public/test/clipboard_write_observer.h:20: explicit
ClipboardWriteObserver(ui::Clipboard* clipboard,
On 2013/02/11 22:58:21, dcheng wrote:
> explicit is only required for single-argument ctors.

Done.

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:412: // Reads raw data from the specified
clipboard with the given format type.
On 2013/02/11 22:58:21, dcheng wrote:
> I think in the long run, I'd prefer to see ReadData() just take a Buffer
> argument to match the other Read* functions.
> 
> Is this something you'd be willing to clean up in a follow up patch?
Yes, I agree to modify ReadData(). But note that ReadBookmark() is also
standalone.

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard_gtk.cc (right):

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_gtk.cc:182: // inside upon failure.
On 2013/02/12 12:50:59, battre wrote:
> I think the entire comment can be simplified to
> // Sets the content of |clipboard| and takes ownership of |clipboard_data|.
The whole function is reverted...

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_gtk.cc:190: iter != clipboard_data->end(); ++iter,
++i) {
On 2013/02/12 12:50:59, battre wrote:
> nit: +1 space

Done.

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_gtk.cc:263: SetGtkClipboard(buffer, tag);
On 2013/02/11 22:58:21, dcheng wrote:
> I'd prefer not to pass tag explicitly here. It's already in clipboard_data_
> right?

Done.

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_gtk.cc:269: if (GtkClipboardSetWithData(clipboard,
clipboard_data_))
On 2013/02/12 12:50:59, battre wrote:
> please add {}

Done.

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_gtk.cc:274: clipboard_data_ = new TargetMap;
On 2013/02/11 22:58:21, dcheng wrote:
> I think this might be cleaner if you pull this logic up into WriteObjectsImpl.
> It's a little icky to mess with clipboard_data_ here, even though it's
> technically "OK", and that way, we won't have to plumb tag-specific code in
> here. I'm thinking something like:
> 
> void Clipboard::WriteObjectsImpl(...) {
>   // Current implementation here...
>   SetGtkClipboard(buffer);
>   if (/* clipboard has text and is the standard buffer */) {
>     ObjectMap selection_objects;
>     selection_objects.insert(/* iterator that points to text entry */);
>     WriteObjectsImpl(BUFFER_SELECTION, selection_objects, tag);
>   }
> }
> 
> WDYT?

Done.

[vasilii, 2013-02-12 17:25:18]

John:
I addressed your comment.

https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
File content/public/test/clipboard_write_observer.h (right):

https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
content/public/test/clipboard_write_observer.h:17: class ClipboardWriteObserver
:
On 2013/02/11 21:28:53, jam wrote:
> i try to keep the number of files in content/public/test to a minimum, since
the
> more there are the harder it is to find the frequently used ones. with the
other
> suggestion about using a callback instead of an interface, this should go
away.
Please note that the current OnWriteObjects() not only interrupts the message
loop but also checks the |buffer|. Do you agree to copy/paste that logic across
the two test files?

[jam, 2013-02-12 17:31:13]

On 2013/02/12 17:25:18, vasilii wrote:
> John:
> I addressed your comment.

which comment? i still see the class in content/public/test, and the code in ui/
uses an observer instead of a callback?

> 
>
https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
> File content/public/test/clipboard_write_observer.h (right):
> 
>
https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
> content/public/test/clipboard_write_observer.h:17: class
ClipboardWriteObserver
> :
> On 2013/02/11 21:28:53, jam wrote:
> > i try to keep the number of files in content/public/test to a minimum, since
> the
> > more there are the harder it is to find the frequently used ones. with the
> other
> > suggestion about using a callback instead of an interface, this should go
> away.
> Please note that the current OnWriteObjects() not only interrupts the message
> loop but also checks the |buffer|. Do you agree to copy/paste that logic
across
> the two test files?

the one if-statement seems ok to copy.

[tony, 2013-02-12 18:41:09]

Bug 171974 is confusing since it's marked as a duplicate of a wontfix bug. 
Also, this change description should have more explanation.

I think this change is incorrect.  The clipboard's primary use is to move data
between applications.  This can be seen by how different the frequency of paste
and copy are (paste is 1.22 times more common than copy+cut in chrome and this
is why the paste button is the largest element in the Ribbon UI).  I'm worried
about use cases like Daniel's where the data loss is unrecoverable.

Also, this change misses some cases like the find-in-page text field (I don't
think copying text there goes through ui::Clipboard) and pressing ctrl+c on a
dialog on Windows (e.g., an alert()).

That said, I think it would be OK to try this on Canary/Dev, but I have a few
suggestions:
- Collect some metrics on how common it is to copy from an incognito window. 
You could even count Daniel's use case by landing most of this code except the
clearing part (copy from incognito, close incognito, paste into non-incognito).
- Consider updating the incognito text.  E.g., "...other traces, like cookies
and clipboard contents,..."

[jam, 2013-02-12 19:24:27]

also: has this gone through ui review?  please check with ui-leads.

[vasilii, 2013-02-13 16:57:25]

Dominic:
nits are fixed

Joi, John:
GetMarkerForOffTheRecordContext() return type fixed. This modification induced
massive header cleanup. Please review
chrome/browser/extensions/* and chrome/browser/ui/* . I added owners to TBR
field.

Toni:
Description has changed. Copying text from "find" dialog goes through Clipboard
on Aura, but does not go on GTK. I didn't aim to implement everything in this
CL, it's already huge.

https://codereview.chromium.org/12041078/diff/51004/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/12041078/diff/51004/content/public/browser/br...
content/public/browser/browser_context.h:102: // destruction it will be flushed.
On 2013/02/07 21:52:17, jam wrote:
> nit: this comment is a bit verbose. can you shrink it down a bit?
> 
> also, the return type should be a "ui::Clipboard::SourceTag" (then you
wouldn't
> need to say that in the comment) so that it matches the cc file.
> 
> i.e. Returns a tag, for OffTheRecord contexts only, that identifies the
contents
> of the clipboard as such.

Done.

https://codereview.chromium.org/12041078/diff/52040/content/public/browser/br...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/12041078/diff/52040/content/public/browser/br...
content/public/browser/browser_context.h:100: static void*
GetMarkerForOffTheRecordContext(BrowserContext* context);
On 2013/02/08 10:37:32, Jói wrote:
> void* -> Clipboard::SourceTag (?)

Done.

https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
File content/public/test/clipboard_write_observer.h (right):

https://codereview.chromium.org/12041078/diff/47048/content/public/test/clipb...
content/public/test/clipboard_write_observer.h:17: class ClipboardWriteObserver
:
On 2013/02/11 21:28:53, jam wrote:
> i try to keep the number of files in content/public/test to a minimum, since
the
> more there are the harder it is to find the frequently used ones. with the
other
> suggestion about using a callback instead of an interface, this should go
away.

Done.

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/47048/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:189: class ClipboardObserverForTesting {
On 2013/02/11 21:28:53, jam wrote:
> observer interfaces for one callback is too heavyweight imo. better to have a
> method that takes a callback, and since it's only used for testing, call it as
> such. i.e.
> 
> void AddWriteObjectsCallbackForTesting(const base::Callback<...)

Done.

[jam, 2013-02-14 07:27:58]

content lgtm

https://codereview.chromium.org/12041078/diff/66001/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/66001/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:189: void AddWriteObjectsCallback(const
base::Callback<void (Buffer)>& cb) {
since this is inline and it's a simple setter, it should be called

set_write_objects_callback()

and since it's a testing-only method, append _for_testing, so

set_write_objects_callback_for_testing()

[Jói, 2013-02-14 09:52:42]

LGTM for content/

[vasilii, 2013-02-14 10:01:00]

Dominic, Daniel:
any comments?

https://codereview.chromium.org/12041078/diff/66001/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/66001/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:189: void AddWriteObjectsCallback(const
base::Callback<void (Buffer)>& cb) {
On 2013/02/14 07:27:58, jam wrote:
> since this is inline and it's a simple setter, it should be called
> 
> set_write_objects_callback()
> 
> and since it's a testing-only method, append _for_testing, so
> 
> set_write_objects_callback_for_testing()

Done.

[battre, 2013-02-14 12:23:09]

lgtm

[dcheng, 2013-02-15 07:29:23]

https://codereview.chromium.org/12041078/diff/67001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_manager.h
(right):

https://codereview.chromium.org/12041078/diff/67001/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_manager.h:12:
#include "base/threading/thread_checker.h"
For future patches, it would be nice to separate IWYU cleanups. This patch is
already quite large =)

https://codereview.chromium.org/12041078/diff/67001/chrome/browser/profiles/i...
File chrome/browser/profiles/incognito_selection_clipboard_uitest.cc (right):

https://codereview.chromium.org/12041078/diff/67001/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_selection_clipboard_uitest.cc:16: const int
kCtrlOrCmdMask = ui::EF_CONTROL_DOWN;
Nit: we can just use this directly now.

https://codereview.chromium.org/12041078/diff/67001/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_selection_clipboard_uitest.cc:64: #if
defined(USE_X11) && !defined(OS_CHROMEOS)
I would just wrap the entire test implementation in #if defined(USE_X11) &&
!defined(OS_CHROMEOS). Then we can also get rid of the MAYBE_ prefixes. WDYT?

https://codereview.chromium.org/12041078/diff/67001/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/67001/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:420: base::Callback<void (Buffer)>
write_objects_callback_;
Nit. No space before (.

https://codereview.chromium.org/12041078/diff/67001/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/12041078/diff/67001/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_aurax11.cc:909: WriteText(&char_vector.front(),
char_vector.size());
Do we need to do the same thing that we do on GTK here?

[vasilii, 2013-02-15 10:37:39]

https://codereview.chromium.org/12041078/diff/67001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_manager.h
(right):

https://codereview.chromium.org/12041078/diff/67001/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_manager.h:12:
#include "base/threading/thread_checker.h"
On 2013/02/15 07:29:23, dcheng wrote:
> For future patches, it would be nice to separate IWYU cleanups. This patch is
> already quite large =)
I agree. But it was required to preserve successful compilation. I included
clipboard.h to browser_context.h and it propagated to many places where it
shouldn't have been.

https://codereview.chromium.org/12041078/diff/67001/chrome/browser/profiles/i...
File chrome/browser/profiles/incognito_selection_clipboard_uitest.cc (right):

https://codereview.chromium.org/12041078/diff/67001/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_selection_clipboard_uitest.cc:16: const int
kCtrlOrCmdMask = ui::EF_CONTROL_DOWN;
On 2013/02/15 07:29:23, dcheng wrote:
> Nit: we can just use this directly now.

Done.

https://codereview.chromium.org/12041078/diff/67001/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_selection_clipboard_uitest.cc:64: #if
defined(USE_X11) && !defined(OS_CHROMEOS)
On 2013/02/15 07:29:23, dcheng wrote:
> I would just wrap the entire test implementation in #if defined(USE_X11) &&
> !defined(OS_CHROMEOS). Then we can also get rid of the MAYBE_ prefixes. WDYT?

Done.

https://codereview.chromium.org/12041078/diff/67001/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/12041078/diff/67001/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard.h:420: base::Callback<void (Buffer)>
write_objects_callback_;
On 2013/02/15 07:29:23, dcheng wrote:
> Nit. No space before (.

Done.

https://codereview.chromium.org/12041078/diff/67001/ui/base/clipboard/clipboa...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/12041078/diff/67001/ui/base/clipboard/clipboa...
ui/base/clipboard/clipboard_aurax11.cc:909: WriteText(&char_vector.front(),
char_vector.size());
On 2013/02/15 07:29:23, dcheng wrote:
> Do we need to do the same thing that we do on GTK here?

Done.

[dcheng, 2013-02-15 18:26:26]

Clipboard bits lgtm.

[commit-bot: I haz the power, 2013-02-18 09:32:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/12041078/70001

[commit-bot: I haz the power, 2013-02-18 10:06:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/12041078/81001

[commit-bot: I haz the power, 2013-02-18 13:34:54]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-18 14:02:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/12041078/81001

[commit-bot: I haz the power, 2013-02-18 15:44:05]

Change committed as 183123