Implement ui::ClipboardMus.

Implement ui::ClipboardMus.

This lets chrome in mash read and write from the mus server's clipboard.

BUG=581460
Committed: https://crrev.com/bbc764eb7a7c1937d26f43f6087604ebc2d048ad
Cr-Commit-Position: refs/heads/master@{#398351}

[Elliot Glaysher, 2016-05-26 22:30:23]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[Elliot Glaysher, 2016-05-26 22:30:24]

This is now ready for review.

This implements a version of ui::Cilpboard for mus, lets it coexist in normal
chrome binaries as with the rest of views-mus, and gets the current
ui::Clipboard test suite passing against the new implementation.

There's also a handful of cleanups where things which were defined in most of
the Clipboard implementations were redefined in each and every one; they have
been centralized in the base class instead of copy/pasting once again.

[sky, 2016-05-26 23:10:25]

sky@chromium.org changed reviewers:
+ dcheng@chromium.org

[sky, 2016-05-26 23:10:27]

+dcheng

https://codereview.chromium.org/2011833003/diff/200001/components/mus/public/...
File components/mus/public/interfaces/clipboard.mojom (right):

https://codereview.chromium.org/2011833003/diff/200001/components/mus/public/...
components/mus/public/interfaces/clipboard.mojom:41: ReadMimeType(Type
clipboard_type, string mime_type)
WDYT of renaming to ReadClipboardData? That better matches WriteClipboardData.

How come you removed the sequence number? It seems like if read doesn't have a
sequence number there isn't much value in the sequence number. But I could
certainly be wrong.

https://codereview.chromium.org/2011833003/diff/200001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/2011833003/diff/200001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard.h:68: static const char kMimeTypeWebCustomData[];
Move over todo about removal?

https://codereview.chromium.org/2011833003/diff/200001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard.h:143: // Builds the clipboard for the current
thread. Previously, there was only
This comment is a bit confusing. It's not building, it's setting, right?

https://codereview.chromium.org/2011833003/diff/200001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard.h:147: // can't use unique_ptr since we don't want
to expose the destructor.)
Have this class friend unique_ptr? Alternatively take a unique_ptr to clarify
ownership but extract the Clipboard from it.

https://codereview.chromium.org/2011833003/diff/200001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_aurax11.cc (right):

https://codereview.chromium.org/2011833003/diff/200001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_aurax11.cc:60: NULL};
while you're here, nullptr

https://codereview.chromium.org/2011833003/diff/200001/ui/base/dragdrop/os_ex...
File ui/base/dragdrop/os_exchange_data_provider_aurax11.cc (right):

https://codereview.chromium.org/2011833003/diff/200001/ui/base/dragdrop/os_ex...
ui/base/dragdrop/os_exchange_data_provider_aurax11.cc:43: NULL};
nullptr

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_mus.cc (right):

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:27: default:
remove default and merge last with drag so you get compile error if new type
added.

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:51: else if (format.Equals(GetMozUrlFormatType()))
nit: no else after return.

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:76: return std::find(available_types.begin(),
available_types.end(), type) !=
ContainsValue(available_types, type)
in base/stl_util.h

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:91: uint64_t sequence_number = 0;
If you cache the types and the current sequence number then you wouldn't need to
ask every time this is called.

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:97: return std::find(available_types.begin(),
available_types.end(),
ContainsValue.

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_mus.h (right):

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.h:75: mojo::Map<mojo::String, mojo::Array<uint8_t>>
current_clipboard_;
I'm confused about when you use this and when you don't. For example, ReadHTML
calls to clipboard_ but WriteHtml writes to current_clipboard_.

[Elliot Glaysher, 2016-05-31 17:01:40]

https://codereview.chromium.org/2011833003/diff/200001/components/mus/public/...
File components/mus/public/interfaces/clipboard.mojom (right):

https://codereview.chromium.org/2011833003/diff/200001/components/mus/public/...
components/mus/public/interfaces/clipboard.mojom:41: ReadMimeType(Type
clipboard_type, string mime_type)
On 2016/05/26 23:10:26, sky wrote:
> WDYT of renaming to ReadClipboardData? That better matches WriteClipboardData.

Done.

> How come you removed the sequence number? It seems like if read doesn't have a
> sequence number there isn't much value in the sequence number. But I could
> certainly be wrong.

Looking at the underlying interface, this isn't how it works. Webkit and PPAPI
read the sequence number, read a bunch of objects, and then read the sequence
number again to make sure that the underlying clipboard hasn't changed.

If I was starting from scratch, I agree that moving the sequence number to read
would be much better design. As ui::Clipboard is written today, we don't have a
sequence number to send as part of the read call, and doing an additional sync
call isn't good.

I've added back the sequence number as a return value for future users who are
more clever, though it's ignored in the current ui::ClipboardMus implementation.

https://codereview.chromium.org/2011833003/diff/200001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/2011833003/diff/200001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard.h:68: static const char kMimeTypeWebCustomData[];
On 2016/05/26 23:10:27, sky wrote:
> Move over todo about removal?

The bug is closed and archived.

https://codereview.chromium.org/2011833003/diff/200001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard.h:147: // can't use unique_ptr since we don't want
to expose the destructor.)
On 2016/05/26 23:10:27, sky wrote:
> Have this class friend unique_ptr? Alternatively take a unique_ptr to clarify
> ownership but extract the Clipboard from it.

