Try 2 - Change how ui::Clipboard is accessed so there's only one per thread.

Try 2 - Change how ui::Clipboard is accessed so there's only one per thread.

Currently, there can be any number of Clipboard objects, which can be massively simplified. This removes interfaces for fetching the Clipboard and makes everyone go through a single static ui::Clipboard::GetForCurrentThread() access point.

[Differences from the first patch: this may fix the pdf tests, but since I have no way to run them locally or on a trybot, this is going in blind.]

BUG=130805
R=jam@chromium.org
TBR=tc,ben,dcheng,kaiwang
First Review URL: https://chromiumcodereview.appspot.com/10911074

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

[Elliot Glaysher, 2012-09-10 17:52:41]

John, since you have access to the pdf renderer, could you please test this? I
fixed the compile problems that I saw in the official buildbot output.

[tony, 2012-09-10 18:15:14]

It seems like it will now be easy to accidentally call
ui::Clipboard::GetForCurrentThread() on the wrong thread.  Previously, when
going through g_browser_process->clipboard(), we had a dcheck to make sure you
were using it on the UI thread.  Maybe we can add that method back (it would
just call ui::Clipboard::GetForCurrentThread())?

https://chromiumcodereview.appspot.com/10916214/diff/1/content/browser/render...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10916214/diff/1/content/browser/render...
content/browser/renderer_host/clipboard_message_filter.cc:35:
ui::Clipboard::BUFFER_STANDARD, *objects);
Will valgrind complain about this as a leak?  Maybe you can still use static to
avoid that?

https://chromiumcodereview.appspot.com/10916214/diff/1/content/browser/render...
content/browser/renderer_host/clipboard_message_filter.cc:247: static
ui::Clipboard* clipboard = ui::Clipboard::GetForCurrentThread();
Does this still need to be static?

[Elliot Glaysher, 2012-09-10 18:29:31]

Part of the motivation here was also simplifying the views interface; any user
could subvert the check just by using the ui singleton method instead of the
views delegate. And the ui layer doesn't know about BrowserThreads, which are
defined at the content layer.

https://chromiumcodereview.appspot.com/10916214/diff/1/content/browser/render...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10916214/diff/1/content/browser/render...
content/browser/renderer_host/clipboard_message_filter.cc:35:
ui::Clipboard::BUFFER_STANDARD, *objects);
On 2012/09/10 18:15:14, tony wrote:
> Will valgrind complain about this as a leak?  Maybe you can still use static
to
> avoid that?

This was comited for a day before it was reverted due to pdf tests (which I
can't run) and I didn't get any notifications from the valgrind bots.

https://chromiumcodereview.appspot.com/10916214/diff/1/content/browser/render...
content/browser/renderer_host/clipboard_message_filter.cc:247: static
ui::Clipboard* clipboard = ui::Clipboard::GetForCurrentThread();
On 2012/09/10 18:15:14, tony wrote:
> Does this still need to be static?

I'd prefer it since I don't want to grab the lock each time.

[tony, 2012-09-10 18:44:40]

On 2012/09/10 18:29:31, Elliot Glaysher wrote:
> Part of the motivation here was also simplifying the views interface; any user
> could subvert the check just by using the ui singleton method instead of the
> views delegate.

Sure, but that was true before as well, but I think people looked at existing
code to figure out proper usage.  I think it would be easy to accidentally call
Clipboard::GetForCurrentThread() on the File thread now (e.g., you refactor some
code not realizing that it calls Clipboard::GetForCurrentThread()).

> And the ui layer doesn't know about BrowserThreads, which are
> defined at the content layer.

I'm not suggesting that the ui layer know about BrowserThreads, just that we
keep BrowserProcess::clipboard() that does the DCHECK and returns
Clipboard::GetForCurrentThread().

https://chromiumcodereview.appspot.com/10916214/diff/1/content/browser/render...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10916214/diff/1/content/browser/render...
content/browser/renderer_host/clipboard_message_filter.cc:35:
ui::Clipboard::BUFFER_STANDARD, *objects);
On 2012/09/10 18:29:31, Elliot Glaysher wrote:
> On 2012/09/10 18:15:14, tony wrote:
> > Will valgrind complain about this as a leak?  Maybe you can still use static
> to
> > avoid that?
> 
> This was comited for a day before it was reverted due to pdf tests (which I
> can't run) and I didn't get any notifications from the valgrind bots.

Wouldn't it be better for us to use a static to avoid having to relock :)

[jam, 2012-09-10 19:09:37]

i'll verify the pdf code after commit

[Elliot Glaysher, 2012-09-10 20:10:21]

I disagree that having a virtual method hanging off the BrowserProcess global
interface, which only calls a singleton method that anyone can call anyway, is
good design.

How about this instead? I've modified Clipboard so that it optionally takes a
list of PlatformThreadIds that can access Clipboard and will DCHECK that its on
one of those threads if it has it. That way, unit tests that don't set up the
threading information will work, and browser_tests that bring up the whole
shebang will exercise the thread check. This doesn't require keeping things on
the current interfaces.

jam: please do a real review of browser_main_loop.cc.

https://chromiumcodereview.appspot.com/10916214/diff/1/content/browser/render...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10916214/diff/1/content/browser/render...
content/browser/renderer_host/clipboard_message_filter.cc:35:
ui::Clipboard::BUFFER_STANDARD, *objects);
On 2012/09/10 18:44:41, tony wrote:
> On 2012/09/10 18:29:31, Elliot Glaysher wrote:
> > On 2012/09/10 18:15:14, tony wrote:
> > > Will valgrind complain about this as a leak?  Maybe you can still use
static
> > to
> > > avoid that?
> > 
> > This was comited for a day before it was reverted due to pdf tests (which I
> > can't run) and I didn't get any notifications from the valgrind bots.
> 
> Wouldn't it be better for us to use a static to avoid having to relock :)

oic what you're saying now. misunderstood before.

[tony, 2012-09-10 20:20:06]

On 2012/09/10 20:10:21, Elliot Glaysher wrote:
> How about this instead? I've modified Clipboard so that it optionally takes a
> list of PlatformThreadIds that can access Clipboard and will DCHECK that its
on
> one of those threads if it has it. That way, unit tests that don't set up the
> threading information will work, and browser_tests that bring up the whole
> shebang will exercise the thread check. This doesn't require keeping things on
> the current interfaces.

I like this approach. ui/base/clipboard LGTM.

[jam, 2012-09-10 22:41:56]

lgtm

[jam, 2012-09-10 22:44:18]

https://codereview.chromium.org/10916214/diff/12005/content/browser/browser_m...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/10916214/diff/12005/content/browser/browser_m...
content/browser/browser_main_loop.cc:478: // Alert the clipboard class to which
threads are allowed to access the
please move this to BrowserThreadsStarted since this function should be just to
start the threads

[commit-bot: I haz the power, 2012-09-10 23:11:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/10916214/15001

[commit-bot: I haz the power, 2012-09-11 01:10:24]

Try job failure for 10916214-15001 (retry) (retry) on win for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...