Extensions: Pass current channel and feature session type to extension unpack utility process.

Extensions: Pass current channel and feature session type to extension unpack utility process.

The extension unpack utility process service uses Extension::Create which
involves availability checks for extension features. However we don't currently
set the current channel and feature session type in the utility process. This CL
fixes the issue by passing the current channel and feature session type to the
extension unpack utility process.

BUG=734854
Review-Url: https://codereview.chromium.org/2952353002
Cr-Commit-Position: refs/heads/master@{#483282}
Committed: https://chromium.googlesource.com/chromium/src/+/456585e962d667fdf18f17d69e3115dfac1d8e8b

[karandeepb, 2017-06-23 01:16:26]

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

[commit-bot: I haz the power, 2017-06-23 01:16:44]

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, 2017-06-23 03:14:34]

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

[commit-bot: I haz the power, 2017-06-23 03:14:35]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2017-06-23 03:27:01]

Description was changed from

==========
Utility process state.
==========

to

==========
Extensions: Pass current channel and feature session type to extension unpack
utility process.

The extension unpack utility process service uses Extension::Create which
involves availability checks for extension features. However we don't currently
set the current channel and feature session type in the utility process. This CL
fixes the issue by passing the current channel and feature session type to the
extension unpack utility process.

BUG=734854
==========

[karandeepb, 2017-06-23 03:29:57]

karandeepb@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[karandeepb, 2017-06-23 03:29:57]

PTAL Devlin. I am not sure if there's a neat way to test this, since the
availability checks don't influence the information passed back to the browser
process. (They show up as warnings and not errors).

[Devlin, 2017-06-24 00:46:42]

I think this lgtm, but you'll need an IPC reviewer as well.

[karandeepb, 2017-06-24 02:32:28]

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

[karandeepb, 2017-06-24 02:32:29]

PTAL dcheng@ for IPC security review for changes to *traits file.

[dcheng, 2017-06-24 03:30:53]

the ipc changes look fine (with one comment)

however, can we add a test for this?

https://codereview.chromium.org/2952353002/diff/20001/extensions/common/featu...
File extensions/common/feature_channel_param_traits.h (right):

https://codereview.chromium.org/2952353002/diff/20001/extensions/common/featu...
extensions/common/feature_channel_param_traits.h:11:
IPC_ENUM_TRAITS_MAX_VALUE(version_info::Channel, version_info::Channel::STABLE)
Hmm. Do we want to put everything in separate files here? I wonder if we just
want to have a common param traits for enums?

[karandeepb, 2017-06-26 21:51:55]

I wasn't able to think of a clean way to test this. As I commented in
https://codereview.chromium.org/2952353002/#msg7, this is primarily because the
result of these changes do not manifest anywhere in the output of the utility
process. (I came across this bug while prototyping something which does depend
on this though).

https://codereview.chromium.org/2952353002/diff/20001/extensions/common/featu...
File extensions/common/feature_channel_param_traits.h (right):

https://codereview.chromium.org/2952353002/diff/20001/extensions/common/featu...
extensions/common/feature_channel_param_traits.h:11:
IPC_ENUM_TRAITS_MAX_VALUE(version_info::Channel, version_info::Channel::STABLE)
On 2017/06/24 03:30:53, dcheng (OOO Jun 25 - Jul 1) wrote:
> Hmm. Do we want to put everything in separate files here? I wonder if we just
> want to have a common param traits for enums?

I feel this is better than having a single common param traits file, since this
allows for more granularity, which is needed for example while specifying
"traits_headers" in mojo typemaps.

[karandeepb, 2017-06-26 21:58:36]

karandeepb@chromium.org changed reviewers:
+ estark@chromium.org

[karandeepb, 2017-06-26 21:58:37]

estark@: Since dcheng@ is OOO, can you do the IPC security review. Thanks.

[dcheng1, 2017-06-26 22:09:48]

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

[dcheng1, 2017-06-26 22:09:49]

So this was manually caught in testing, but didn't have an observable effect
since the work done in the utility process still succeeded?

https://codereview.chromium.org/2952353002/diff/20001/extensions/common/featu...
File extensions/common/feature_channel_param_traits.h (right):

https://codereview.chromium.org/2952353002/diff/20001/extensions/common/featu...
extensions/common/feature_channel_param_traits.h:11:
IPC_ENUM_TRAITS_MAX_VALUE(version_info::Channel, version_info::Channel::STABLE)
On 2017/06/26 21:51:54, karandeepb wrote:
> On 2017/06/24 03:30:53, dcheng (OOO Jun 25 - Jul 1) wrote:
> > Hmm. Do we want to put everything in separate files here? I wonder if we
just
> > want to have a common param traits for enums?
> 
> I feel this is better than having a single common param traits file, since
this
> allows for more granularity, which is needed for example while specifying
> "traits_headers" in mojo typemaps.

There's something to be said for granularity, but it also results in more
multiply included headers proliferating and the amount of boilerplate that needs
to be used to make all the more granular include files get preprocessed
correctly.

Unless there's a need to expose this outside of extensions (which there doesn't
appear to be), just keeping it in the original location seems good enough to
me...

[karandeepb, 2017-06-26 22:16:44]

Yeah so I was prototyping something (which I'll try to land separately in some
time), which added some code here, which was failing due to this bug. Since
Extensions::Create does use this global state, I think this is the right thing
to do, even if it doesn't have an observable effect currently.

https://codereview.chromium.org/2952353002/diff/20001/extensions/common/featu...
File extensions/common/feature_channel_param_traits.h (right):