Done.

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_mus.cc (right):

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:91: uint64_t sequence_number = 0;
On 2016/05/26 23:10:27, sky wrote:
> If you cache the types and the current sequence number then you wouldn't need
to
> ask every time this is called.

If I cache the types and the sequence number, I'd have to fetch the sequence
number anyway to see if I then need to refetch the formats available. And I
can't (easily) change the ui::Clipboard interface to take the current sequence
number.

(Unless I then use something like a shared memory atomic for the sequence
number. Is that what you're asking for?)

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_mus.h (right):

https://codereview.chromium.org/2011833003/diff/200001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.h:75: mojo::Map<mojo::String, mojo::Array<uint8_t>>
current_clipboard_;
On 2016/05/26 23:10:27, sky wrote:
> I'm confused about when you use this and when you don't. For example, ReadHTML
> calls to clipboard_ but WriteHtml writes to current_clipboard_.

Elaborated about this temporary buffer.

[dcheng, 2016-05-31 17:46:46]

https://codereview.chromium.org/2011833003/diff/200001/components/mus/public/...
File components/mus/public/interfaces/clipboard.mojom (right):

https://codereview.chromium.org/2011833003/diff/200001/components/mus/public/...
components/mus/public/interfaces/clipboard.mojom:41: ReadMimeType(Type
clipboard_type, string mime_type)
On 2016/05/31 at 17:01:40, Elliot Glaysher wrote:
> On 2016/05/26 23:10:26, sky wrote:
> > WDYT of renaming to ReadClipboardData? That better matches
WriteClipboardData.
> 
> Done.
> 
> > How come you removed the sequence number? It seems like if read doesn't have
a
> > sequence number there isn't much value in the sequence number. But I could
> > certainly be wrong.
> 
> Looking at the underlying interface, this isn't how it works. Webkit and PPAPI
read the sequence number, read a bunch of objects, and then read the sequence
number again to make sure that the underlying clipboard hasn't changed.
> 
> If I was starting from scratch, I agree that moving the sequence number to
read would be much better design. As ui::Clipboard is written today, we don't
have a sequence number to send as part of the read call, and doing an additional
sync call isn't good.
> 
> I've added back the sequence number as a return value for future users who are
more clever, though it's ignored in the current ui::ClipboardMus implementation.

I haven't had time to look over this patch yet, but the reason Blink (and
probably PPAPI, since it was written based on what Blink did at the time) does
that is largely historical. I would support changing this. Seems a bit out of
scope for this particular patch though.

[sky, 2016-05-31 20:50:24]

https://codereview.chromium.org/2011833003/diff/300001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_mus.cc (right):

https://codereview.chromium.org/2011833003/diff/300001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:154: std::string text =
text_data.To<std::string>();
Do all of the converters succeed (not DCHECK/CHECK) if the array is null?

https://codereview.chromium.org/2011833003/diff/300001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:217: GetType(type),
mojo::String(mus::mojom::MIME_TYPE_RTF),
TYPE_PNG?

