ImageCapture: replace Mojo String/Array with stl/wtf string/vector

ImageCapture: replace Mojo String/Array with stl/wtf string/vector

This CL nukes the usage of mojo::String and mojo::Array<> after [1],
so where it was mojo::String it now reads std::string or WTF::String
and also mojo::Array<> is replaced with std::vector or WTF::Vector,
as appropriate.

[1] https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer#!msg/chromium-mojo/2SHvKyahLGg/sU61P0IIBgAJ

BUG=624136, 518807
TBR=xhwang@chromium.org
for the trivial change in media/mojo/interfaces/BUILD.gn

Committed: https://crrev.com/53d04735fed4c229ba5757ed38f4150dd4c15d7f
Cr-Commit-Position: refs/heads/master@{#406776}

[mcasas, 2016-07-20 03:27:33]

Description was changed from

==========
ImageCapture: switch to new Mojo types (stl/wtf string/vector)

This CL nukes the usage of mojo::String and mojo::Array<> after [1].

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=624136, 518807
==========

to

==========
ImageCapture: replace Mojo String/Array with stl/wtf string/vector

This CL nukes the usage of mojo::String and mojo::Array<> after [1].

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=624136, 518807
==========

[mcasas, 2016-07-20 03:27:46]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-07-20 16:19:16]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-07-20 17:37:14]

Description was changed from

==========
ImageCapture: replace Mojo String/Array with stl/wtf string/vector

This CL nukes the usage of mojo::String and mojo::Array<> after [1].

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=624136, 518807
==========

to

==========
ImageCapture: replace Mojo String/Array with stl/wtf string/vector

This CL nukes the usage of mojo::String and mojo::Array<> after [1],
so where it was mojo::String it now reads std::string or WTF::String
and also mojo::Array<> is replaced with std::vector or WTF::Vector,
as appropriate.

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=624136, 518807
==========

[mcasas, 2016-07-20 17:38:26]

mcasas@chromium.org changed reviewers:
+ yzshen@chromium.org

[mcasas, 2016-07-20 17:38:27]

yzshen@ PTAL

[yzshen1, 2016-07-20 17:40:53]

LGTM

Yay!

[mcasas, 2016-07-20 17:43:32]

mcasas@chromium.org changed reviewers:
+ esprehn@chromium.org, wez@chromium.org

[mcasas, 2016-07-20 17:43:33]

wez@: Owners RS content/browser/media/capture plz

esprehn@ Owners RS third_party/WebKit/Source/modules/imagecapture

[haraken, 2016-07-20 17:43:56]

On 2016/07/20 17:43:33, mcasas wrote:
> wez@: Owners RS content/browser/media/capture plz
> 
> esprehn@ Owners RS third_party/WebKit/Source/modules/imagecapture

Elliott is OOO. modules/ LGTM.

[Wez, 2016-07-20 18:52:07]

https://codereview.chromium.org/2166713002/diff/40001/content/browser/media/c...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/2166713002/diff/40001/content/browser/media/c...
content/browser/media/capture/image_capture_impl.cc:57: base::Bind(callback,
mime_type, data));
|data| was previously move()'d, which was presumably just a pointer-swap, but
must now be copied - can we arrange to keep the move semantics?

https://codereview.chromium.org/2166713002/diff/40001/media/capture/video/fak...
File media/capture/video/fake_video_capture_device.cc (right):

https://codereview.chromium.org/2166713002/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:111:
callback.Run(std::string("image/png"), encoded_data);
nit: Do you need the explicit std::string here?

[mcasas, 2016-07-20 22:57:02]

https://codereview.chromium.org/2166713002/diff/40001/content/browser/media/c...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/2166713002/diff/40001/content/browser/media/c...
content/browser/media/capture/image_capture_impl.cc:57: base::Bind(callback,
mime_type, data));
On 2016/07/20 18:52:07, Wez wrote:
> |data| was previously move()'d, which was presumably just a pointer-swap, but
> must now be copied - can we arrange to keep the move semantics?

It's complicated due to the automatic generation
of the TakePhotoCallback, that includes const& for
|data|, but not impossible, so, done.

An alternative is to have a temporary
std::vector<uint8_t> data_copy = std::move(data);
but even then, due to |data| being const&, the
copy constructor would be used instead of the
move constructor, so one way or another, 
const_cast is the only way I saw.

