Convert SetContentSettingRules to use mojo, part 1/2.

Convert SetContentSettingRules to use mojo, part 1/2.

This converts 1 of the 2 existing ChromeViewMsg_SetContentSettingRules
uses. Converting the other will be a separate commit.

BUG=577685
Review-Url: https://codereview.chromium.org/2582203003
Cr-Commit-Position: refs/heads/master@{#441862}
Committed: https://chromium.googlesource.com/chromium/src/+/20e3d92371109d1bf4e2f26040bbaac964794da1

[nigeltao1, 2016-12-18 03:31:11]

nigeltao@chromium.org changed reviewers:
+ sammc@chromium.org

[nigeltao1, 2016-12-18 03:31:12]

I'm not sure about a few things, marked with XXX, that I'd like some advice on,
and doing it by code review seems as good a place as any. The XXX comments
aren't for submitting.

I'm also not 100% sure that I've got all the memory ownership / lifetime issues
correct with typemaps, so please take a close look.

[nigeltao1, 2016-12-18 03:31:21]

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

[commit-bot: I haz the power, 2016-12-18 03:31:27]

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-12-18 03:35:17]

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

[commit-bot: I haz the power, 2016-12-18 03:35:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[nigeltao1, 2016-12-18 03:57:19]

Trybots are failing, e.g.
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
says:

----
ERROR at //components/content_settings/core/common/content_settings.h:13:11:
Include not allowed.
#include "components/content_settings/core/common/content_settings_pattern.h"
          ^-----------------------------------------------------------------
It is not in any dependency of
  //chrome/common:mojo_bindings_cpp_sources
The include file is in the target(s):
  //components/content_settings/core/common:common
which should somehow be reachable.
GN gen failed: 1
----

So I tried

----
$ git diff
diff --git a/chrome/common/BUILD.gn b/chrome/common/BUILD.gn
index 95e7857..e8d7504 100644
--- a/chrome/common/BUILD.gn
+++ b/chrome/common/BUILD.gn
@@ -684,6 +684,7 @@ mojom("mojo_bindings") {
   ]
 
   public_deps = [
+    "//components/content_settings/core/common",
     "//url/mojo:url_mojom_gurl",
   ]
 }
----

but that doesn't work:

