Chrome Cleaner: Remove indirect base::FilePath mojo dependency.

Chrome Cleaner: Remove indirect base::FilePath mojo dependency.

The dependency on mojo.common.mojom.FilePath resolves to base::FilePath
through a [Native] typemap. The Chrome cleaner is an independent binary 
whose version of base::FilePath will diverge from Chrome's base::FilePath,
causing compatibility issues down the line.

This will also allow the Chrome Cleaner project to set 
enable_mojom_typemapping to false.

BUG=

Review-Url: https://codereview.chromium.org/2932703006
Cr-Commit-Position: refs/heads/master@{#479471}
Committed: https://chromium.googlesource.com/chromium/src/+/4d3b5c089c535512ac4c9f4b79f95f7f216454d3

[proberge, 2017-06-09 21:56:45]

Description was changed from

==========
Chrome Cleaner: Remove indirect base::FilePath mojo dependency.

The dependency on mojo.common.mojom.FilePath resolves to base::FilePath
through typemaps. The Chrome cleaner is an independent binary whose
version of base::FilePath will diverge from Chrome's base::FilePath,
causing compatibility issues down the line.

This will also allow our upstream project to set enable_mojom_typemapping
to false.

BUG=
==========

to

==========
Chrome Cleaner: Remove indirect base::FilePath mojo dependency.

The dependency on mojo.common.mojom.FilePath resolves to base::FilePath
through typemaps. The Chrome cleaner is an independent binary whose
version of base::FilePath will diverge from Chrome's base::FilePath,
causing compatibility issues down the line.

This will also allow our project to set enable_mojom_typemapping
to false.

BUG=
==========

[proberge, 2017-06-12 13:44:05]

proberge@chromium.org changed reviewers:
+ csharp@chromium.org, ftirelo@chromium.org

[proberge, 2017-06-12 13:44:40]

Description was changed from

==========
Chrome Cleaner: Remove indirect base::FilePath mojo dependency.

The dependency on mojo.common.mojom.FilePath resolves to base::FilePath
through typemaps. The Chrome cleaner is an independent binary whose
version of base::FilePath will diverge from Chrome's base::FilePath,
causing compatibility issues down the line.

This will also allow our project to set enable_mojom_typemapping
to false.

BUG=
==========

to

==========
Chrome Cleaner: Remove indirect base::FilePath mojo dependency.

The dependency on mojo.common.mojom.FilePath resolves to base::FilePath
through typemaps. The Chrome cleaner is an independent binary whose
version of base::FilePath will diverge from Chrome's base::FilePath,
causing compatibility issues down the line.

This will also allow the Chrome Cleaner project to set 
enable_mojom_typemapping to false.

BUG=
==========

[Fabio Tirelo, 2017-06-12 14:04:49]

ftirelo@google.com changed reviewers:
+ ftirelo@google.com

[Fabio Tirelo, 2017-06-12 14:04:50]

https://codereview.chromium.org/2932703006/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc
(right):

https://codereview.chromium.org/2932703006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc:47:
std::vector<base::FilePath> file_paths;
Instead of creating this vector to later copy all elements to a set, why not
using a set directly here?

https://codereview.chromium.org/2932703006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc:49:
files_to_delete) {
Please move this to inside the conditional in line 54.

https://codereview.chromium.org/2932703006/diff/1/components/chrome_cleaner/p...
File components/chrome_cleaner/public/interfaces/chrome_prompt.mojom (right):

https://codereview.chromium.org/2932703006/diff/1/components/chrome_cleaner/p...
components/chrome_cleaner/public/interfaces/chrome_prompt.mojom:24: // aren't
guaranteed to be valid utf8.
Mojo's string16 are also mapped to base. I think we should describe here as well
that we are only using primitive types to avoid version sheering of base.

[proberge, 2017-06-12 17:49:30]

The unit tests identified a mistake in my vector->file path logic. PTAL.

https://codereview.chromium.org/2932703006/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc
(right):

https://codereview.chromium.org/2932703006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc:47:
std::vector<base::FilePath> file_paths;
On 2017/06/12 14:04:50, Fabio Tirelo wrote:
> Instead of creating this vector to later copy all elements to a set, why not
> using a set directly here?

Done.

https://codereview.chromium.org/2932703006/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc:49:
files_to_delete) {
On 2017/06/12 14:04:50, Fabio Tirelo wrote:
> Please move this to inside the conditional in line 54.

Done.

https://codereview.chromium.org/2932703006/diff/1/components/chrome_cleaner/p...
File components/chrome_cleaner/public/interfaces/chrome_prompt.mojom (right):

https://codereview.chromium.org/2932703006/diff/1/components/chrome_cleaner/p...
components/chrome_cleaner/public/interfaces/chrome_prompt.mojom:24: // aren't
guaranteed to be valid utf8.
On 2017/06/12 14:04:50, Fabio Tirelo wrote:
> Mojo's string16 are also mapped to base. I think we should describe here as
well
> that we are only using primitive types to avoid version sheering of base.

Done.

[Fabio Tirelo, 2017-06-12 18:20:47]

LGTM

https://codereview.chromium.org/2932703006/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc
(right):

https://codereview.chromium.org/2932703006/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:204:
std::vector<chrome_cleaner::mojom::FilePathPtr> files_to_delete;
This only needs to be done if condition in line 214 is true.

https://codereview.chromium.org/2932703006/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc
(right):

https://codereview.chromium.org/2932703006/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc:20: return
base::FilePath(base::string16(string.begin(), string.end()));
It may be worth static asserting that 
  size_of(uint16_t) == size_of(base::FilePath::StringType::value_type)

[csharp, 2017-06-12 19:02:46]

lgtm

[proberge, 2017-06-12 19:02:53]

https://codereview.chromium.org/2932703006/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc
(right):

https://codereview.chromium.org/2932703006/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc:204:
std::vector<chrome_cleaner::mojom::FilePathPtr> files_to_delete;
On 2017/06/12 18:20:46, Fabio Tirelo wrote:
> This only needs to be done if condition in line 214 is true.

Line 214 looks like a pointer deref? Not a condition.

https://codereview.chromium.org/2932703006/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc
(right):

https://codereview.chromium.org/2932703006/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc:20: return
base::FilePath(base::string16(string.begin(), string.end()));
On 2017/06/12 18:20:46, Fabio Tirelo wrote:
> It may be worth static asserting that 
>   size_of(uint16_t) == size_of(base::FilePath::StringType::value_type)

Done.

[proberge, 2017-06-12 19:09:47]

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

[proberge, 2017-06-12 19:11:05]

++dcheng for chrome_prompt.mojom SECURITY_OWNERS. I would also appreciate if you
could take a look at the uint16_t* wrangling logic for security issues.

[dcheng, 2017-06-12 21:12:51]

The uint16 hijinks look fine, but maybe we can typemap this still so the
serialization logic won't be mixed into other logic?

(It would be even nicer if we could use the mojo string type directly? I see a
string_traits_string16.h which leads me to believe it might Just Work for struct
traits--though it'd be a bit more complex for the other side)

[proberge, 2017-06-12 21:40:08]

On 2017/06/12 21:12:51, dcheng wrote:
> The uint16 hijinks look fine, but maybe we can typemap this still so the
> serialization logic won't be mixed into other logic?
> 
> (It would be even nicer if we could use the mojo string type directly? I see a
> string_traits_string16.h which leads me to believe it might Just Work for
struct
> traits--though it'd be a bit more complex for the other side)

There's two reasons we think we should avoid using typemaps for this interface.

1. The current way typemaps work require depending on more parts of Chrome than
otherwise necessary. For example, the Chrome Cleaner project currently has to
pull
in ash, components, content, device, gpu, media, net, services, skia and ui.
The typemaps are currently an "all or nothing" build argument: we have no way of
specifying only the typemaps we care about.
(https://cs.chromium.org/chromium/src/mojo/public/tools/bindings/mojom.gni?q=e...)

2. There doesn't seem to be any guarantees that typemapped objects are backwards
compatible. For example, if a Chrome Cleaner executable was built with M57's 
version of base::FilePath, we're worried that it could fail to properly
communicate
with M65 Chrome. 
As a more concrete example, there's been some changes to serialization logic for
base::string16. (Diff revision 3 and revision 4 of 
https://cs.chromium.org/chromium/src/out/Debug/gen/mojo/common/string16.mojom.h)
Is the wire format the same between the two? Will it always be consistent?

[dcheng, 2017-06-13 07:01:52]

On 2017/06/12 21:40:08, proberge wrote:
> On 2017/06/12 21:12:51, dcheng wrote:
> > The uint16 hijinks look fine, but maybe we can typemap this still so the
> > serialization logic won't be mixed into other logic?
> > 
> > (It would be even nicer if we could use the mojo string type directly? I see
a
> > string_traits_string16.h which leads me to believe it might Just Work for
> struct
> > traits--though it'd be a bit more complex for the other side)
> 
> There's two reasons we think we should avoid using typemaps for this
interface.
> 
> 1. The current way typemaps work require depending on more parts of Chrome
than
> otherwise necessary. For example, the Chrome Cleaner project currently has to
> pull
> in ash, components, content, device, gpu, media, net, services, skia and ui.
> The typemaps are currently an "all or nothing" build argument: we have no way
of
> specifying only the typemaps we care about.
>
(https://cs.chromium.org/chromium/src/mojo/public/tools/bindings/mojom.gni?q=e...)

Does Chrome cleaner need to build independently of Chrome? Maybe we could just
use the typemaps while we're in Chrome, and the other end can use raw mojom?

> 
> 2. There doesn't seem to be any guarantees that typemapped objects are
backwards
> compatible. For example, if a Chrome Cleaner executable was built with M57's 
> version of base::FilePath, we're worried that it could fail to properly
> communicate
> with M65 Chrome. 
> As a more concrete example, there's been some changes to serialization logic
for
> base::string16. (Diff revision 3 and revision 4 of 
>
https://cs.chromium.org/chromium/src/out/Debug/gen/mojo/common/string16.mojom.h)
> Is the wire format the same between the two? Will it always be consistent?

The wire serialization is up to you, especially if you just use an array of
uint16.

[proberge, 2017-06-13 21:12:11]

> Does Chrome cleaner need to build independently of Chrome? Maybe we could just
> use the typemaps while we're in Chrome, and the other end can use raw mojom?

Yes, the Chrome Cleaner needs to be built independently of Chrome. We don't want
to be tied to the Chrome release cycle.
Re: Typemaps while in Chrome: ,akes sense. Added a typemap file and associated 
struct_traits, PTAL.

> The wire serialization is up to you, especially if you just use an array of
> uint16.

I think you're right; as long as we explicitly define the struct instead of
using
the [Native] attribute, the wire format should stay the same between versions.

[proberge, 2017-06-13 21:13:10]

@csharp, ftirelo
Big changes, PTAL and explicitly re-LGTM.

[proberge, 2017-06-13 21:37:45]

Description was changed from

==========
Chrome Cleaner: Remove indirect base::FilePath mojo dependency.

The dependency on mojo.common.mojom.FilePath resolves to base::FilePath
through typemaps. The Chrome cleaner is an independent binary whose
version of base::FilePath will diverge from Chrome's base::FilePath,
causing compatibility issues down the line.

This will also allow the Chrome Cleaner project to set 
enable_mojom_typemapping to false.

BUG=
==========

to

==========
Chrome Cleaner: Remove indirect base::FilePath mojo dependency.

The dependency on mojo.common.mojom.FilePath resolves to base::FilePath
through a [Native] typemap. The Chrome cleaner is an independent binary 
whose version of base::FilePath will diverge from Chrome's base::FilePath,
causing compatibility issues down the line.

This will also allow the Chrome Cleaner project to set 
enable_mojom_typemapping to false.

BUG=
==========

[ftirelo, 2017-06-13 23:29:29]

LGTM with a nit.

https://codereview.chromium.org/2932703006/diff/80001/components/chrome_clean...
File components/chrome_cleaner/public/typemaps/chrome_prompt_struct_traits.cc
(right):

https://codereview.chromium.org/2932703006/diff/80001/components/chrome_clean...
components/chrome_cleaner/public/typemaps/chrome_prompt_struct_traits.cc:31:
reinterpret_cast<const base::char16*>(view.data()), view.size()));
Nit: could we use base::FilePath::StringType::value_type instead of base::char16
?

[dcheng, 2017-06-13 23:45:35]

https://codereview.chromium.org/2932703006/diff/80001/components/chrome_clean...
File components/chrome_cleaner/public/typemaps/chrome_prompt_struct_traits.cc
(right):

https://codereview.chromium.org/2932703006/diff/80001/components/chrome_clean...
components/chrome_cleaner/public/typemaps/chrome_prompt_struct_traits.cc:14:
return std::vector<uint16_t>(file_path.value().begin(),
I have bad news... this gets called twice on serialization =/

(there's an open bug to consider fixing this, but it's gotten no traction...)

Two alternatives:
1. Have this return a CArray / ConstCArray which avoids copying the memory.
2. Use SetUpContext/TearDownContext() to create the vector once per
serialization, and return a reference to it here.

I'd probably go with (1) here.

[proberge, 2017-06-14 14:13:56]

proberge@chromium.org changed reviewers:
+ blundell@chromium.org

[proberge, 2017-06-14 14:13:58]

Thanks for the review!

++blundell for components/typemaps.gni approval

https://codereview.chromium.org/2932703006/diff/80001/components/chrome_clean...
File components/chrome_cleaner/public/typemaps/chrome_prompt_struct_traits.cc
(right):

https://codereview.chromium.org/2932703006/diff/80001/components/chrome_clean...
components/chrome_cleaner/public/typemaps/chrome_prompt_struct_traits.cc:14:
return std::vector<uint16_t>(file_path.value().begin(),
On 2017/06/13 23:45:35, dcheng wrote:
> I have bad news... this gets called twice on serialization =/
> 
> (there's an open bug to consider fixing this, but it's gotten no traction...)
> 
> Two alternatives:
> 1. Have this return a CArray / ConstCArray which avoids copying the memory.
> 2. Use SetUpContext/TearDownContext() to create the vector once per
> serialization, and return a reference to it here.
> 
> I'd probably go with (1) here.

Done.

https://codereview.chromium.org/2932703006/diff/80001/components/chrome_clean...
components/chrome_cleaner/public/typemaps/chrome_prompt_struct_traits.cc:31:
reinterpret_cast<const base::char16*>(view.data()), view.size()));
On 2017/06/13 23:29:29, ftirelo wrote:
> Nit: could we use base::FilePath::StringType::value_type instead of
base::char16
> ?

Maybe. Is this so we can remove the defined(OS_WIN)? I don't think we'll ever
need to implement this method for non-Windows OSes.

[csharp, 2017-06-14 14:24:14]

lgtm

[dcheng, 2017-06-14 17:44:15]

ipc lgtm. thanks!

[blundell, 2017-06-14 18:05:27]

lgtm

[proberge, 2017-06-14 18:08:24]

The CQ bit was checked by proberge@chromium.org

[proberge, 2017-06-14 18:08:24]

The patchset sent to the CQ was uploaded after l-g-t-m from ftirelo@google.com,
ftirelo@chromium.org
Link to the patchset: https://codereview.chromium.org/2932703006/#ps100001
(title: "Address review comments on #5")

[commit-bot: I haz the power, 2017-06-14 18:08:45]

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, 2017-06-14 19:44:14]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497463704235220,
"parent_rev": "f3435db20a758c7560614c4dbe367dcec6f1a7e7", "commit_rev":
"5bd58a9d60b174196b0c61c1a8637bc290c48712"}

[commit-bot: I haz the power, 2017-06-14 19:44:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497463704235220,
"parent_rev": "7a5c7e067f550762c21fc76056fd3254f1c2ae8e", "commit_rev":
"4d3b5c089c535512ac4c9f4b79f95f7f216454d3"}

[commit-bot: I haz the power, 2017-06-14 19:44:55]

Description was changed from

==========
Chrome Cleaner: Remove indirect base::FilePath mojo dependency.

The dependency on mojo.common.mojom.FilePath resolves to base::FilePath
through a [Native] typemap. The Chrome cleaner is an independent binary 
whose version of base::FilePath will diverge from Chrome's base::FilePath,
causing compatibility issues down the line.

This will also allow the Chrome Cleaner project to set 
enable_mojom_typemapping to false.

BUG=
==========

to

==========
Chrome Cleaner: Remove indirect base::FilePath mojo dependency.

The dependency on mojo.common.mojom.FilePath resolves to base::FilePath
through a [Native] typemap. The Chrome cleaner is an independent binary 
whose version of base::FilePath will diverge from Chrome's base::FilePath,
causing compatibility issues down the line.

This will also allow the Chrome Cleaner project to set 
enable_mojom_typemapping to false.

BUG=

Review-Url: https://codereview.chromium.org/2932703006
Cr-Commit-Position: refs/heads/master@{#479471}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d3b5c089c535512ac4c9f4b79f9...
==========

[commit-bot: I haz the power, 2017-06-14 19:44:57]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4d3b5c089c535512ac4c9f4b79f9...