https://codereview.chromium.org/2166713002/diff/40001/media/capture/video/fak...
File media/capture/video/fake_video_capture_device.cc (right):

https://codereview.chromium.org/2166713002/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:111:
callback.Run(std::string("image/png"), encoded_data);
On 2016/07/20 18:52:07, Wez wrote:
> nit: Do you need the explicit std::string here?

Nope, removed.
It wouldn't hurt performance because a temporary
constructed like this would not be re-wrapped into 
another std:string, correct? So, removed for the sake
of less verbosity.

[yzshen1, 2016-07-20 23:05:13]

https://codereview.chromium.org/2166713002/diff/40001/content/browser/media/c...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/2166713002/diff/40001/content/browser/media/c...
content/browser/media/capture/image_capture_impl.cc:57: base::Bind(callback,
mime_type, data));
On 2016/07/20 22:57:01, mcasas wrote:
> On 2016/07/20 18:52:07, Wez wrote:
> > |data| was previously move()'d, which was presumably just a pointer-swap,
but
> > must now be copied - can we arrange to keep the move semantics?
> 
> It's complicated due to the automatic generation
> of the TakePhotoCallback, that includes const& for
> |data|, but not impossible, so, done.
> 
> An alternative is to have a temporary
> std::vector<uint8_t> data_copy = std::move(data);
> but even then, due to |data| being const&, the
> copy constructor would be used instead of the
> move constructor, so one way or another, 
> const_cast is the only way I saw.

I don't know whether it is necessary, but if you want here is one way to do it:

(1) define a mojom struct
// in mojom:
struct SomeData {
  array<uint8> data;
};

(2) create a custom typemapping between SomeData and
std::unique_ptr<std::vector<uint8_t>>, in the typemap config annotate that this
type is move-only:

type_mappings = [ "SomeData=std::unique_ptr<std::vector<uint8_t>>[move_only]" ]

[Wez, 2016-07-20 23:19:55]

Basically I defer to Yuzhu's Mojo expertise!

https://codereview.chromium.org/2166713002/diff/40001/content/browser/media/c...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/2166713002/diff/40001/content/browser/media/c...
content/browser/media/capture/image_capture_impl.cc:57: base::Bind(callback,
mime_type, data));
I mentioned this because |data| seems like it's some sort of bulky image data,
and the old mojo::Array form was move-only, with [potentially] no copying
end-to-end - I'd be wary of introducing an extra copy for any form of "bulk"
data.

[yzshen1, 2016-07-20 23:27:36]

https://codereview.chromium.org/2166713002/diff/40001/content/browser/media/c...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/2166713002/diff/40001/content/browser/media/c...
content/browser/media/capture/image_capture_impl.cc:57: base::Bind(callback,
mime_type, data));
On 2016/07/20 23:19:54, Wez wrote:
> I mentioned this because |data| seems like it's some sort of bulky image data,
> and the old mojo::Array form was move-only, with [potentially] no copying
> end-to-end - I'd be wary of introducing an extra copy for any form of "bulk"
> data.

Agreed. Copy seems sad here. mcasas@ mentioned that he could use the const_cast
as a temp solution and add a TODO to address using typemapping separately. I am
personally fine with that.

[mcasas, 2016-07-20 23:46:06]

Done adding a TODO()

[Wez, 2016-07-21 00:10:46]

LGTM (is there a looks-horrible-to-me-but-OK-I-suppose?)

https://codereview.chromium.org/2166713002/diff/70001/content/browser/media/c...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/2166713002/diff/70001/content/browser/media/c...
content/browser/media/capture/image_capture_impl.cc:61:
base::Passed(const_cast<std::vector<uint8_t>*>(&data))));
nit: I take it it's not possible to have this work with std::move in some way?

[mcasas, 2016-07-21 00:25:38]

Description was changed from

==========
ImageCapture: replace Mojo String/Array with stl/wtf string/vector

This CL nukes the usage of mojo::String and mojo::Array<> after [1],
so where it was mojo::String it now reads std::string or WTF::String
and also mojo::Array<> is replaced with std::vector or WTF::Vector,
as appropriate.

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=624136, 518807
==========

to

==========
ImageCapture: replace Mojo String/Array with stl/wtf string/vector

This CL nukes the usage of mojo::String and mojo::Array<> after [1],
so where it was mojo::String it now reads std::string or WTF::String
and also mojo::Array<> is replaced with std::vector or WTF::Vector,
as appropriate.

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=624136, 518807

