Merge Content Settings IPCs back to chrome/common/render_messages.h

Merge Content Settings IPCs back to chrome/common/render_messages.h

This reverts https://codereview.chromium.org/798923003 (manually, because the
context code has changed significantly). The separation of content settings
IPCs was done to prepare componentising of
chrome/renderer/content_settings_observer.cc, which was not needed in the end.
As jam@ points out in https://codereview.chromium.org/2375333003/#msg9, it
should be cleaned-up.

BUG=384874, 387075
Committed: https://crrev.com/c5eaaddfa4a70f69796722e6f247ad5ac505818f
Cr-Commit-Position: refs/heads/master@{#423142}

[vabr (Chromium), 2016-09-30 12:53:46]

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

[commit-bot: I haz the power, 2016-09-30 12:53:54]

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-09-30 13:03:24]

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

[commit-bot: I haz the power, 2016-09-30 13:03:25]

Dry run: 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...)

[vabr (Chromium), 2016-09-30 13:17:26]

Description was changed from

==========
Merge Content Settings IPCs back to chrome/common/render_messages.h

This reverts https://codereview.chromium.org/798923003 (manually, for the
context code has changed significantly). The separation of content settings IPCs
was done to prepare componentising of
chrome/renderer/content_settings_observer.cc, which was not needed in the end.
As jam@ points out in https://codereview.chromium.org/2375333003/#msg9, it
should be cleaned-up.

BUG=384874,387075
==========

to

==========
Merge Content Settings IPCs back to chrome/common/render_messages.h

This reverts https://codereview.chromium.org/798923003 (manually, because the
context code has changed significantly). The separation of content settings
IPCs was done to prepare componentising of
chrome/renderer/content_settings_observer.cc, which was not needed in the end.
As jam@ points out in https://codereview.chromium.org/2375333003/#msg9, it
should be cleaned-up.

BUG=384874,387075
==========

[vabr (Chromium), 2016-09-30 13:22:25]

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

[commit-bot: I haz the power, 2016-09-30 13:22:39]

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-09-30 13:32:30]

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

[commit-bot: I haz the power, 2016-09-30 13:32:31]

Dry run: 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...)

[vabr (Chromium), 2016-09-30 16:02:01]

vabr@chromium.org changed reviewers:
+ dcheng@chromium.org, jam@chromium.org, msramek@chromium.org

[vabr (Chromium), 2016-09-30 16:02:02]

Hi all.

msramek@, please review components/content_settings and c/b/content_settings.

dcheng@, please review chrome/common/render_messages.h, ipc/ipc_message_start.h,
tools/ipc_fuzzer/message_lib/all_messages.h and the removal of
components/content_settings/content/common/content_settings_messages.h

jam@, please review the rest.

Thanks!
Vaclav

[msramek, 2016-09-30 16:19:20]

LGTM.

Looks like "core/" is now superfluous since there's no subdivision at that level
anymore. (I'm not suggesting to do anything about it in this CL :) )

[jam, 2016-09-30 16:21:51]

lgtm

[vabr (Chromium), 2016-10-04 08:51:54]

vabr@chromium.org changed reviewers:
+ mkwst@chromium.org

[vabr (Chromium), 2016-10-04 08:51:55]

Hi mkwst@,

Do you think you could review chrome/common/render_messages.h,
ipc/ipc_message_start.h,
tools/ipc_fuzzer/message_lib/all_messages.h and the removal of
components/content_settings/content/common/content_settings_messages.h ?

Cheers,
Vaclav

[Mike West, 2016-10-04 14:57:00]

Merging the IPC messages back into the existing file LGTM.

[vabr (Chromium), 2016-10-04 16:05:05]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-10-04 16:05:26]

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-10-04 16:16:50]

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

[commit-bot: I haz the power, 2016-10-04 16:16:51]

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...)

[vabr (Chromium), 2016-10-04 16:32:00]

Thanks, Mike, for review!

The presubmit still fails with somewhat confusing message:

** Presubmit ERRORS **
Found changes to IPC files without a security OWNER!

***************
chrome/common/OWNERS is missing the following lines:

per-file *_param_traits*.*=set noparent
per-file *_param_traits*.*=file://ipc/SECURITY_OWNERS

for changed files:
  chrome/common/render_messages.h
  chrome/common/common_param_traits_macros.h
***************

I'll try to understand and fix it tomorrow.

Cheers,
Vaclav

[dcheng, 2016-10-04 16:41:07]

On 2016/10/04 16:32:00, vabr (Chromium) wrote:
> Thanks, Mike, for review!
> 
> The presubmit still fails with somewhat confusing message:
> 
> ** Presubmit ERRORS **
> Found changes to IPC files without a security OWNER!
> 
> ***************
> chrome/common/OWNERS is missing the following lines:
> 
> per-file *_param_traits*.*=set noparent
> per-file *_param_traits*.*=file://ipc/SECURITY_OWNERS
> 
> for changed files:
>   chrome/common/render_messages.h
>   chrome/common/common_param_traits_macros.h
> ***************
> 
> I'll try to understand and fix it tomorrow.
> 
> Cheers,
> Vaclav

It means that you'll need to add those two per-files lines to
//chrome/common/OWNERS. Suggestions on how to improve the message are welcome,
sorry for the confusion.

[vabr (Chromium), 2016-10-04 18:19:32]