https://codereview.chromium.org/2952353002/diff/20001/extensions/common/featu...
extensions/common/feature_channel_param_traits.h:11:
IPC_ENUM_TRAITS_MAX_VALUE(version_info::Channel, version_info::Channel::STABLE)
On 2017/06/26 22:09:49, dcheng1 wrote:
> On 2017/06/26 21:51:54, karandeepb wrote:
> > On 2017/06/24 03:30:53, dcheng (OOO Jun 25 - Jul 1) wrote:
> > > Hmm. Do we want to put everything in separate files here? I wonder if we
> just
> > > want to have a common param traits for enums?
> > 
> > I feel this is better than having a single common param traits file, since
> this
> > allows for more granularity, which is needed for example while specifying
> > "traits_headers" in mojo typemaps.
> 
> There's something to be said for granularity, but it also results in more
> multiply included headers proliferating and the amount of boilerplate that
needs
> to be used to make all the more granular include files get preprocessed
> correctly.
> 
> Unless there's a need to expose this outside of extensions (which there
doesn't
> appear to be), just keeping it in the original location seems good enough to
> me...

"just keeping it in the original location" -> This isn't clear to me. I am
assuming you don't mean extension_messages.h since these need to be shared with
mojo? 
I'll introduce a new file to hold these enums though.

[dcheng1, 2017-06-26 23:07:35]

On 2017/06/26 22:16:44, karandeepb wrote:
> Yeah so I was prototyping something (which I'll try to land separately in some
> time), which added some code here, which was failing due to this bug. Since
> Extensions::Create does use this global state, I think this is the right thing
> to do, even if it doesn't have an observable effect currently.

Can we add a test at that time then? Assuming it lands =)

https://codereview.chromium.org/2952353002/diff/20001/extensions/common/featu...
File extensions/common/feature_channel_param_traits.h (right):

https://codereview.chromium.org/2952353002/diff/20001/extensions/common/featu...
extensions/common/feature_channel_param_traits.h:11:
IPC_ENUM_TRAITS_MAX_VALUE(version_info::Channel, version_info::Channel::STABLE)
On 2017/06/26 22:16:44, karandeepb wrote:
> On 2017/06/26 22:09:49, dcheng1 wrote:
> > On 2017/06/26 21:51:54, karandeepb wrote:
> > > On 2017/06/24 03:30:53, dcheng (OOO Jun 25 - Jul 1) wrote:
> > > > Hmm. Do we want to put everything in separate files here? I wonder if we
> > just
> > > > want to have a common param traits for enums?
> > > 
> > > I feel this is better than having a single common param traits file, since
> > this
> > > allows for more granularity, which is needed for example while specifying
> > > "traits_headers" in mojo typemaps.
> > 
> > There's something to be said for granularity, but it also results in more
> > multiply included headers proliferating and the amount of boilerplate that
> needs
> > to be used to make all the more granular include files get preprocessed
> > correctly.
> > 
> > Unless there's a need to expose this outside of extensions (which there
> doesn't
> > appear to be), just keeping it in the original location seems good enough to
> > me...
> 
> "just keeping it in the original location" -> This isn't clear to me. I am
> assuming you don't mean extension_messages.h since these need to be shared
with
> mojo? 
> I'll introduce a new file to hold these enums though.

sgtm

[karandeepb, 2017-06-26 23:28:09]

Patchset #3 (id:40001) has been deleted

[karandeepb, 2017-06-26 23:32:39]

Patchset #3 (id:60001) has been deleted

[karandeepb, 2017-06-26 23:38:07]

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

[commit-bot: I haz the power, 2017-06-26 23:38:19]

Dry run: CQ is trying da patch.

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

[karandeepb, 2017-06-27 00:35:18]

PTAL dcheng@. 

On 2017/06/26 23:07:35, dcheng1 wrote:
> On 2017/06/26 22:16:44, karandeepb wrote:
> > Yeah so I was prototyping something (which I'll try to land separately in
some
> > time), which added some code here, which was failing due to this bug. Since
> > Extensions::Create does use this global state, I think this is the right
thing
> > to do, even if it doesn't have an observable effect currently.
> 
> Can we add a test at that time then? Assuming it lands =)
Sure will add a test later.

[commit-bot: I haz the power, 2017-06-27 00:59:27]

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

[commit-bot: I haz the power, 2017-06-27 00:59:28]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-06-28 03:29:35]

lgtm

[karandeepb, 2017-06-29 01:52:02]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2017-06-29 01:52:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2952353002/#ps80001
(title: "Use a single file to hold enum traits.")

[commit-bot: I haz the power, 2017-06-29 01:52:29]

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-29 03:41:57]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498701122365900,
"parent_rev": "14d5ed2dd6b7ca8b6eea5875c9c64e142846e4b6", "commit_rev":
"456585e962d667fdf18f17d69e3115dfac1d8e8b"}

[commit-bot: I haz the power, 2017-06-29 03:42:10]

Description was changed from

==========
Extensions: Pass current channel and feature session type to extension unpack
utility process.

The extension unpack utility process service uses Extension::Create which
involves availability checks for extension features. However we don't currently
set the current channel and feature session type in the utility process. This CL
fixes the issue by passing the current channel and feature session type to the
extension unpack utility process.

BUG=734854
==========

to

==========
Extensions: Pass current channel and feature session type to extension unpack
utility process.

The extension unpack utility process service uses Extension::Create which
involves availability checks for extension features. However we don't currently
set the current channel and feature session type in the utility process. This CL
fixes the issue by passing the current channel and feature session type to the
extension unpack utility process.

BUG=734854

Review-Url: https://codereview.chromium.org/2952353002
Cr-Commit-Position: refs/heads/master@{#483282}
Committed:
https://chromium.googlesource.com/chromium/src/+/456585e962d667fdf18f17d69e31...
==========

[commit-bot: I haz the power, 2017-06-29 03:42:11]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/456585e962d667fdf18f17d69e31...