TBR=xhwang@chromium.org 
for the trivial change in media/mojo/interfaces/BUILD.gn
==========

[mcasas, 2016-07-21 00:25:44]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-07-21 00:25:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2166713002/#ps70001
(title: "Added TODO to use mojo typemappings, https://crbug.com/630040")

[mcasas, 2016-07-21 00:25:58]

https://codereview.chromium.org/2166713002/diff/70001/content/browser/media/c...
File content/browser/media/capture/image_capture_impl.cc (right):

https://codereview.chromium.org/2166713002/diff/70001/content/browser/media/c...
content/browser/media/capture/image_capture_impl.cc:61:
base::Passed(const_cast<std::vector<uint8_t>*>(&data))));
On 2016/07/21 00:10:46, Wez wrote:
> nit: I take it it's not possible to have this work with std::move in some way?

It's too complicated. TBH let's commit this first,
knowing that I'll be working imminently on its
sequel (fret not, you'll review it) and that all 
ImageCapture related code is behind a flag
(and the bug for introducing TypeMapping is
blocking the larger ImageCapture one).

You could siphon off the memory inside |data| 
into a newly allocated std::unique_ptr<> that 
is then base::Passed(std::move()) into the 
base::Callback, but, again, not worth it.

[commit-bot: I haz the power, 2016-07-21 00:26:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 00:49:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 00:49:21]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mcasas, 2016-07-21 00:52:45]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-07-21 00:53:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 01:09:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 01:10:02]

Try jobs failed on following builders:
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)

[mcasas, 2016-07-21 01:26:52]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 01:27:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 02:03:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 02:03:27]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mcasas, 2016-07-21 03:14:15]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-07-21 03:14:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yzshen@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2166713002/#ps90001
(title: "forgot media/.../mojo_bindings.gyp")

[commit-bot: I haz the power, 2016-07-21 03:14:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 04:14:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 04:14:42]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mcasas, 2016-07-21 04:16:24]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-07-21 04:16:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 06:01:15]

Description was changed from

==========
ImageCapture: replace Mojo String/Array with stl/wtf string/vector

This CL nukes the usage of mojo::String and mojo::Array<> after [1],
so where it was mojo::String it now reads std::string or WTF::String
and also mojo::Array<> is replaced with std::vector or WTF::Vector,
as appropriate.

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=624136, 518807

TBR=xhwang@chromium.org 
for the trivial change in media/mojo/interfaces/BUILD.gn
==========

to

==========
ImageCapture: replace Mojo String/Array with stl/wtf string/vector

This CL nukes the usage of mojo::String and mojo::Array<> after [1],
so where it was mojo::String it now reads std::string or WTF::String
and also mojo::Array<> is replaced with std::vector or WTF::Vector,
as appropriate.

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=624136, 518807

TBR=xhwang@chromium.org 
for the trivial change in media/mojo/interfaces/BUILD.gn
==========

[commit-bot: I haz the power, 2016-07-21 06:01:17]

Committed patchset #4 (id:90001)

[commit-bot: I haz the power, 2016-07-21 06:03:06]

Description was changed from

==========
ImageCapture: replace Mojo String/Array with stl/wtf string/vector

This CL nukes the usage of mojo::String and mojo::Array<> after [1],
so where it was mojo::String it now reads std::string or WTF::String
and also mojo::Array<> is replaced with std::vector or WTF::Vector,
as appropriate.

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=624136, 518807

TBR=xhwang@chromium.org 
for the trivial change in media/mojo/interfaces/BUILD.gn
==========

to

==========
ImageCapture: replace Mojo String/Array with stl/wtf string/vector

This CL nukes the usage of mojo::String and mojo::Array<> after [1],
so where it was mojo::String it now reads std::string or WTF::String
and also mojo::Array<> is replaced with std::vector or WTF::Vector,
as appropriate.

[1]
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

BUG=624136, 518807

TBR=xhwang@chromium.org
for the trivial change in media/mojo/interfaces/BUILD.gn

Committed: https://crrev.com/53d04735fed4c229ba5757ed38f4150dd4c15d7f
Cr-Commit-Position: refs/heads/master@{#406776}
==========

[commit-bot: I haz the power, 2016-07-21 06:03:08]

Patchset 4 (id:??) landed as
https://crrev.com/53d04735fed4c229ba5757ed38f4150dd4c15d7f
Cr-Commit-Position: refs/heads/master@{#406776}