On 2016/10/04 16:41:07, dcheng wrote:
> On 2016/10/04 16:32:00, vabr (Chromium) wrote:
> > Thanks, Mike, for review!
> > 
> > The presubmit still fails with somewhat confusing message:
> > 
> > ** Presubmit ERRORS **
> > Found changes to IPC files without a security OWNER!
> > 
> > ***************
> > chrome/common/OWNERS is missing the following lines:
> > 
> > per-file *_param_traits*.*=set noparent
> > per-file *_param_traits*.*=file://ipc/SECURITY_OWNERS
> > 
> > for changed files:
> >   chrome/common/render_messages.h
> >   chrome/common/common_param_traits_macros.h
> > ***************
> > 
> > I'll try to understand and fix it tomorrow.
> > 
> > Cheers,
> > Vaclav
> 
> It means that you'll need to add those two per-files lines to
> //chrome/common/OWNERS. Suggestions on how to improve the message are welcome,
> sorry for the confusion.

Thanks for the explanation! I guess I was confused by the first sentence: "Found
changes to IPC files without a security OWNER," because my brain automatically
added "review" to the end of that sentence. Now reading properly, it all makes
sense, the issue was completely on my side.

So I will add the lines suggested by the script and land the CL tomorrow, hoping
that it is OK not to wait for re-review for that.

Cheers,
Vaclav

[vabr (Chromium), 2016-10-05 12:18:54]

On 2016/10/04 18:19:32, vabr (Chromium) wrote:
> On 2016/10/04 16:41:07, dcheng wrote:
> > On 2016/10/04 16:32:00, vabr (Chromium) wrote:
> > > Thanks, Mike, for review!
> > > 
> > > The presubmit still fails with somewhat confusing message:
> > > 
> > > ** Presubmit ERRORS **
> > > Found changes to IPC files without a security OWNER!
> > > 
> > > ***************
> > > chrome/common/OWNERS is missing the following lines:
> > > 
> > > per-file *_param_traits*.*=set noparent
> > > per-file *_param_traits*.*=file://ipc/SECURITY_OWNERS
> > > 
> > > for changed files:
> > >   chrome/common/render_messages.h
> > >   chrome/common/common_param_traits_macros.h
> > > ***************
> > > 
> > > I'll try to understand and fix it tomorrow.
> > > 
> > > Cheers,
> > > Vaclav
> > 
> > It means that you'll need to add those two per-files lines to
> > //chrome/common/OWNERS. Suggestions on how to improve the message are
welcome,
> > sorry for the confusion.
> 
> Thanks for the explanation! I guess I was confused by the first sentence:
"Found
> changes to IPC files without a security OWNER," because my brain automatically
> added "review" to the end of that sentence. Now reading properly, it all makes
> sense, the issue was completely on my side.
> 
> So I will add the lines suggested by the script and land the CL tomorrow,
hoping
> that it is OK not to wait for re-review for that.
> 
> Cheers,
> Vaclav

I added the rules to chrome/common/OWNERS as suggested by the presubmit script
and will be sending this to CQ.

Cheers,
Vaclav

[vabr (Chromium), 2016-10-05 12:19:02]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-10-05 12:19:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, mkwst@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2387613002/#ps60001
(title: "Cover param_traits in chrome/common/OWNERS, as requested by the
presubmit script")

[commit-bot: I haz the power, 2016-10-05 12:19:15]

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-10-05 13:32:57]

Description was changed from

==========
Merge Content Settings IPCs back to chrome/common/render_messages.h

This reverts https://codereview.chromium.org/798923003 (manually, because the
context code has changed significantly). The separation of content settings
IPCs was done to prepare componentising of
chrome/renderer/content_settings_observer.cc, which was not needed in the end.
As jam@ points out in https://codereview.chromium.org/2375333003/#msg9, it
should be cleaned-up.

BUG=384874,387075
==========

to

==========
Merge Content Settings IPCs back to chrome/common/render_messages.h

This reverts https://codereview.chromium.org/798923003 (manually, because the
context code has changed significantly). The separation of content settings
IPCs was done to prepare componentising of
chrome/renderer/content_settings_observer.cc, which was not needed in the end.
As jam@ points out in https://codereview.chromium.org/2375333003/#msg9, it
should be cleaned-up.

BUG=384874,387075
==========

[commit-bot: I haz the power, 2016-10-05 13:32:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-05 13:34:53]

Description was changed from

==========
Merge Content Settings IPCs back to chrome/common/render_messages.h

This reverts https://codereview.chromium.org/798923003 (manually, because the
context code has changed significantly). The separation of content settings
IPCs was done to prepare componentising of
chrome/renderer/content_settings_observer.cc, which was not needed in the end.
As jam@ points out in https://codereview.chromium.org/2375333003/#msg9, it
should be cleaned-up.

BUG=384874,387075
==========

to

==========
Merge Content Settings IPCs back to chrome/common/render_messages.h

This reverts https://codereview.chromium.org/798923003 (manually, because the
context code has changed significantly). The separation of content settings
IPCs was done to prepare componentising of
chrome/renderer/content_settings_observer.cc, which was not needed in the end.
As jam@ points out in https://codereview.chromium.org/2375333003/#msg9, it
should be cleaned-up.

BUG=384874,387075

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

[commit-bot: I haz the power, 2016-10-05 13:34:54]

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