Mojo C++ bindings: provide data view for all object types.

Mojo C++ bindings: provide data view for all object types.

BUG=577686
Committed: https://crrev.com/3854898086ae1a7f0153ea0f7d6bade835a8a6d0
Cr-Commit-Position: refs/heads/master@{#407341}

[yzshen1, 2016-07-21 18:00:10]

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

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

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 18:10:52]

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

[commit-bot: I haz the power, 2016-07-21 18:10:53]

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

[yzshen1, 2016-07-21 20:41:16]

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

[commit-bot: I haz the power, 2016-07-21 20:41:38]

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 20:59:07]

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

[commit-bot: I haz the power, 2016-07-21 20:59:08]

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

[yzshen1, 2016-07-21 23:40:45]

Description was changed from

==========
[WIP] Mojo C++ bindings: provide data view for all object types.

BUG=577686
==========

to

==========
Mojo C++ bindings: provide data view for all object types.

BUG=577686
==========

[yzshen1, 2016-07-21 23:40:46]

yzshen@chromium.org changed reviewers:
+ dcheng@chromium.org, rockot@chromium.org

[yzshen1, 2016-07-21 23:41:05]

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

[commit-bot: I haz the power, 2016-07-21 23:41:33]

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

[yzshen1, 2016-07-21 23:42:56]

Hi, Ken and Daniel.

Would you please take a look? Thanks!

Ken: everything
Daniel: changes in OWNERS files. I added security owners for
mojo/public/interfaces/bindings and removed security owners for
mojo/public/interfaces/bindings/tests

[commit-bot: I haz the power, 2016-07-21 23:48:20]

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

[commit-bot: I haz the power, 2016-07-21 23:48:20]

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

[dcheng, 2016-07-22 02:30:25]

On 2016/07/21 23:42:56, yzshen1 wrote:
> Hi, Ken and Daniel.
> 
> Would you please take a look? Thanks!
> 
> Ken: everything
> Daniel: changes in OWNERS files. I added security owners for
> mojo/public/interfaces/bindings and removed security owners for
> mojo/public/interfaces/bindings/tests

The PRESUBMIT will complain if it doesn't find the lines it's looking for, so
I'd just leave SECURITY_OWNERS in there (despite the fact that security owners
should probably never actually have to look at those changes).

[Ken Rockot(use gerrit already), 2016-07-22 16:12:26]

On 2016/07/22 at 02:30:25, dcheng wrote:
> On 2016/07/21 23:42:56, yzshen1 wrote:
> > Hi, Ken and Daniel.
> > 
> > Would you please take a look? Thanks!
> > 
> > Ken: everything
> > Daniel: changes in OWNERS files. I added security owners for
> > mojo/public/interfaces/bindings and removed security owners for
> > mojo/public/interfaces/bindings/tests
> 
> The PRESUBMIT will complain if it doesn't find the lines it's looking for, so
I'd just leave SECURITY_OWNERS in there (despite the fact that security owners
should probably never actually have to look at those changes).

Can we just add a wildcard line too to any OWNERs files for test-only mojom?
Then we can avoid unnecessary review overhead and not have to change the
presubmit.

[yzshen1, 2016-07-22 18:46:50]

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

[commit-bot: I haz the power, 2016-07-22 18:47:37]

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

[yzshen1, 2016-07-22 18:51:18]

On 2016/07/22 16:12:26, Ken Rockot wrote:
> On 2016/07/22 at 02:30:25, dcheng wrote:
> > On 2016/07/21 23:42:56, yzshen1 wrote:
> > > Hi, Ken and Daniel.
> > > 
> > > Would you please take a look? Thanks!
> > > 
> > > Ken: everything
> > > Daniel: changes in OWNERS files. I added security owners for
> > > mojo/public/interfaces/bindings and removed security owners for
> > > mojo/public/interfaces/bindings/tests
> > 
> > The PRESUBMIT will complain if it doesn't find the lines it's looking for,
so
> I'd just leave SECURITY_OWNERS in there (despite the fact that security owners
> should probably never actually have to look at those changes).
> 
> Can we just add a wildcard line too to any OWNERs files for test-only mojom?
> Then we can avoid unnecessary review overhead and not have to change the
> presubmit.

Thanks for suggestions!