https://codereview.chromium.org/2011833003/diff/300001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_mus.h (right):

https://codereview.chromium.org/2011833003/diff/300001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.h:79: mojo::Map<mojo::String, mojo::Array<uint8_t>>
current_clipboard_;
std::unique_ptr and DCHECK that non-null in all writes? This makes it clearer
what is going on.

[Elliot Glaysher, 2016-06-01 19:41:14]

ptal

https://codereview.chromium.org/2011833003/diff/300001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_mus.cc (right):

https://codereview.chromium.org/2011833003/diff/300001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:154: std::string text =
text_data.To<std::string>();
On 2016/05/31 20:50:24, sky wrote:
> Do all of the converters succeed (not DCHECK/CHECK) if the array is null?

These type converters, defined in common_type_converters.cc, check for nullness
and return the empty string in that case.

[sky, 2016-06-01 20:23:25]

LGTM

[Elliot Glaysher, 2016-06-01 20:47:51]

erg@chromium.org changed reviewers:
+ brettw@chromium.org

[Elliot Glaysher, 2016-06-01 20:47:52]

dcheng: mojom review

brettw: base review. In the non-mus case, we called blocking OS functions in the
ui::Clipboard implementation which weren't caught by threading restrictions. Our
blocking calls to mus, however, are.

(If we were to drop all the existing clipboard usage through //content/ and
//chrome/, we could actually make this asynchronous! However, that's too large
of a change to make until much, much later in the musification of chrome.)

[Elliot Glaysher, 2016-06-03 21:04:13]

dcheng/brettw: friendly ping

[brettw, 2016-06-03 21:39:00]

base lgtm

https://codereview.chromium.org/2011833003/diff/360001/base/threading/thread_...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/2011833003/diff/360001/base/threading/thread_...
base/threading/thread_restrictions.h:239: friend class views::ClipboardMus;
I think it would be good to have a comment here about why this must be blocking
(like your review comment).

[dcheng, 2016-06-04 06:14:01]

Also, what's your plan with sequence numbers in the mojom interface? It seems
like they're pretty much not used at all atm.

https://codereview.chromium.org/2011833003/diff/360001/components/mus/public/...
File components/mus/public/interfaces/clipboard.mojom (right):