----
$ ninja -j 1000 -C out/def chrome
ninja: Entering directory `out/def'
[1/1] Regenerating ninja files
FAILED: build.ninja 
/usr/local/google/home/nigeltao/chromium/src/buildtools/linux64/gn
--root=/usr/local/google/home/nigeltao/chromium/src -q gen .
ERROR Unresolved dependencies.
//chrome/common:mojo_bindings__check_deps_are_all_mojom(//build/toolchain/linux:clang_x64)
  needs
//components/content_settings/core/common:common__is_mojom(//build/toolchain/linux:clang_x64)
//chrome/common:mojo_bindings__type_mappings(//build/toolchain/linux:clang_x64)
  needs
//components/content_settings/core/common:common__type_mappings(//build/toolchain/linux:clang_x64)
//chrome/common:mojo_bindings_blink(//build/toolchain/linux:clang_x64)
  needs
//components/content_settings/core/common:common_blink(//build/toolchain/linux:clang_x64)
//chrome/common:mojo_bindings_blink__type_mappings(//build/toolchain/linux:clang_x64)
  needs
//components/content_settings/core/common:common_blink__type_mappings(//build/toolchain/linux:clang_x64)
//chrome/common:mojo_bindings_blink_cpp_sources(//build/toolchain/linux:clang_x64)
  needs
//components/content_settings/core/common:common_blink_cpp_sources(//build/toolchain/linux:clang_x64)
//chrome/common:mojo_bindings_cpp_sources(//build/toolchain/linux:clang_x64)
  needs
//components/content_settings/core/common:common_cpp_sources(//build/toolchain/linux:clang_x64)
//chrome/common:mojo_bindings_shared_cpp_sources(//build/toolchain/linux:clang_x64)
  needs
//components/content_settings/core/common:common_shared_cpp_sources(//build/toolchain/linux:clang_x64)

ninja: error: rebuilding 'build.ninja': subcommand failed
----

Any suggestions?

[Sam McNally, 2016-12-19 00:17:51]

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
File chrome/common/renderer_configuration.mojom (right):

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration.mojom:8: //
components/content_settings/core/common/content_settings.h
I think these structs/enums should live in
components/content_settings/core/common/content_settings.mojom. Similarly with
the typemap and traits.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration.mojom:10: CONTENT_SETTING_DEFAULT = 0,
Mojo enums generate scoped enums in C++ so you don't need to put the
CONTENT_SETTING_ prefix in the values.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration.mojom:28: string primary_pattern;
I think a struct would be preferred.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
File chrome/common/renderer_configuration.typemap (right):

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration.typemap:19: ]
Add public_deps = [ "//components/content_settings/core/common" ]

Mojoms can only depend on other mojoms. Typemaps are where all the extra
information for custom C++ types go.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
File chrome/common/renderer_configuration_struct_traits.cc (right):

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.cc:28: // XXX: Should we
really catch the "NUM_SETTINGS" dummy value that defines
I think I would leave it out of the mojom enum entirely.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.cc:33: default:
Put the catchall outside the switch instead of using default. That way the
compiler will enforce that new values are handled.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.cc:38: // malicious
renderers? Ditto below.
Yes. This NOTREACHED would be hit in the sender of invalid values.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.cc:72: NOTREACHED();
Again, move this out of the switch. I think it makes sense to remove this
NOTREACHED(); receiving invalid messages should just fail validation by
returning false.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.cc:81: const std::string
This is inefficient. This accessor is called twice. If you do need to construct
a string, you'll want to use a context to store the temporary string.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
File chrome/common/renderer_configuration_struct_traits.h (right):

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.h:28: static const
std::string primary_pattern(
Inline simple accessors.

https://codereview.chromium.org/2582203003/diff/1/components/content_settings...
File components/content_settings/core/common/content_settings.h (right):

https://codereview.chromium.org/2582203003/diff/1/components/content_settings...
components/content_settings/core/common/content_settings.h:20: // type is not
automatically generated or updated.
Add a TODO to migrate the Java users of this enum to the mojom-generated enum.

[nigeltao1, 2016-12-22 00:31:23]

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

[commit-bot: I haz the power, 2016-12-22 00:31:53]

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-12-22 01:56:20]

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

[commit-bot: I haz the power, 2016-12-22 01:56:21]

Dry run: This issue passed the CQ dry run.

[nigeltao1, 2016-12-22 04:09:46]

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
File chrome/common/renderer_configuration.mojom (right):

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration.mojom:8: //
components/content_settings/core/common/content_settings.h
On 2016/12/19 00:17:51, Sam McNally wrote:
> I think these structs/enums should live in
> components/content_settings/core/common/content_settings.mojom. Similarly with
> the typemap and traits.

Done.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration.mojom:10: CONTENT_SETTING_DEFAULT = 0,
On 2016/12/19 00:17:51, Sam McNally wrote:
> Mojo enums generate scoped enums in C++ so you don't need to put the
> CONTENT_SETTING_ prefix in the values.

Done.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration.mojom:28: string primary_pattern;
On 2016/12/19 00:17:51, Sam McNally wrote:
> I think a struct would be preferred.

Done.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
File chrome/common/renderer_configuration.typemap (right):

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration.typemap:19: ]
On 2016/12/19 00:17:51, Sam McNally wrote:
> Add public_deps = [ "//components/content_settings/core/common" ]
> 
> Mojoms can only depend on other mojoms. Typemaps are where all the extra
> information for custom C++ types go.

Done.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
File chrome/common/renderer_configuration_struct_traits.cc (right):

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.cc:28: // XXX: Should we
really catch the "NUM_SETTINGS" dummy value that defines
On 2016/12/19 00:17:51, Sam McNally wrote:
> I think I would leave it out of the mojom enum entirely.

Done.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.cc:33: default:
On 2016/12/19 00:17:51, Sam McNally wrote:
> Put the catchall outside the switch instead of using default. That way the
> compiler will enforce that new values are handled.

Done.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.cc:38: // malicious
renderers? Ditto below.
On 2016/12/19 00:17:51, Sam McNally wrote:
> Yes. This NOTREACHED would be hit in the sender of invalid values.

Acknowledged.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.cc:72: NOTREACHED();
On 2016/12/19 00:17:51, Sam McNally wrote:
> Again, move this out of the switch. I think it makes sense to remove this
> NOTREACHED(); receiving invalid messages should just fail validation by
> returning false.

Done.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.cc:81: const std::string
On 2016/12/19 00:17:51, Sam McNally wrote:
> This is inefficient. This accessor is called twice. If you do need to
construct
> a string, you'll want to use a context to store the temporary string.

No longer applicable, as I serialize the underlying data structure, not a
string.

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
File chrome/common/renderer_configuration_struct_traits.h (right):

https://codereview.chromium.org/2582203003/diff/1/chrome/common/renderer_conf...
chrome/common/renderer_configuration_struct_traits.h:28: static const
std::string primary_pattern(
On 2016/12/19 00:17:51, Sam McNally wrote:
> Inline simple accessors.

Done.

https://codereview.chromium.org/2582203003/diff/1/components/content_settings...
File components/content_settings/core/common/content_settings.h (right):

https://codereview.chromium.org/2582203003/diff/1/components/content_settings...
components/content_settings/core/common/content_settings.h:20: // type is not
automatically generated or updated.
On 2016/12/19 00:17:51, Sam McNally wrote:
> Add a TODO to migrate the Java users of this enum to the mojom-generated enum.

Done.

[nigeltao1, 2016-12-22 04:10:25]

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

[commit-bot: I haz the power, 2016-12-22 04:10:40]

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-12-22 04:14:15]

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

[commit-bot: I haz the power, 2016-12-22 04:14:16]

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

[nigeltao1, 2016-12-22 04:29:19]

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

[commit-bot: I haz the power, 2016-12-22 04:29:31]

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-12-22 04:33:13]

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

[commit-bot: I haz the power, 2016-12-22 04:33:14]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2016-12-22 04:46:53]

lgtm

https://codereview.chromium.org/2582203003/diff/60001/chrome/common/renderer_...
File chrome/common/renderer_configuration.mojom (right):

https://codereview.chromium.org/2582203003/diff/60001/chrome/common/renderer_...
chrome/common/renderer_configuration.mojom:16:
SetContentSettingRules(content_settings.mojom.RendererContentSettingRules
rules);
>80 chars.

[nigeltao1, 2016-12-22 05:00:23]

https://codereview.chromium.org/2582203003/diff/60001/chrome/common/renderer_...
File chrome/common/renderer_configuration.mojom (right):

https://codereview.chromium.org/2582203003/diff/60001/chrome/common/renderer_...
chrome/common/renderer_configuration.mojom:16:
SetContentSettingRules(content_settings.mojom.RendererContentSettingRules
rules);
On 2016/12/22 04:46:53, Sam McNally wrote:
> >80 chars.

Done.

[nigeltao1, 2016-12-22 05:01:05]

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

[commit-bot: I haz the power, 2016-12-22 05:01:46]

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-12-22 06:16:15]

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

[commit-bot: I haz the power, 2016-12-22 06:16:16]

Dry run: This issue passed the CQ dry run.

[nigeltao1, 2016-12-22 07:09:47]

nigeltao@chromium.org changed reviewers:
+ sky@chromium.org, tsepez@chromium.org

[nigeltao1, 2016-12-22 07:09:48]

R=sky,tsepez for OWNERS.

I know it's the holidays. There's no rush on the code review.

[Tom Sepez, 2017-01-03 19:16:03]

https://codereview.chromium.org/2582203003/diff/80001/components/content_sett...
File components/content_settings/core/common/content_settings.mojom (right):

https://codereview.chromium.org/2582203003/diff/80001/components/content_sett...
components/content_settings/core/common/content_settings.mojom:9: string scheme;
Are there any assumptions on the receiving end about what characters may appear
in these strings? do we need to sanitize them when reading via traits?

https://codereview.chromium.org/2582203003/diff/80001/components/content_sett...
components/content_settings/core/common/content_settings.mojom:12: bool
has_domain_wildcard;
nit: has_host_wildcard?

[sky, 2017-01-03 22:01:42]

LGTM

[nigeltao1, 2017-01-04 23:53:38]

https://codereview.chromium.org/2582203003/diff/80001/components/content_sett...
File components/content_settings/core/common/content_settings.mojom (right):

https://codereview.chromium.org/2582203003/diff/80001/components/content_sett...
components/content_settings/core/common/content_settings.mojom:9: string scheme;
On 2017/01/03 19:16:02, Tom Sepez wrote:
> Are there any assumptions on the receiving end about what characters may
appear
> in these strings? do we need to sanitize them when reading via traits?

I've copy/pasted the comments from the C++ PatternParts type at
https://cs.chromium.org/chromium/src/components/content_settings/core/common/...

https://codereview.chromium.org/2582203003/diff/80001/components/content_sett...
components/content_settings/core/common/content_settings.mojom:12: bool
has_domain_wildcard;
On 2017/01/03 19:16:02, Tom Sepez wrote:
> nit: has_host_wildcard?

As per those copy/pasted comments, the host might not be a domain, such as an IP
address.

[Tom Sepez, 2017-01-05 00:29:43]

lgtm

[nigeltao1, 2017-01-05 00:40:32]

The CQ bit was checked by nigeltao@chromium.org

[nigeltao1, 2017-01-05 00:40:32]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2582203003/#ps100001
(title: "Convert SetContentSettingRules to use mojo, part 1/2.")

[commit-bot: I haz the power, 2017-01-05 00:41:12]

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-01-05 00:50:39]

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

[commit-bot: I haz the power, 2017-01-05 00:50:40]

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

[nigeltao1, 2017-01-05 01:41:18]

nigeltao@chromium.org changed reviewers:
+ jochen@chromium.org

[nigeltao1, 2017-01-05 01:41:19]

jochen, can you review for components/content_settings/OWNERS?

[jochen (gone - plz use gerrit), 2017-01-05 12:13:27]

lgtm

[nigeltao1, 2017-01-06 00:08:24]

The CQ bit was checked by nigeltao@chromium.org

[commit-bot: I haz the power, 2017-01-06 00:09:06]

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-01-06 03:22:20]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483661304981370,
"parent_rev": "6ab313adb654c1362673f92befa229cb72103373", "commit_rev":
"20e3d92371109d1bf4e2f26040bbaac964794da1"}

[commit-bot: I haz the power, 2017-01-06 03:23:57]

Description was changed from

==========
Convert SetContentSettingRules to use mojo, part 1/2.

This converts 1 of the 2 existing ChromeViewMsg_SetContentSettingRules
uses. Converting the other will be a separate commit.

BUG=577685
==========

to

==========
Convert SetContentSettingRules to use mojo, part 1/2.

This converts 1 of the 2 existing ChromeViewMsg_SetContentSettingRules
uses. Converting the other will be a separate commit.

BUG=577685

Review-Url: https://codereview.chromium.org/2582203003
Cr-Commit-Position: refs/heads/master@{#441862}
Committed:
https://chromium.googlesource.com/chromium/src/+/20e3d92371109d1bf4e2f26040bb...
==========

[commit-bot: I haz the power, 2017-01-06 03:23:59]

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