The current PRESUBMIT looks for the following lines in OWNERS of the same
folder:
per-file *.mojom=set noparent
per-file *.mojom=file://ipc/SECURITY_OWNERS
(The parent folder has already had these lines is not sufficient.)

We cannot just use a wildcard there. PRESUBMIT will be unhappy.

[Ken Rockot(use gerrit already), 2016-07-22 19:01:39]

What about:

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

Does that not work?

On Fri, Jul 22, 2016 at 11:51 AM, <yzshen@chromium.org> wrote:

> On 2016/07/22 16:12:26, Ken Rockot wrote:
> > On 2016/07/22 at 02:30:25, dcheng wrote:
> > > On 2016/07/21 23:42:56, yzshen1 wrote:
> > > > Hi, Ken and Daniel.
> > > >
> > > > Would you please take a look? Thanks!
> > > >
> > > > Ken: everything
> > > > Daniel: changes in OWNERS files. I added security owners for
> > > > mojo/public/interfaces/bindings and removed security owners for
> > > > mojo/public/interfaces/bindings/tests
> > >
> > > The PRESUBMIT will complain if it doesn't find the lines it's looking
> for,
> so
> > I'd just leave SECURITY_OWNERS in there (despite the fact that security
> owners
> > should probably never actually have to look at those changes).
> >
> > Can we just add a wildcard line too to any OWNERs files for test-only
> mojom?
> > Then we can avoid unnecessary review overhead and not have to change the
> > presubmit.
>
> Thanks for suggestions!
>
> The current PRESUBMIT looks for the following lines in OWNERS of the same
> folder:
> per-file *.mojom=set noparent
> per-file *.mojom=file://ipc/SECURITY_OWNERS
> (The parent folder has already had these lines is not sufficient.)
>
> We cannot just use a wildcard there. PRESUBMIT will be unhappy.
>
>
>
> https://codereview.chromium.org/2165233003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2016-07-22 19:09:25]

On 2016/07/22 19:01:39, Ken Rockot wrote:
> What about:
> 
> per-file *.mojom=set noparent
> per-file *.mojom=file://ipc/SECURITY_OWNERS
> per-file *.mojom=*
> 
> Does that not work?

I see what you meant. Yes, I think this works.
I was thinking that it is good to use the same OWNERs as other files in //mojo.
It is unlikely that people will touch these files without making changes to any
other //mojo files anyway.

WDYT?

[Ken Rockot(use gerrit already), 2016-07-22 19:10:43]

SGTM, also I meant to LGTM before!

On Fri, Jul 22, 2016 at 12:09 PM, <yzshen@chromium.org> wrote:

> On 2016/07/22 19:01:39, Ken Rockot wrote:
> > What about:
> >
> > per-file *.mojom=set noparent
> > per-file *.mojom=file://ipc/SECURITY_OWNERS
> > per-file *.mojom=*
> >
> > Does that not work?
>
> I see what you meant. Yes, I think this works.
> I was thinking that it is good to use the same OWNERs as other files in
> //mojo.
> It is unlikely that people will touch these files without making changes
> to any
> other //mojo files anyway.
>
> WDYT?
>
> https://codereview.chromium.org/2165233003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-07-22 19:11:11]

lgtm

[Fady Samuel, 2016-07-22 19:12:01]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-07-22 19:12:02]

Drive by excitement! wooohooo

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

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

[commit-bot: I haz the power, 2016-07-22 20:22:22]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-07-23 01:40:26]

I don't think my LGTM is required, but LGTM for the owners changes.

[yzshen1, 2016-07-23 02:18:29]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2016-07-23 02:18:43]

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-23 03:39:06]

Description was changed from

==========
Mojo C++ bindings: provide data view for all object types.

BUG=577686
==========

to

==========
Mojo C++ bindings: provide data view for all object types.

BUG=577686
==========

[commit-bot: I haz the power, 2016-07-23 03:39:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-23 03:40:50]

Description was changed from

==========
Mojo C++ bindings: provide data view for all object types.

BUG=577686
==========

to

==========
Mojo C++ bindings: provide data view for all object types.

BUG=577686

Committed: https://crrev.com/3854898086ae1a7f0153ea0f7d6bade835a8a6d0
Cr-Commit-Position: refs/heads/master@{#407341}
==========

[commit-bot: I haz the power, 2016-07-23 03:40:51]

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