https://codereview.chromium.org/2011833003/diff/360001/components/mus/public/...
components/mus/public/interfaces/clipboard.mojom:7: const string MIME_TYPE_HTML
= "text/html";
Is the convention to name CONSTANTS_LIKE_THIS in Mojo? Isn't C++ convention
kNaming instead?

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard.cc:45: // before we've established our first mojo
connection to mus.
I'm a little worried about potential side effects of swapping out the Clipboard
implementation… there's no way to change things so we can order things more
sanely here?

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard.cc:46: delete it->second;
We may as well fix the clipboard map to store unique_ptrs.

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard.h:149: // can't use unique_ptr since we don't want
to expose the destructor.)
Is the last part of this comment still accurate?

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_test_template.h (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_test_template.h:372: if (!this->IsMusTest()) {
Why do we exclude mus here? Wouldn't we expect this to work on Linux, even with
mus?

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/cust...
File ui/base/clipboard/custom_data_helper_mac.mm (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/cust...
ui/base/clipboard/custom_data_helper_mac.mm:11: // TODO(dcheng): This name is
temporary. See crbug.com/106449.
Please move this comment to clipboard_constants.cc, where
chromium/x-web-custom-data is defined now, even though it's not particularly
temporary anymore =P

https://codereview.chromium.org/2011833003/diff/360001/ui/base/test/test_clip...
File ui/base/test/test_clipboard_unittest.cc (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/base/test/test_clip...
ui/base/test/test_clipboard_unittest.cc:18: return
PlatformEventSource::CreateDefault();
This can just return nullptr, I think.

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_mus.cc (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:34: const char kInternalURL[] =
"chromium/internal-url";
Either name this to reflect or comment that this is only used for the source URL
of markup (if any).

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:108: clipboard_->WriteClipboardData(GetType(type),
std::move(null_data),
You can probably just use nullptr directly as the second argument here.

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.cc:259: for (ObjectMap::const_iterator iter =
objects.begin(); iter != objects.end();
for (const auto& object : objects)
  DispatchObject(static_cast<ObjectType>(object.first), object.second);

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_mus.h (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.h:28: mojo::String GetMimeTypeFor(const FormatType&
format) const;
Does this actually need to be exposed in the header? At the very least, it seems
like it can be static.

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.h:79: std::unique_ptr<mojo::Map<mojo::String,
mojo::Array<uint8_t>>>
Why unique_ptr this? Can't we just clear it when we need to write new data in?

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_unittest.cc (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_unittest.cc:17: return
std::unique_ptr<PlatformEventSource>();
Nit: nullptr

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_unittest.cc:21: EXPECT_FALSE(test_helper.get());
Nit: EXPECT_FALSE(test_helper)

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_unittest.cc:47: PlatformClipboardTraits::test_helper;
I don't think we allow globals of non-POD type.

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_unittest.cc:49: typedef PlatformClipboardTraits
TypesToTest;
Nit: using (I guess this is copying the pattern in the existing test
instantiations, but we may as well use the recommended way in new code)

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/window_ma...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/window_ma...
ui/views/mus/window_manager_connection.cc:113: // We keep a weak ptr to
clipboard_
Stale comment? I don't see |clipboard_|

[Elliot Glaysher, 2016-06-06 20:10:12]

> Also, what's your plan with sequence numbers in the mojom interface?
> It seems like they're pretty much not used at all atm.

So currently, I've implemented GetSequenceNumber() for the current interfaces in
ui::Clipboard.

In the (very) long run, I'd like to get rid of GetSequenceNumber() everywhere
and modify the callers that need them to verify that different representations
are from the same clipboard state to be rolled into read. Currently, the
sequence number return value on read isn't used.

https://codereview.chromium.org/2011833003/diff/360001/components/mus/public/...
File components/mus/public/interfaces/clipboard.mojom (right):

https://codereview.chromium.org/2011833003/diff/360001/components/mus/public/...
components/mus/public/interfaces/clipboard.mojom:7: const string MIME_TYPE_HTML
= "text/html";
On 2016/06/04 06:14:00, dcheng wrote:
> Is the convention to name CONSTANTS_LIKE_THIS in Mojo? Isn't C++ convention
> kNaming instead?

It's inconsistent; I can find examples of each. Moved to kNaming anyway since
there are more kName than LOUD.

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard.cc (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard.cc:45: // before we've established our first mojo
connection to mus.
On 2016/06/04 06:14:00, dcheng wrote:
> I'm a little worried about potential side effects of swapping out the
Clipboard
> implementation… there's no way to change things so we can order things more
> sanely here?

This was entirely my fault. Fixed.

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard.cc:46: delete it->second;
On 2016/06/04 06:14:00, dcheng wrote:
> We may as well fix the clipboard map to store unique_ptrs.

Done.

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard.h (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard.h:149: // can't use unique_ptr since we don't want
to expose the destructor.)
On 2016/06/04 06:14:00, dcheng wrote:
> Is the last part of this comment still accurate?

Done.

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_test_template.h (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_test_template.h:372: if (!this->IsMusTest()) {
On 2016/06/04 06:14:00, dcheng wrote:
> Why do we exclude mus here? Wouldn't we expect this to work on Linux, even
with
> mus?

Because mus is chromeos in a linux build.

When chrome mus is enabled, any build of chrome can have --mus passed in to
activate ash. And ash doesn't have a selection clipboard.

(The other option is to always write to CLIPBOARD_TYPE_SELECTION in mus which
will usually be ignored. I think this is better.)

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_mus.h (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.h:28: mojo::String GetMimeTypeFor(const FormatType&
format) const;
On 2016/06/04 06:14:00, dcheng wrote:
> Does this actually need to be exposed in the header? At the very least, it
seems
> like it can be static.

Made it static.

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_mus.h:79: std::unique_ptr<mojo::Map<mojo::String,
mojo::Array<uint8_t>>>
On 2016/06/04 06:14:00, dcheng wrote:
> Why unique_ptr this? Can't we just clear it when we need to write new data in?

Because I was asked upthread to do so. It used to just clear().

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_unittest.cc (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_unittest.cc:47: PlatformClipboardTraits::test_helper;
On 2016/06/04 06:14:00, dcheng wrote:
> I don't think we allow globals of non-POD type.

In chrome code. This is in a unit test.

I also don't see a way to avoid this, unless you're talking about linkage, in
which case I put this in an anonymous namespace.

[dcheng, 2016-06-07 05:39:00]

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
File ui/base/clipboard/clipboard_test_template.h (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/base/clipboard/clip...
ui/base/clipboard/clipboard_test_template.h:372: if (!this->IsMusTest()) {
On 2016/06/06 at 20:10:12, Elliot Glaysher wrote:
> On 2016/06/04 06:14:00, dcheng wrote:
> > Why do we exclude mus here? Wouldn't we expect this to work on Linux, even
with
> > mus?
> 
> Because mus is chromeos in a linux build.
> 
> When chrome mus is enabled, any build of chrome can have --mus passed in to
activate ash. And ash doesn't have a selection clipboard.
> 
> (The other option is to always write to CLIPBOARD_TYPE_SELECTION in mus which
will usually be ignored. I think this is better.)

It feels a bit odd, but I can live with this reasoning.

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_unittest.cc (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_unittest.cc:47: PlatformClipboardTraits::test_helper;
On 2016/06/06 at 20:10:12, Elliot Glaysher wrote:
> On 2016/06/04 06:14:00, dcheng wrote:
> > I don't think we allow globals of non-POD type.
> 
> In chrome code. This is in a unit test.
> 
> I also don't see a way to avoid this, unless you're talking about linkage, in
which case I put this in an anonymous namespace.

I don't think the style guide distinguishes between test code and not-test code:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

[Elliot Glaysher, 2016-06-07 17:16:20]

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
File ui/views/mus/clipboard_unittest.cc (right):

https://codereview.chromium.org/2011833003/diff/360001/ui/views/mus/clipboard...
ui/views/mus/clipboard_unittest.cc:47: PlatformClipboardTraits::test_helper;
On 2016/06/07 05:39:00, dcheng wrote:
> On 2016/06/06 at 20:10:12, Elliot Glaysher wrote:
> > On 2016/06/04 06:14:00, dcheng wrote:
> > > I don't think we allow globals of non-POD type.
> > 
> > In chrome code. This is in a unit test.
> > 
> > I also don't see a way to avoid this, unless you're talking about linkage,
in
> which case I put this in an anonymous namespace.
> 
> I don't think the style guide distinguishes between test code and not-test
code:
> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

Replaced static with a forwarding subclass of ui::Clipboard which also owns the
ScoepdVeiwsTestHelper.

[dcheng, 2016-06-07 17:33:51]

lgtm

(Honestly, I'm fine with just making it a static ScopedViewsTestHelper* and
having Create() allocate and assign and Destroy() will delete and null. This is
fine too though.)

[Elliot Glaysher, 2016-06-07 17:39:55]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-06-07 17:39:55]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2011833003/#ps440001
(title: "Remove static in test by creating forwarding clipboard subclass.")

[commit-bot: I haz the power, 2016-06-07 17:40:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2011833003/440001

[commit-bot: I haz the power, 2016-06-07 18:28:54]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2016-06-07 18:29:58]

Description was changed from

==========
Implement ui::ClipboardMus.

This lets chrome in mash read and write from the mus server's clipboard.

BUG=581460
==========

to

==========
Implement ui::ClipboardMus.

This lets chrome in mash read and write from the mus server's clipboard.

BUG=581460

Committed: https://crrev.com/bbc764eb7a7c1937d26f43f6087604ebc2d048ad
Cr-Commit-Position: refs/heads/master@{#398351}
==========

[commit-bot: I haz the power, 2016-06-07 18:29:59]

Patchset 23 (id:??) landed as
https://crrev.com/bbc764eb7a7c1937d26f43f6087604ebc2d048ad
Cr-Commit-Position: refs/heads/master@{#398351}

[Elliot Glaysher, 2016-06-07 19:21:56]

A revert of this CL (patchset #23 id:440001) has been created in
https://codereview.chromium.org/2047703003/ by erg@chromium.org.

The reason for reverting is: Broke Win x64 (dbg) due to size_t -> int
conversion..